Can someone tell me how to do this best way?

I have no idea about what your are doing, an don't know of the context.
But to make your code more readable, get your indenting right. If you are lazy, use perltidy. And it's prettify.

use strict;
use utf8;
use warnings;
use Mojo::JSON qw|from_json|;
use bytes;
use MBclient;

sub new {
    ... .;
}   

sub _device_read {

    # $d as data
    # $mbc as ModBusControl
    # $r as response
    my ( $d, $mbc, $r ) = ( shift, MBclient->new(), undef );
    
    # parse device config
    $d->{regs} = from_json( $d->{regs} );
    
    # setup device attributes
    $mbc->host( $d->{ip} );
    $mbc->port( $d->{port} );
    $mbc->unit_id(0);    # by default.

    # for all specified registers
    foreach my $reg ( keys %{ $d->{regs}->{r} } ) {
        $r->{$reg} = _prettify_byte(

            # read data from device registers
            $mbc->read_input_registers(

                # converting text register address to hex
                hex($reg),

                # specify number of registers to read
                $d->{regs}->{r}->{$reg}
            )   
        );
    }
    return ($r);
}   

# converts from hex data to deximal
# recieve output from modbus sensors
sub _prettify_byte {

    # okay state of deal
    return (
        sprintf( '%.2f', unpack( 'f', pack( 's2', @{ $_[0] }[ 1, 0 ] )
+ ) ) )
      if ( $_[0] );
      
    #return error message
    return ( $c{_err_} );
}
[download]

Shame on me. Thnx. Well this is a part of mojolicious method. It collects data from sensors. Some other methods sends driver signals and set on\off devices.
So simply it is climatic data collector.

I don't have anything to add regarding functionality, but I've got a few ideas for readability.

First thing I'd do is change this:

# $d as data
# $mbc as ModBusControl
# $r as response
my ( $d, $mbc, $r )=( shift, MBclient->new(), undef );
[download]

...to this:

my $data = shift;
my $mod_bus_control = MBClient->new;
my $response = undef;
[download]

...and then rename all the variables below this code to the proper names. It's difficult to have to scroll through code to find the legend of what the variable $r stands for, and I guarantee you'll forget in three months when you're trying to do more work on your code. You could have $r in this sub meaning one thing, and in other subs, something totally different. Just use the real names.

Also, remove most of the comment lines. There's no need to say something like # read... or # return... when the next line below are read_input_registers and return($r). In almost all cases, the code speaks for itself. If you want a piece to stand out (like a return), separate it with whitespace on the lines above and below.

UPDATE: I'd also change this:

$mbc->host    ( $d->{ip}   );
$mbc->port    ( $d->{port} );
$mbc->unit_id ( 0 );
[download]

...to this:

$mbc->host( $d->{ip} );
$mbc->port( $d->{port} );
$mbc->unit_id( 0 );
[download]

I had to do a double-take to realize those were method calls with params, with the parens so far away. Parens should be right next to the function name. The whitespace *inside* of the parens is personal preference though. Depending on the situation, I use whitespace between the parens and params, and most of the times not.

-stevieb

Thank you.
Well, in my project I have one style for all var names. $r stands for response, $d for data. In all methods no exeptions.
But it is good idea that someone can see and reuse my code. So I'll try to rewrite public version of it.

$r stands for response, $d for data

This would never pass code review in many shops. See for example Single Letter Variable Names Still Considered Harmful.

If $r is for the response or result, say $response or $result. Better yet, call it something meaningful, related to what it actually is, and not whether it's an input parameter or gets returned. In the OP, this might be $prettified_registers or somesuch.

To make my point for me, line 50 conveniently provides:

    return( $c{_err_} );
[download]

Note that $c is not mentioned in the OP code snippet anywhere else, it is not obvious what it is, etc.

A slightly weaker complaint is using literal strings to index hashes (_err_). In a large project, typos abound. This code may never be exercised in testing, and a typo here will not be caught until much too late. Debugging typos in hash keys is awkward. And there is no "great" solution in Perl. A reasonable stab is to use variables to hold the literals, such as:

my $some_key = q/Some Key/;
$hash{$some_key} = 42;
[download]

Then at least, the compiler will complain for most typos. But it won't complain if you mix in literals, so you have to be consistent. If you're implementing classes anyway, you can then use object accessors instead, and check that way.

-QM
--
Quantum Mechanics: The dreams stuff is made of

Hello builat

A couple of things have been mentioned already. Apart from that there are 2 things that I find pretty bad about your code:

a) _device_read() is misnamed

sub _device_read{
   my $d = shift;  # reduced line to relevant parts

   ...

   # parse device config
   $d->{regs}=from_json( $d->{regs} );
[download]

You are modifying the provided hash reference but the function name implies no such thing. A more suitable description would be something like '_convert_registers_and_device_read' if this is intentional or assign the result of from_json($d->{regs}) to a temporary variable if it is not. As a third option you could extract the JSON-conversion into a different function.

b) error handling

sub _device_read{
  ...
  $r->{$reg} = _prittyfy_byte(...);
  ...
};

sub _prettify_byte{
    return (...) if( $_[0] );
    return( $c{_err_} );
};
[download]

If prettify fails, then it returns a hash key from a global variable? Why even bother to return something if it's a static string? Why not simply return undef? (If it's an error message, then you can access $c{_err_} later on anyway?)

additional comment 1: Why do you convert a byte into a float in the first place?

additional comment 2: How does this work? ('_prettify_byte([1, 2])' results in 0.00)

General comments

Technically there's nothing wrong with optimizing your code to use as few variables as possible. From a maintenance / debug POV I think that is bad. If I can split a computation into separate steps then usually I put the value into a suitably named variable. My loop could end up like this:

my %registers = %{$d->{regs}->{r}};
foreach my $reg (keys %registers){
  my $reg_id  = hex $reg;
  my $reg_num = $registers{$reg};

  # read_input_registers returns an array reference
  my $tmp = $mbc->read_input_registers($reg_id, $reg_num);

  $r->{$reg} = _prettify_byte($tmp);
};
[download]

my %registers = %{$d->{regs}->{r}};
foreach my $reg (keys %registers){
  # read_input_registers returns an array reference
  my $tmp = $mbc->read_input_registers(hex $reg, $registers{$reg});

  $r->{$reg} = _prettify_byte($tmp);
};
[download]

...but not as a single statement that spans multiple lines and calls 2 different functions.

Why? I'm optimizing for debugging / maintenance and not for performance. If performance turns out to be an issue, then I can still optimize for it later on. But first I need to get the code correct. Having variables I can inspect and validate is a nice thing. Being able to tell at a glance if a statement looks reasonable is another.

Apart from that: I dislike your coding convention regarding $d and $r. I think it's totally fine to use a single letter variable inside a small loop (let's say 4 lines of code). For everything longer than that I'd like to have a descriptive name. Even if it's just 'input' and 'result'. If it works for you, then fine. I'm just saying I don't like it.