Beefy Boxes and Bandwidth Generously Provided by pair Networks
Welcome to the Monastery

Tact and the Monastery

by FoxtrotUniform (Prior)
on Sep 14, 2002 at 06:33 UTC ( #197817=perlmeditation: print w/replies, xml ) Need Help??

First of all, this isn't yet another harangue about stupid user names or Chatterbox pranks.

I recently had the chance to look over some of a colleague's Perl code. It was nasty -- thousands of lines of code and maybe twenty lines of subroutines, poorly chosen variable names, inconsistent coding style... the list goes on and on. Problem is, I wasn't the designated code reviewer, and I would have felt awkward (not to mention rude) had I simply confronted him with my opinion: "Dude, your code sucks."

Instead, I suggested that he register an account on Perl Monks.

And why not? When I started here, I had a functioning grasp of the language, but fsck-all idea of how to write decent (much less elegant) Perl code. I have since improved by several orders of magnitude, in no small part thanks to this site. There's no reason to expect that our combination of expert knowledge and newbie-friendliness won't help him just as much.

Just another way to respond the next time you run across some atrocious Perl code....

F o x t r o t U n i f o r m
Found a typo in this node? /msg me
The hell with paco, vote for Erudil!

Edited: ~Mon Sep 16 16:45:24 2002 (GMT) by footpad: Moved to Meditations, per Consideration.

Replies are listed 'Best First'.
Re: Tact and the Monastery
by clintp (Curate) on Sep 14, 2002 at 15:01 UTC
    A technique I've employed is holding a code review of my own code. There's an awful lot you can say about someone else's code and have it come off as self-criticism. first I had it coded like this {show nasty code} and decided it'd be more efficient/pretty/maintainable like this {show good code}...
    and then go on to explain why it was more maintainable/pretty/efficient.

    And you don't really have to beat it to death either, and certainly not drop any snide remarks about the bad stuff you've seen in their code. Just make sure that the bad stuff you *do* show them is your own: they may recognize their own code style.

      Wholeheartedly agree. A post worth more than just a ++

      Makeshifts last the longest.

(jeffa) Re: Tact and the Monastery
by jeffa (Bishop) on Sep 14, 2002 at 12:57 UTC
    Thank you FoxtrotUniform. In a short while i will be traveling 300 or so miles to meet with a client who has an application that is ... "not quite stylish". ;) This was the first post i read this morning and it has served as a reminder for me to not mention this to the client as we work to fix it's ... "unique quirks" this afternoon. This is a good attitude to maintain for all code, not just Perl - however, we are extremely fortunate to have this community. While it is easier to put down someone's code, it can more rewarding <cough>$$</cough> to work with it rather than against it. Cheers! :)


    (the triplet paradiddle with high-hat)
Re: Tact and the Monastery
by Ovid (Cardinal) on Sep 14, 2002 at 16:55 UTC

    Interesting ... we just hired a new programmer and I recommended that he get an account here. His code seems good and solid and I definitely signed off on his code review. However, he has had no exposure to the Perl community. Personally, I was rather surprised by that as I've noticed that there tends to be a correlation between code quality and exposure to the community (and I don't just mean Perlmonks).

    I have had the same experience as you regarding self-improvement since I've come to this site. I've heard some complain about the signal to noise ratio or they think that the Monks are too dogmatic, but while I would choose different labels, I wouldn't disagree with those statements: these qualities often foster spirited debate and when I get to read long threads of pros and cons of a particular thing, I actually have a chance to see a variety of viewpoints. When's the last time you saw that in some dusty ol' computer book? :)


    Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

      They complain about signal to noise ratio..? These must be some rather picky folks. I honestly haven't seen anything that beats the monastery in that regard.

      Makeshifts last the longest.

Re: Tact and the Monastery
by kschwab (Vicar) on Sep 14, 2002 at 15:02 UTC

    I'm always leery of these "this other guys code was a mess" comments. Certainly it's sometimes (mostly?) true, but "messy" is in the eye of the beholder.

    For example, I tend to use lots of temporary variables where they probably aren't needed. I've gotten negative feedback on that. I think, however, that they make the code more maintainable:

    # crypt the password my @chars=(a..z,A..Z,0..9,'.','/'); my $salt= $chars[rand(@chars)] . $chars[rand(@chars)]; my $crypted=crypt($password,$salt);
    Now, the @chars and $salt variables aren't really needed, and I could have done something like:
    my $crypted = crypt( $password, join( '', ('.', '/', 0..9, 'A'..'Z', 'a'..'z')[rand 64, rand 64]));
    which is more compact, but less readable, IMO.

    I've also gotten negative feedback on code reviews for overly defensive programming, which again, adds lines of code and generally makes for more "mess". ( additional eval blocks, lots of defined() calls, etc).

      My rule is that a temporary variable is fine if it can be related to the application in some way, and not just an artifact of optimization. One way to tell is if you can give it a truly meaningful name. If you can't, see if you can eliminate it.

      -- Randal L. Schwartz, Perl hacker

        My rule is that a temporary variable is fine if it can be related to the application in some way, and not just an artifact of optimization.
        One way to tell is if you can give it a truly meaningful name. If you can't, see if you can eliminate it.

        There is sense & beauty in that. But it does require a high degree of Perl knowledge.

        Corollaries can be derived.

        Temporary variables should be named so that it is obvious that they are temporaries.

        Temporary variables should be scoped to advertise their ephemeral nature.

        For those who can't readily parse dense Perl temporary variables are like the x modifier to m//.

        a temporary variable is fine if it can be related to the application in some
        way, ... One way to tell is if you can give it a truly meaningful name. If you can't,
        see if you can eliminate it.

        From early on I have been a believer in good names. The above as inspired me to
        raise the level of quality I try to achieve in naming. A good name should
        clarify obscure/advanced/complex/efficient 1 code by expressing what the variable
        should be.

        It is going to be tough to break the habit of using result for return values.

        1 Choose one. Define yourself.
      I find your first example unnecessarily noisy, while the second is too golfish. Balance is key. Mine would look like so:
      my @chars = ('a' .. 'z', 'A' .. 'Z', 0 .. 9, '.', '/'); my $crypted = crypt $password, $chars[rand @chars].$chars[rand @chars] +;

      That way I can't make a mistake in the size of the list vs the constant passed to rand. Nor will I have to figure out why I used 64 there if I read the source again in a year. It also lets me use concatenation vs the noisy and distracting join/slice method.

      $salt on the other hand is superfluous - that value is trivially calculated on the fly.

      Makeshifts last the longest.

      OTOH... ;)

      I personally think the second of your examples really is more readable. But maybe I'm just a bit nuts.

      Myself, I use as few temporary (synthetic) variables as possible, I think they often make things harder to understand. Even if I immediately get the idea what @chars is, I would glance back on every occurance of it, to make sure what your idea of "characters" really is (with or without underscore, hypen, etc.). If the code spreads out, things become even worse...

      In your second example, it's right there.

      Which brings us back to the topic of this thread. While I am sure that there are some style ideas, any good Perlmonk would propagate, some other things really are a matter of taste.

        I don't like the comma-prepending style. You can easily add a new line at the bottom that way, but not at the top. It might be fine for languages that are inflexible about their commata, but Perl is not - you can append one to the last value with no harm. My formatting looks like this:
        my %hash = ( one => 1, two => 2, three => 3, );
        Note the "errant" comma after the last value. Separating the assignment and closing bracket on their own lines adds more convenience. This way, I can add a hash key anywhere - even at the bottom or the top - without having to edit any other line than the one I'm on.

        Makeshifts last the longest.

      I'm with you on this one kschwab.

      I certainly try not to use too many temporary, intermediate variables, but I do if they add to readability for a future maintainer of the code.

      My basic criteria for using a temporary variable is this:
      If I can think up a meaningful, transparent name for it, I can use it.

      For example, in your case, @chars and $salt are both meaningfully named, thus allowed. What I would not allow would be @array and $foo for the same variables.

      Helgi Briem

      I would probably change that to look like
      # crypt the password my $crypted; { my @chars=(a..z,A..Z,0..9,'.','/'); my $salt= $chars[rand(@chars)] . $chars[rand(@chars)]; $crypted=crypt($password,$salt); }
      so that the temporary variables (@chars and $salt) disappear after the closure ends. And I agree with the suggestion lower down: you could lose $salt and incorporate the salting in the call to crypt.

      --t. alex
      but my friends call me T.

        That's not a closure. :-) It's just a naked block.

        Makeshifts last the longest.

Re: Tact and the Monastery
by jryan (Vicar) on Sep 14, 2002 at 07:14 UTC
    Of course, if he knows your username, he'll now know your true opinion on his code ;)

      ...or if he reads the frontpage, or newest nodes, and can put 2 and 2 together to get 4.

      Warning: Unless otherwise stated, code is untested. Do not use without understanding. Code is posted in the hopes it is useful, but without warranty. All copyrights are relinquished into the public domain unless otherwise stated. I am not an angel. I am capable of error, and err on a fairly regular basis. If I made a mistake, please let me know (such as by replying to this node).

Log In?

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

How do I use this? | Other CB clients
Other Users?
Others pondering the Monastery: (2)
As of 2022-01-19 04:00 GMT
Find Nodes?
    Voting Booth?
    In 2022, my preferred method to securely store passwords is:

    Results (55 votes). Check out past polls.