Beefy Boxes and Bandwidth Generously Provided by pair Networks
"be consistent"

Silly code reviews and shift

by tilly (Archbishop)
on Jan 18, 2001 at 13:11 UTC ( [id://52735] : perlmeditation . print w/replies, xml ) Need Help??

This came out of Re: Re: How to pass a variable to function selected from a hash. I was originally going to post there but figured this would be of wider interest.

cat2014 mentioned there that she had been told that shifting off the input to a function was a bad horrible thing to do. I asked her where she heard that, and she said that somewhere she worked would be really hard on you in code reviews if you had done that.

This irritated me. I don't mind code reviews, in fact I think that they are a good thing. But I don't like code reviews that teach people to think of a good chunk of the Perl core and many of the best modules on CPAN as being fundamentally bad code. I particularly don't like that when I would fail their reviews.

So I decided to get a picture of whether or not the standard made sense based on code available for a variety of purposes on a single well-known site. Namely PerlMonks....

My first stop was to my best nodes. I scanned the top 50 looking for ones where I had written a subroutine. I found that 1Y, 2Y, 3Y, and 4Y used shift. And I found that 1N and 2N did not. (I am going to keep a running tally.) Yes indeed, I would do poorly on this code review!

My second stop was to Best Nodes, what about the top 10 posts here of all time? Well 5Y used shift, eurdil had a couple of dishonourable mentions for coding style. And nobody else declared any subs.

So I decided to try to add to my test all of the Saints other than vroom and me. So I went to the good book and grabbed the top 3 posts from each with subs declared, and looked for whether they had shift. The ones that don't are a grab-bag. Demo subs without any arguments. Ones where it isn't that person's style. Weird funky ones. merlyn contributed one with pop instead which I doubt would go down any better in the reviews. davorg has an amusing one which doesn't really process its own arguments at all...

Here is what I found:

  1. merlyn shifted in 6Y, 7Y and didn't in 3N.
  2. chromatic shifted in 8Y, 9Y and didn't in 4N.
  3. Ovid shifted in 10Y, 11Y and didn't in 5N. (And wrote an impressive number of tomes, but I digress.)
  4. turnstep shifted in 12Y and 13Y and didn't in 6N.
  5. btrott clearly has strong feelings, he shifted in 14Y, 15Y and 16Y.
  6. Adam starts the tide turning. He did not shift in 6N, 7N, and 8N. Adam can ask cat2014 if he wants a job that suits him...
  7. jcwren shifts in 17Y but not in 9N and 10N.
  8. tye shifts in 18Y but not in 11N and 12N.
  9. Fastolfe shifts in 19Y but not in 13N and 14N.
  10. davorg shifts in 20Y but not in 15N and 16N.
  11. lhoward rounds out our saints by shifting in 21Y and 22Y but not in 17N.
So what do I conclude? Well I took a well-known Perl site, and took a sample of what are supposed to be its best posts. Of the ones which have functions, most used shift to process arguments at some point. So now I can with confidence tell cat2014 that she can go back to whoever did her code review and tell them that they failed me, but I find the roster of my very good fellow failures to be very comforting indeed!

Which makes me curious what other items people out there have encountered in code reviews that didn't particularly make sense to them...

Replies are listed 'Best First'.
Re: Silly code reviews and shift
by zigster (Hermit) on Jan 18, 2001 at 14:59 UTC
    In my experience code review is only a valid experience under one of two catagories.
    • The review refers to a defacto standard that has not been adhered to
    • The review offers at least basic reasoning behind a decision.
    Neither apears to have occured in this case.

    There are very few central core truths to programming. The rest are interpritation and implimentation, specifically of the core truthes. In my experience:

    • code should be as simple as it needs to be be and no more so.
    • code should be as efficient as it needs to be and no more so.
    • code should be as readable as it needs to be and no more so.
    • code should be as reliable as it needs to and no more so.
    • code should fulfil its technical design brief.
    In my opinion all coding standards come from these. The question I would be asking is not what do other people do, because other people often know no better than you (or I) I would ask what core principle does 'not using shift' fulfil. Does it make code faster, simpler, more reliable? However as all decisions are a trade off I would ask myself do I need whatever advantage and can I accept the disadvantages that it would also bring.

    On balance looking at the list I have given above to shift or not to shift. It dont matter one wit, tiss simply a matter of style, style being something that too frequenly is mixed up with a coding standard. Where do those brackets go again?


      First of all my experience is that most of your "and no more so" statements address areas that are already shortchanged. People cut too many corners as it is, they don't (IME) need more encouragement to do so. Trying to work cleanly usually works out faster in the end.

      As for shifting, yes, most of the time it doesn't make a big difference. If you have fairly meaningless rules (eg indent 4, not 2, using hanging braces, uncuddle your elses) to provide a common look and feel, that is perfectly fine. Look and feel issues affect ease of moving around your code base and consistency reduces bug rates. If cat2014 didn't use shift because of a style rule that she understood to just be a style rule, I would not be bothered. But she had trouble accepting a shift to be OK because she had it drilled in that it was a bad thing to do.

      Now that is fine if the argument against - that you are losing track of what is at what index - really is important. But it is bogus. If you are thinking of Perl arrays as something to access by index, most of the time you are doing something wrong. They are things you access in order. Besides which if you write OO methods then the order of your argument list is already off by one. Repeating bogus arguments for meaningless details is bad.

      Secondly while style is fairly arbitrary, it is good to not have rules which are not widely shared. The best references that any programmer at that company is likely to encounter all shift. That style is used in the top Perl books. It is used by good CPAN modules. It is used in core Perl modules. It is used by competent posters, in articles, etc. It will be used by most competent consultants, trainers, and hires. Do you really want to make reading all of that good advice a jarring experience? If not then your style should conform to the community.

      Thirdly I ran across the argument for not shifting and believed it for a while. I changed my mind. Why you ask? Well I found that very frequently I have a few special arguments that I want to process one way and an arbitrary list I want to do something else with. For instance a wrapper method will grab the object and then for each parameter it will do something. Now yes, you can copy @_ into a few variables and a new temporary array, but why? First of all that copy goes by value not reference so it is more work for Perl. Secondly there are now more mechanics for the programmer to keep track of. Is, or isn't this new array important by virtue of its existence? This happened to me regularly enough that I found it cleaner to always be consistent and grab fixed arguments with shift.

      So if I came to a new job and they wanted me to change how I indent, how I use braces, the capitalization style of variables and functions, naming convention, etc I would not object. I can adjust fairly quickly and I know that for those things, within reason, consistency matters far more than exact details. But I would argue if I was told to not use shift to process arguments in Perl. I have tried it both ways and I my experience says that it does make a difference.

        You must remember my audience. I was not writting a set of coding standards but explaining my thinking when writting coding standards. I agree completely that the statements I gave were not useful except to expert users but hay that is who I am talking too. We were discussing the merits of a particular standard that requires understanding of the origins of those standards. I feel very strongly that all too often people forget why software is written, it is written to perform a specific task. If the code performs that task then it is a success, if that task requires that the code be extensiable then cool as. The code must be written to be extensiable. If the code is for a one shot task then it does not. That is where the 'and no more so' come from.

        I feel that perl exemplifies this idea. It has the capiabilities to allow for write once code that can be quickly written. It also allows for long term maintanable code to be written. As a programmer it is my job to strike a balance between the two.


Re: Silly code reviews and shift
by footpad (Abbot) on Jan 18, 2001 at 20:09 UTC
    I've found code reviews to be either excellent learning opportunities or exercises in political motivation.

    As an example of the latter, I was once dinged in a performance review because I didn't "adhere to the organization's coding standards," as expressed in a third-party book authored by the company's owners. (Clearly, this wasn't Perl.)

    The main point under contention involved consistent use of capitalization, e.g. source formatting. The language in question used keywords to delineate blocks. In the example given, it was an IF/ENDIF block. I got dinged 10% (out of 100) because I typed "if...EndIf", instead of "if...endIf". (No, it wasn't any form of BASIC.)

    I lost another 10% because I didn't properly lock all the files being referenced. (The reviewer conveniently ignored the fact that I'd been after him to clarify his views on proper locking techniques for the previous nine months.)

    Of course, the whole exercise was really a way for the owner to justify a less than reasonable salary increase. This became even more apparent when the partner showed the reviewer several examples from "the book" where similar typos were made. It didn't have any effect on the review or my increase. I shortly left the organization.

    My point being that you should take code reviews in the spirit that they are given. If code reviews (and standards compliance) are part of your performance review, then you'd best make certain you do what you can to follow them. On the other hand, if they're meant as the educational opportunities that they were originally intended to be *and* performed by people you respect, then you'd best take any comments to heart.

    If you're part of a code review process, then it might be wise to think twice before flagging code as questionable. After all, there are (often) multiple ways to write an algorithm and that a certain amount of latitude needs to be made for personal style, knowledge, and coding practices. Mind you, I'm not referring to the most major blunders, such as not tainting, ignoring strict, or avoiding warnings. I'm referring to the little stuff (e.g. capitalization flaws, indenting three spaces instead of two, placing the opening brace of a block on the next line instead of the line initiating the block, and so on.)

    Finally, if you're a manager who uses code reviews as part of your performance review process, I would strongly suggest you:

    • Weight code reviews very lightly--especially if the mistakes being flagged are trivial or not documented. If you have clearly documented standards and the flagged bits are major violations, well, then you might have an argument...if you can demonstrate a pattern of disregard.

      However, I don't believe it should be enough to rate the employee lower. The key is whether or not the employee is willing to work toward better adherence, not whether or not they do things the same way as the person who drafted your standards. (And, if you don't have documented standards, you shouldn't be using compliance as a benchmark for employee performance.)

    • Discuss your findings with the employee in question *before* finalizing your evaluation. Surprises can only lead to bitterness and turnover.

    • Ensure that enough documentation and standard libraries exists to properly illustrate the organization's coding practices. (If you do not have standards, then it may be time to discuss the topic and develop some.)

    • Ensure that the standards are similar to the best practices of the community; that is, standards that the experts would be comfortable with. Of course, this means you need to take the time to learn who those experts are. (Far too many people think MSA is a good resource; far too few review the code they download from there and other common places.)

    • Focus on the code, not the personality.

    In the end, individual programmers needs to find ways to balance personal style against the organization's needs for reliability, consistency, readability, and maintainability. On one hand, it's not worth trashing your opportunities inside an organization due to a stylistic difference of opinion. On the other hand, if you're constantly frustrated because you're trying to follow standards that strike you as superior to the ones in your organziation, then it may be time to either a) try to improve the local standards or b) consider different opportunities.

    Life's too short to be continually frustrated.


(Ovid -- Programming style - Off topic) Re: Silly code reviews and shift
by Ovid (Cardinal) on Jan 18, 2001 at 22:08 UTC
    tilly, when we started discussing this earlier, I was going through some of my posts on this site to realize something very interesting. I am a huge fan of structured design. I typically use Warnier-Orr to design my programs. If I'm rushed or given poor specs, I tend to forget about that and put out code that can be read from top to bottom with relatively few functions. This is not good, but I noticed that it seems prevalent in many of my posts here.

    By contrast, one program I have has the first few lines of code being the following (I stripped the comments):

    #!C:/perl/bin/perl.exe -w use strict; use URI::Escape; use HTML::Entities; use CGI::Pretty qw/:no_debug/; $CGI::Pretty::INDENT = "\t"; my $q = CGI->new; my $table_data = create_table_data(); my $source_code = read_source_code(); create_web_page( headers => [ 'Symbol', 'ASCII Value', 'URL encoded', +'HTML code' ], data => $table_data, code => $source_code ); exit;
    That's pretty much the entire logic of the program. Yeah, you can go down into my functions and into their functions, and so on and so on, but you already have a decent idea of what I'm doing. If you view the output of the program, the above code is crystal clear. It's just a matter of filling in the details. I wonder if other monks have a tendancy to post code that answers a question but demonstrates poor programming style.

    I think, in future posts, I'll try to focus better on code quality in addition to accuracy.


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

      I'm sure that I occupy a substantially lower station than Ovid does in this community, because I feel that I will have a tendency to post more questions than answers in the near future. To that end, I would point out that I crafted the code that I submitted in Why is this link parser only working the first time? as carefully as I could. I did so because I did not want to embarass myself, nor waste the other members' time. As you can see if you follow the thread, I was rewarded with two helpful answers.

      tilly's response, which concisely answered my question, did not need to be written in a clean programming style. In fact, I suggest that a more lengthy, more stylistic response would have been harder for a subsequent Seeker of Wisdom to follow. And the result was that I felt a responsibility to summarize the situation after I followed the advice provided.

      Perhaps it is more important, from a community learning perspective, that at least one side of the code-related question be written in good form.

      Dave Aiello
      Chatham Township Data Corporation

Re: Silly code reviews and shift
by Adam (Vicar) on Jan 23, 2001 at 23:09 UTC
    Ah! I've been mis-represented!

    I have long used shift to manipulate named arguments, but not unnamed. Sometimes I like to use @_ unnamed, and sometimes I like to name everything in one fell swoop:

    sub QuickAndDirty { return SubSubroutine( shift, 3, shift, 'etc' ) } sub MultipleChoice { my $arg = @_ ? shift : die "Where is arg?"; my $opt = @_ ? shift : 'default'; } sub SetParams { die "Expected 3 args" unless 3 == @_; my ( $arg1, $arg2, $arg3 ) = @_; }
    Note that it is also important to realize that naming your arguments, as in the later examples, is equivalent to pass by copy. But Perl is actually a pass by reference language! An example:
    DB<7> sub s1 { $_[0] = 1 } DB<8> $g = 5 DB<9> x $g 0 5 DB<10> x s1( $g ) 0 1 DB<11> x $g 0 1
    So, Tilly, please don't judge me by 3 posts, none of which includes a sub routine with named arguments.
Re: Silly code reviews and shift
by Anonymous Monk on Feb 05, 2002 at 03:32 UTC
    I too very often use shift. I always shift of the first argument in methods. I'm sure you understand why, but I'll say why anyway. It's because I want them to be more like regular subroutine calls, where I can do nice checks on scalar(@_). And, having
    sub foo { my $self = shift; ... }
    certainly makes a good visual aid for me. I often even prefer to do
    my $self = shift; my ($this, $that) = @_;
    over     my ($self, $this, $that) = @_; -Anomo