Beefy Boxes and Bandwidth Generously Provided by pair Networks
Keep It Simple, Stupid
 
PerlMonks  

comment on

( [id://3333]=superdoc: print w/replies, xml ) Need Help??
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.

--f


In reply to Re: Silly code reviews and shift by footpad
in thread Silly code reviews and shift by tilly

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post; it's "PerlMonks-approved HTML":



  • Are you posting in the right place? Check out Where do I post X? to know for sure.
  • Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
    <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
  • Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
  • Want more info? How to link or How to display code and escape characters are good places to start.
Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others having an uproarious good time at the Monastery: (7)
As of 2024-03-28 11:22 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found