in reply to Way to make this code better.

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

Replies are listed 'Best First'.
Re^2: Way to make this code better.
by builat (Monk) on Aug 15, 2015 at 13:46 UTC
    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.
[reply]
      $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

[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom