Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl-Sensitive Sunglasses
 
PerlMonks  

Help with Coding Style

by Seij (Initiate)
on Dec 25, 2009 at 16:24 UTC ( [id://814355]=perlquestion: print w/replies, xml ) Need Help??

Seij has asked for the wisdom of the Perl Monks concerning the following question:

Hello there,

I am new to Perl and I am not sure if my Coding Style is good.

Could some of you please check if my Coding Style is good or tell me what I should change to get a good Coding Style?

You can find 2 files of my current project under http://www.christophfriedrich.de/code/

Please check it so that I can write good code on bigger projects.

Thank you and greetings

Christoph

Replies are listed 'Best First'.
Re: Help with Coding Style
by CountZero (Bishop) on Dec 25, 2009 at 16:46 UTC
    You can run your code past Perl::Critic to check the coding style. There is even an online version so you do not have to install this module: Perl Critic Online.

    Some program editors (such as Komodo) now even incorporate an "as you type" Perl::Critic check.

    CountZero

    A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James

Re: Help with Coding Style
by erix (Prior) on Dec 25, 2009 at 16:53 UTC
Re: Help with Coding Style
by ww (Archbishop) on Dec 25, 2009 at 21:33 UTC
    And (NOT finally, as 'style wars' can make 'religious wars' seem tame by comparison), please avoid making off-site links integral to your question.

    Because the linked material or its address may change, making it part of your question may devalue the answers for future readers -- if-and-when they can't find the source.

Re: Help with Coding Style
by amir_e_a (Hermit) on Dec 25, 2009 at 19:41 UTC

    First of all, i strongly join the people who recommended using Perl::Critic. It is an excellent to improve your style and avoid bugs.

    You should also use Perl::Tidy.

    Your naming, indentation and spacing style is mostly OK, but there are some lines which may be wrong.

    Among them:

    my $self = bless ({}, ref ($class) || $class); # This is not necessari +ly wrong, but make sure that you understand exactly what it means. $self->{'allowed'} = (); # This variable is not properly defined as a +list. my $size = $options{'size'} || 9; # This may not do what you want. If +you use Perl 5.10 (you should!), consider using // instead of ||. for (my $i = 1; $i <= $size; $i++) # This is OK, but it is C style, no +t Perl. Consider writing: for my $i (1 .. $size) # This is more Perlish and more readable. It's +a matter of taste. $self->set_value(value => $value) if ($value > 0); # This may be it's +correct for Sudoku, but what if $value is supposed to 0? Consider: if (defined $value) my ($self) = @_; # You should probably write: my ($self) = shift; # Option 1. my ($self) = $_[0]; # Option 2. The two options do a very similar thin +g, choose the one that suits you and make sure you understand what th +ey mean. $cells[$row] = (); # You probably want to write: $cells[$row] = []; # Read the documents perllol and perlreftut in the +standard Perl documentation. (If you're don't know where to find them +, you can just Google these names :)

    Another hint: If you only use integer values in your program, consider writing `use integer' in the beginning. It may improve performance. But compare your results with and without it.

    Good luck!

      First of all, i strongly join the people who recommended using Perl::Critic. It is an excellent to improve your style
      Actually, using Perl::Critic doesn't help you to improves one style. It will however help you mimic the style of the people in charge of Perl::Critic.
      my ($self) = @_; # You should probably write: my ($self) = shift; # Option 1. my ($self) = $_[0]; # Option 2. The two options do a very similar thin +g, choose the one that suits you and make sure you understand what th +ey mean.
      I loathe this kind of advice. You give two alternatives to a line, without any explanation of why any of the alternatives are better. I certainly don't see any problem with the first line than any of the alternatives solves.
      for (my $i = 1; $i <= $size; $i++) # This is OK, but it is C style, no +t Perl.
      It's Perl. Hence, it's Perl style. Just because it looks like C doesn't mean it's bad.
      for my $i (1 .. $size) # This is more Perlish and more readable. It's +a matter of taste.
      If it's a matter of taste, why recommend one over the other?

      Personally, I also look at the body when deciding what kind of loop to use. In this case, I wouldn't use a for statement at all.

      $self->{allowed} = [undef, (1) x $size];
      I suspect $self->{allowed}[0] is never used, but I didn't look at the code a lot. If it's not used, I wouldn't use the undef in the code above.
        Actually, using Perl::Critic doesn't help you to improves one style. It will however help you mimic the style of the people in charge of Perl::Critic.
        That may be but when the guidelines of Perl::Critic are quite sensible and when there is also the possibility to edit its rules to suit your own style, I find Perl::Critic really quite helpful.

        CountZero

        A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James

        Actually, using Perl::Critic doesn't help you to improves one style. It will however help you mimic the style of the people in charge of Perl::Critic.

        ... Who more or less mimic the style suggested by Damian Conway. Since i started using Perl::Critic, i get from an idea to working bugless code faster. Everyone is free not to use it.

        I loathe this kind of advice. You give two alternatives to a line, without any explanation of why any of the alternatives are better. I certainly don't see any problem with the first line than any of the alternatives solves.

        Writing my ($self) = @_ is not wrong, but it may mean that the coder - who admits to being a novice - is not sure that he knows what it means. I am showing other ways to pass args, TMTOWTDI.

        You may loathe it, but i hope that it encourages people to read documentation. Maybe i'm wrong.

Re: Help with Coding Style
by Khen1950fx (Canon) on Dec 25, 2009 at 22:58 UTC
    Drawing upon the suggestions of CountZero and amir_e_a, I'd say use Perl::Critic and Perl::Tidy. The critic is relative. Sometimes, I'll follow its advice, sometimes not. It all depends on the context of the script that I'm working on. An easy way to use the critic is to add use criticism; to your script. You also want to add use diagnostics; and use Perl::Tidy; Using Board.pm as an example:

    package DragonNet::Games::Sudoku::Board; BEGIN { $diagnostics::DEBUG = 1, $diagnostics::PRETTY = 1 } use strict; use warnings; use criticism 'harsh'; use diagnostics -verbose; use Data::Dumper; use Perl::Tidy; Perl::Tidy::perltidy(); use DragonNet::Games::Sudoku::Board::Cell; # Initialise new Sudoku Board # Params: # size => Size of the new Sudoku Board (optional) # Returns: # Object of the class DragonNet::Games::Sudoku::Board sub new { my ($class, %options) = @_; my $self = bless({}, ref ($class) || $class); # Array with cells my @cells = (); # Get the size from the options # If size is not specified set it to 9 # If size is smaller than 9 set it to 9 (smallest Sudoku Board) # If the squareroot of size is not a natural number set it to the +next smaller size my $size = $options{'size'} || 9; $size = 9 if $size < 9; $size = int(sqrt($size)) ** 2 if $size != int(sqrt($size)) ** 2; $self->{'size'} = $size; # Initialise cells for (my $row = 1; $row <= $size; $row++) { $cells[$row] = (); for (my $col = 1; $col <= $size; $col++) { $cells[$row][$col] = DragonNet::Games::Sudoku::Board::Cell +->new(size => $size); } } $self->{'cells'} = \@cells; return $self; } # Initialise new Sudoku Board and set the values of the cells to the v +alue at # the according string position # Params: # string => String with the values (if not specified creates an e +mpty board) # Returns: # Object of the class DragonNet::Games::Sudoku::Board sub new_from_string { my ($class, %options) = @_; my $self = bless({}, ref ($class) || $class); # Array with cells my @cells = (); # If the parameter string is not specified create an empty board my $string = $options{'string'} || return $class->new(size => 9); # Get the size from the squareroot of the length of the string # If size is smaller than 9 set it to 9 (smallest Sudoku Board) # If the squareroot of size is not a natural number set it to the +next smaller size my $size = int(sqrt(length($string))); $size = 9 if $size < 9; $size = int(sqrt($size)) ** 2 if $size != int(sqrt($size)) ** 2; $self->{'size'} = $size; # Split the string into chars my @string_parts = split(//, $string); for (my $row = 1; $row <= $size; $row++) { $cells[$row] = (); for (my $col = 1; $col <= $size; $col++) { my $value = $string_parts[($row - 1) * $size + ($col - 1)] +; $value = -1 if ($value == 0); $cells[$row][$col] = DragonNet::Games::Sudoku::Board::Cell +->new(size => $size, value => $value); } } $self->{'cells'} = \@cells; return $self; } # Prints the Board in a pretty way sub print_pretty { my ($self) = @_; for (my $row = 1; $row <= $self->{'size'}; $row++) { for (my $col = 1; $col <= $self->{'size'}; $col++) { my $value = $self->get_cell_value(row => $row, column => $ +col); if ($value == -1 || !$value) { print ' '; } else { print "$value "; } } print "\n"; } print "\n"; } # Gets a specific cell # Params: # row => Row of the cell # column => Column of the cell # Returns: # Returns an object of the class DragonNet::Games::Sudoku::Board::C +ell (the specified cell) # or false if this cell does not exists sub get_cell { my ($self, %options) = @_; my $row = $options{'row'} or die "Parameter row not specified!"; my $col = $options{'column'} or die "Parameter column not specifie +d!"; return if ($row < 1 || $row > $self->{'size'}); return if ($col < 1 || $col > $self->{'size'}); return $self->{'cells'}->[$row][$col]; } # Gets the value of a specific cell # Params: # row => Row of the cell # column => Column of the cell # Returns: # Returns the value of the specific cell # or false if this cell does not exists sub get_cell_value { my ($self, %options) = @_; my $cell = $self->get_cell(%options); return if (!ref $cell); return $cell->get_value(); } # Sets a specific cell value # Params: # row => Row of the cell # column => Column of the cell # value => new value of cell # Returns: # Returns true on success else false sub set_cell_value { my ($self, %options) = @_; my $row = $options{'row'} or die "Parameter row not specified!"; my $col = $options{'column'} or die "Parameter column not specifie +d!"; my $value = $options{'value'} or die "Parameter value not specifie +d!"; return if ($row < 1 || $row > $self->{'size'}); return if ($col < 1 || $col > $self->{'size'}); return $self->get_cell(row => $row, column => $col)->set_value(val +ue => $value); } 1;
Re: Help with Coding Style
by matze77 (Friar) on Dec 26, 2009 at 10:29 UTC

    I would not think myself as experienced enough to give well advice, what i learned so far:
    Try to write the code the way that others might understand it fast (maybe you find someone who could reread and comment ...).
    Read the code a few days later (or weeks) and if you understand it fast thats a good sign.
    Use alot of Comments especially on "more complicated code blocks". I try to add a few comment lines at top of the Script to explain to people not knowing perl what my script is doing (or should do ;-)), so these people could know if it might be safe to run it ...(for the sysadmin colleagues ...)
    I am no friend of the many "cryptic shortcuts" on perl if the choice is there maybe write a little more if it is better understandable.
    Warnings should be generally enabled.
    Is a must, since it doesnt "exit|break" like strict.
    I am not a friend "of strict using strict" (but you could disable on some code blocks if you want to use it, sometimes i think it makes life a little too hard esp. with $_. But on bigger projects it might help with some common errors, for smaller code snippets its "overkill" i think. So i wouldnt blame people for not using strict ...) Just my 2ct.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://814355]
Approved by Old_Gray_Bear
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others examining the Monastery: (8)
As of 2024-03-28 15:02 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found