Beefy Boxes and Bandwidth Generously Provided by pair Networks
more useful options
 
PerlMonks  

Practical example of "Is Perl code maintainable"

by eyepopslikeamosquito (Canon)
on Aug 13, 2007 at 07:44 UTC ( #632152=perlmeditation: print w/ replies, xml ) Need Help??

Further to Some thoughts around the "is Perl code maintainable" discussion, today I inherited some code written by an external contractor. Here is one of his functions that caught my eye:

sub file_mode { my ($file) = @_; if( ! -f $file ) { return -1; } my $dummy; my $mode; ($dummy,$dummy, $mode, $dummy, $dummy, $dummy, $dummy, $dummy, $dumm +y, $dummy, $dummy, $dummy, $dummy) = stat($file); return $mode; }

Maybe he thought he was writing in a very "clear" style, but I had to reach for my sunglasses looking through this code. :-) Assuming that I don't change the (dubious) interface, my instinct would be to write this function as:

sub file_mode { -f shift or return -1; return (stat(_))[2]; }
Then I thought, maybe a Perl novice would find the original function easier to understand and maintain. I guess it's like writing, you should write to a level appropriate to your audience. What do you think?

Comment on Practical example of "Is Perl code maintainable"
Select or Download Code
Re: Practical example of "Is Perl code maintainable"
by hossman (Prior) on Aug 13, 2007 at 07:54 UTC

    There is "readable" and then there is "rambling and confusing"

    I would argue that from a maintainability standpoint, your version may be too terse for a novice, while the original version may be too verbose and full of noise. Might i suggest something like...

    sub file_mode { my ($file) = @_; -f $file or return -1; return (stat(_))[2]; # reuse stat from -f }

    ...as the appropriate middle ground.

    (NOTE: technically the two versions you posted aren't equivalent since yours only does one stat op and the original does two, but unless you expect the file perms to change after the "-f" test i'm guessing one stat is fine)

      There is "readable" and then there is "rambling and confusing"

      ++

      At the cost of using a module, I'd go a bit further if readability were really the goal.

      use File::stat qw(stat); sub file_mode { my ($path) = @_; -f $path or return -1; my $stat = stat($path) or return -1; return $stat->mode; }

      That's assuming I didn't have permission to replace whatever was calling file_mode with File::stat in the first place. Something is calling file_mode, storing the result, checking for -1, etc., etc. That part is probably just as crufty.

      Update: ysth pointed out that I omitted '-f', so I re-wrote the code to be more explicit.

      -xdg

      Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

        Bravo, I just wanted to post the suggest for File::stat but you beat me to it. I would suggest you not return -1 for two things that could be a result of a vastly different failure, and I would instead die if I failed to stat the object. I also wouldn't import stat() as failing to include File::stat would surely result in a runtime error, and not the expected compile-time. (in addition two names for the same function has a potential to confuse, as does pollution is any form)


        Evan Carroll
        www.EvanCarroll.com
Re: Practical example of "Is Perl code maintainable"
by eyepopslikeamosquito (Canon) on Aug 13, 2007 at 08:24 UTC

    Yes, I like your version more than mine, especially the nice comment.

    This example does highlight what was perhaps a design mistake in Perl 5, namely the magical performance hack via the "_" special filehandle ... which I'm sure will disappear in Perl 6. Though tricky and magical, one cannot ignore this special filehandle in practical Perl 5 programming for performance reasons, so the best practical step is to use and comment it, as you did.

    Update: Oops, responded to the wrong node. I meant to respond to hossman's node above.

Re: Practical example of "Is Perl code maintainable"
by eyepopslikeamosquito (Canon) on Aug 13, 2007 at 08:49 UTC

    Just found another:

    sub file_size { my ($file) = @_; if( ! -f $file ) { return 0; } my $dummy; my $size; ($dummy,$dummy, $dummy, $dummy, $dummy, $dummy, $dummy, $size, $dumm +y, $dummy, $dummy, $dummy, $dummy) = stat($file); return $size; }
    which was either written because he never heard of -s or maybe to silence warnings when the file does not exist. I'm tempted to replace it with:
    sub file_size { -s shift || 0 }

Re: Practical example of "Is Perl code maintainable"
by perrin (Chancellor) on Aug 13, 2007 at 12:27 UTC
    It's ugly, but I've seen people do this with time() as well. The magic return code is more of a maintainability problem, in my opinion.
Re: Practical example of "Is Perl code maintainable"
by Fletch (Chancellor) on Aug 13, 2007 at 13:02 UTC

    What jumps out at me is the use of $dummy rather than undef as the placeholder for value-to-discard-in-list-assignment, especially since this very function (stat) is used in the example of doing so in List value constructors list . . .

      I've seen valid reasons for using '$dummy' rather than 'undef', but not in this example.

      I can't remember if it was specifically a MacPerl thing, but there were some older versions where you couldn't cast undef with my, so when I'd be working with tab delim files, I'd do stuff like:

          my ($foo, $bar, $undef, $undef, $baz) = split (/\t/, $line);

      As the following would throw errors:

          my ($foo, $bar, undef, undef, $baz) = split (/\t/, $line);

      It's not a problem any longer, and the example given declares the variables in the lines before, so it wouldn't have been an issue. I'd assume that either the person was learning perl by looking at other people's old code, or had learned a while back, and hadn't kept up with the changes in the language.

Re: Practical example of "Is Perl code maintainable"
by one4k4 (Hermit) on Aug 13, 2007 at 13:15 UTC
    I don't know if you simply didn't post them, but if this function were commented (via some paragraph at the top, or between lines) and the comments were written to the audience, like you mentioned -- I would say it's just fine. Not very readable, but then again with the comments you might get an answer to the "why" (Or "why the hell??") question that pops up when looking at code like that.
Re: Practical example of "Is Perl code maintainable"
by Limbic~Region (Chancellor) on Aug 13, 2007 at 13:36 UTC
    eyepopslikeamosquito,
    If I were writing it, it would probably look something like:
    use constant MODE => 2; sub file_mode { my ($file) = @_; return -1 if ! -f $file; return (stat($file))[MODE]; }

    I think perl's shortcuts can get in the way of writing maintainable code sometimes. For instance, shift can default to two different variables depending on where in the program it appears. If I were to use shift, I would be sure to explicitly state which variable it was acting on. By avoiding shift, adding additional positional variables doesn't require modifying both sides.

    I know that using _ to avoid another stat call is a nice optimization but I would never do it in production code without a comment. I also prefer the use of constants for array indices so that another author can immediately see what is going on.

    For the record, I am not a developer and do not write production code so take my comments with a boulder of salt.

    Cheers - L~R

      Limbic~Region. I fail to see the ambiguity?

      I am aware of the two different defaults for shift, but if I'm using shift inside a subroutine, and let's face it the vast majority of uses are right up there at the top of the subroutine just below the sub, then there is no ambiguity. So explicitly naming @_ on every use of shift just seems like cargo cult to me.

      If we go down the slippery slope of deeming the implicit use of defaults as bad practice, then we're well on our way to throwing the baby out with the bath water. Today @_. Tomorrow $_.

      The day after that him, her, it, this, that, they ... :)

      As with so many of these 'to avoid confusing the newbie' measures, we're always defending against the lack of knowledge of the newbie maintenance programmer--though I doubt anyone could survive more than a month of coding Perl without becoming familiar with this--but I never see anyone owning up to how they still find it confusing after some minimal period of familiarity with perl.

      And if no one ever is still confused after some minimal period, then who are we protecting?


      Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
      "Science is about questioning the status quo. Questioning authority".
      In the absence of evidence, opinion is indistinguishable from prejudice.
        BrowserUk,
        I fail to see the ambiguity? ... So explicitly naming @_ on every use of shift just seems like cargo cult to me.

        The point I was trying to make "I think perl's shortcuts can get in the way of writing maintainable code sometimes", wasn't based on personal experience as I caveated. When I explain a piece of code to someone who doesn't know perl that well, regardless of how well they program in other languages, I find myself constantly having to explain what is implied but not said. When I explain my own code to someone who does understand perl, they still often complain that my terseness is too much (diotalevi for instance).

        To be clear, I was advocating avoiding shift entirely.

        sub foo { my $this = shift; } sub foo { my ($this, $that) = @_; } sub bar { my ($this) = @_; } sub bar { my ($this, $that) = @_; }
        I find the bar() sub faster to update with less mistakes. I know the mistake is easily found and corrected with warnings and strictures but it helps me to avoid them in the first place.

        I guess working around a bunch of Java programmers who can't understand my code has tainted me. Update: I don't know if I defended my position that well. As I said, I don't have any real world experience with production code. My limited non-professional experience tells me terse implicit code requires another person more time to understand than it would take me to write it explicitly the first time.

        Cheers - L~R

        In sum: "If we extend this practice to ridiculous extremes, then we end up doing ridiculous things. Therefore, the practice as proposed is certainly ridiculous."

        Although already having acquired a preference for avoiding using shift in particular without arguments (having actually run into code that got moved into a subroutine and thus broken, having seen uglier work-arounds for things like $hash{shift}, and just coming to the conclusion that saving three keystrokes doesn't balance the even minor loss of clarity), I finally fully adopted a "best practice" after a frustrating bought with Test.

        I was trying to fill in some details that were not clearly explained by the Test.pm documentation as to how the arguments to ok() are used. Looking at the source (search for "sub ok") we find a fairly short routine so it should be no problem to figure out how the arguments are used. Searching for @_ or $_[ finds no real uses of arguments (only checks as to whether @_ is empty). So next I had to search for all of the different ways that @_ might be used implicitly. Do you know them all off the top of your head? Are you sure? Personally, I didn't want to perform that mental exercise at that point. Turning up a few "hidden" uses of @_ still left me wondering if I'd found them all.

        So I endeavor to no longer leave such mental land-mines for others (nor for my future self). No, I was not a Perl newbie at the time (Test.pm didn't exist when I was a Perl newbie).

        Now, although I find the small improvement offered by replacing "shift" with "shift @_" to be wholely worthwhile in that particular case, I think the clarity of that subroutine would have been even more helped had shift not been used at all and, even more so, if the argument processing had not been spread over nearly the entire 60 lines of the subroutine. But those are more complex style points that I'm not convinced aren't worth "violating" in some situations so I keep them around as style guidelines to be weighed.

        But I see no even moderate down-side to being explicit with @_, so being explicit with @_ is something that I just always try to do, saving myself from spending any time wondering whether I should leave it off or not. It also saves me from having to worry about using shift by itself in a bareword context and wondering whether @ARGV was really intended. I also enjoy the presense of @ making it that much faster to parse the code.

        You can certainly assume that someone's reasons for having a policy are likely "cargo cult" in the face of no evidence either way. Making a choice to prefer to see other people in such a light is a personal matter. I note that you gave not a single benefit to leaving out @_, so I'll just presume that this is due to a personality flaw on your part, since my experience is that you wouldn't leave out such arguments if you had any. ;)

        - tye        

Re: Practical example of "Is Perl code maintainable" (golf)
by tye (Cardinal) on Aug 13, 2007 at 15:54 UTC

    Reading your proposed code, I'd think you were shooting for a golf win rather than for maintainable code. I think you make a good case for the bad influence of "golf" on the ability of Perl coders to write maintainable code, which I'm certain is not something you want to do.

    Here are some of your infractions against my personal list of best practices for Perl:

    1. Never use implicit shift; always specify @_ as in shift @_.
    2. Don't use logical operators in place of real flow control.
    3. Don't hide flow control off to the right of a statement.
    4. Don't write code that looks like it would be tripped up by Perl's "looks like a function" rule.
    5. Avoid magic numbers like "2".
    6. Use meaningful variable names (having no variable name means it can't be meaningful).

    The original code is a bit verbose and demonstrates a lack of knowledge about list slices and a lack of confidence about list assignment in Perl. I'd certainly chop all of the trailing $dummys since that just adds noise. The rest of the code is at least acceptable and most of it I much prefer over your golfed version.

    The most maintainable code I'd write would look more like:

    sub file_mode { my( $fileName )= @_; if( ! -f $fileName ) { return -1; } my( $dev, $ino, $mode )= stat( _ ); return $mode; }

    or maybe

    # The items returned by stat(): my( @Stat, %Stat ); BEGIN { @Stat= qw( dev ino mode nlink uid gid rdev size atime mtime ctime blksize blocks ); %Stat{@Stat}= ( 0 .. $#Stat ); } sub file_mode { my( $fileName )= @_; if( ! -f $fileName ) { return -1; } my $mode= ( stat _ )[ $Stat{mode} ]; return $mode; }

    Note that I didn't use File::stat because the BUGS section is unacceptable to me. ETOOMUCHMAGIC is such a common problem. I wish more Perl coders could learn to be proud of writing simple code, rather than so often nearly trying to invent a new language with each module in the apparent attempt to provide one's personal "perfect syntax" for expressing what the module is meant to address. It would have been better for File::stat to avoid overriding the name stat().

    - tye        

      It would have been better for File::stat to avoid overriding the name stat()

      But easily avoided, since symbol pollution is optional.

      use File::stat (); my $st = File::stat::stat( 'some_file' );

      -xdg

      Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

        It is even easier to avoid it by not using the module at all. The amount of code required to replace the module is small enough that it is hardly worth using the module unless it provides a marked interface improvement, which it comes close to doing. Perhaps before Perl 5.8 it fully succeeded.

        And I think it would only require (any number of) minimal changes to the module for it to be generally useful to me. As is, it just misses the mark except in limited situations. Offering to export stat()/lstat() with non-conflicting names [Stat()/LStat()] and populate() with a better name1 would certainly suffice.

        1 StatHash(stat _) ? Except File::stat doesn't produce a hash. It produces a Class::Struct object. So $st= StatObj( -d ? stat _ : lstat ) for $fileName;.

        - tye        

      I think you make a good case for the bad influence of "golf" on the ability of Perl coders to write maintainable code, which I'm certain is not something you want to do.
      Yes. This reminds me of Twenty20 versus one-day versus test cricket or blitz (five-minute) versus traditional (five hour) tournament chess. Playing too much Twenty20/one-day cricket can be poor preparation for a test series, just as playing too much blitz chess can be poor preparation for a traditional chess tournament. To which I will add that playing too much golf can be poor preparation for traditional coding. :-)

      In my defence, I always follow the Perl Best Practices "Always unpack @_ first" advice for non-trivial subroutines. It's just that these two are so trivial, I couldn't resist writing them as one-liners.

      my( $dev, $ino, $mode )= stat( _ ); return $mode; }
      I disagree with introducing the $dev and $ino variables here, given that they are not being used. Many languages will throw a "variable is initialized but never used" warning if you do that sort of thing. Apart from that, I feel it makes maintenance harder because the maintenance programmer has more variables to visually digest ... and then perhaps ponder why they are not being used. For similar reasons, I dislike capturing parens in regexes when the captured values are not being used; I find myself visually checking the code for $1, $2, ..., wondering if I've missed something.

Re: Practical example of "Is Perl code maintainable"
by cLive ;-) (Parson) on Aug 13, 2007 at 20:22 UTC

    I agree with tye - does look like golf :)

    There comes a time when you have to balance this stuff out in a shared environment. For example, I've been optimising stuff perhaps too far:)

    sub set_attr { # 0 self # 1 attr # 2 value $_[0]->{$_[1]} = $_[2]; } sub get_attr { # 0 self # 1 attr $_[0]->{$_[1]} }

    What has come from this though is that I'm moving towards a more consistant @_ usage, and then passing the script through a source filter before publishing. IE, if the sub doesn't amend any arguments unexpectedly, I write code that shifts the args

    sub set_attr { my $self = shift; my $attr = shift; my $value = shift; $self->{$attr} = $value; }

    but, before publishing, I run the code through a source filter to change it to:

    sub addNums { # 0 self # 1 attr # 2 value $_[0]->{$_[1]} = $_[2]; }
    If the source filter may cause confusion by amending any of the arguments, I use @_ explicitly:

    sub capitalize { my ($word) = @_; $word = "\u$word"; return $word; }

    I personally like using subs that tweak args direct:

    sub capitalize { $_[0]="\u$_[0]"; }

    But I find that most people feel more comfortable with:

    $word = captitalize($word);

    over

    captitalize($word);

    I'm sure there are some disadvantages to my approach, but as long as I keep it consistent, document the source filter and ensure I run tests before and after source filter is applied, I think I'm OK :)

Re: Practical example of "Is Perl code maintainable"
by DrHyde (Prior) on Aug 16, 2007 at 11:11 UTC
    sub file_mode { my $filename = shift; return -1 unless(-f $filename); return(stat($filename))[2]; }
    The only significant change from your version being avoidance of magical _ in stat and the consequent reading of the parameter into a variable.

    I might combine the two returns into one:

    return (-f $filename) ? (stat($filename))[2] : -1

    As far as I'm concerned, I'm not writing for an audience of novices. I'm writing for an audience of people employed to work on perl code, and I assume that my employer will provide training if he employed non-perl programmers. In the case of my own code, I'm writing for people who are sufficiently motivated to hack on it, who would usually be at least adequate perl hackers already. A novice with motivation can be expected to find a relevant book, or ask a question here or on his local perl mongers mailing list, or even ask me for help, and quickly stop being a novice.

    Actually, in my own code I'm normally writing for an audience of me, and I am a hacker of very little brane who can never understand the neat tricks that he wrote the night before, and so tries not to do that.

Re: Practical example of "Is Perl code maintainable"
by Anonymous Monk on Aug 18, 2007 at 11:34 UTC
    This is what you'd get out of me:
    sub file_mode { my ($file_name) = @_; if (-f $file_name) { my $mode = (stat _)[2]; return $mode; } else { return -1; } }
      Call me wool headed, but none of the samples here look maintainable to me - for one simple reason. Not one of them contains a single comment about what the code is supposed to do or what it is actually doing. Apparently perl does not support the inclusion of comments in code. I know when I'm maintaining existing code, I like to have to act like a language interpreter in my head to determine what a bunch of code is doing - comments are for noobies.
        Call me whatever you want to, but the example you've just replied to looks about as close to self-documenting as it gets.

        I wouldn't call you names but–

        1. Of course Perl has comments.
        2. Besides taking up space and concentration, comments drift and end up being lies or broken crutches a lot. They should only clarify what the code and the Pod (and the unit tests for that matter) cannot.
        3. That the code presented isn't runtime application of composed roles and mixins built dynamically with business logic stored and versioned in the DB, it's a basic file check one perldoc away. It doesn't warrant even a correct comment.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://632152]
Approved by Corion
Front-paged by Corion
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others browsing the Monastery: (8)
As of 2014-08-28 04:08 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    The best computer themed movie is:











    Results (256 votes), past polls