Software Best Practices

I have encountered the same conversation during code reviews a number of times and suspect I may not be alone in this. The abridged version of the conversation starts something like:

Colin:  "You've done some good work implementing a complicated piece of code, well done. The XYZ function though is particularly complex and I think it could be simplified by doing <particular improvement as appropriate>."

Maurine: "I don't see a problem with function XYZ, and I don't see the need for <particular improvement as appropriate>. The code works fine and anyway it was based on some other existing code."

.... The conversation can now continue in a number of directions all of which are looking to work around the same fundamental issue, Colin and Maureen have different opinions or expectations of the code quality. The complexity of a piece of code is a somewhat subjective matter and good code design is driven by heuristics not rules. How then does one find an objective way to resolve this issue in a code review without simply saying 'Do it my way'.

I would be very greatful to know that I am not alone in encountering this scenario and particularly greatful to hear how others handle this situation.

Cheers 

 I have a couple of reactions to this.

1. If implementation choices are being discussed during the meeting, then the meeting is not a code review. It might be a design discussion. Code reviews are intended to highlight potential defects. It is the author's or maintainer's responsiblity to actually implement the necessary fixes. So, one approach would be for the meeting moderator to remind the participants to stick to the meeting's agenda.

2. Maybe you're not actually talking about code review meetings, but a more informal discussion between peers. In that case, I as the author would first express my thanks to the person providing feedback. Then I'd make sure I understood the feedback by asking clarifying questions. Then I'd take some time in private to sincerely consider the suggestion. Then I'd make a decision whether or not to make any changes. Then I'd move on.

In an agile environment, these kinds of arguments are fewer and are less emotionally intense. The reason is that code isn't owned by individuals. It is owned collectively. Anyone is free to refactor pieces of code as long as its functionality isn't changed (something verified by unit tests). If someone feels strongly enough about the code to take time out to refactor it, then that is allowed. Also, with collective ownership, the conversation shifts from a he versus she turf battle to a higher level conversation among the team members about where the risky code is and what should be done about it. Have a relatively large number of defects been reported against this code? Have other team members had difficulty maintaining this code? And so on.

 Hi Eric,

 Thank you for your response. Theres a few things in there I find quite useful, particularly with regards to your experience of agile development. The model in my team has individuals working largely independently on code which is then reviewed prior to testing. This contrasts with agile team development which in comparison helps avoid too much individiual ownership. Also code reviews often end up being done by one other person and so comments are that of an individual rather than of the team. I know that in some respects this shouldn't make a difference but, people are people!

I can see that Team coding can improve this situation but what is your experience of it working in practice? If there is a junior engineer on your team how do you ensure that his code isn't continually being refactored leaving him feeling demotivated. Also do other team members actually refactor other peoples code where this is appropriate or do they avoid doing this for fear of causing offence? Hmmm ... perhaps there is a cultural element here, I am English you see, and my Englishness may inhibit me from refactoring a colleagues code for fear of causing such offense! 

One part of your response which surprised me was not discussing implementation issues during code reviews. While there are different views concerning the level of design completed prior to implementation I think it is inevitable that during implementation the previously reviewed design will be modified or further areas of complexity will be discovered. In addition known areas of complexity will also be implemented poorly. We find this is often spotted during formal inspections. I guess this happens less often if you are using an agile process but does it not happen at all?

In my original email I said code quality was objective, I of course meant to say it was subjective. In your response you suggest team members identify areas of risky code which have a large number of defects and which other members had difficulty with. At the point at which you have a formal inspection is this information available?

Cheers

  Hello! Good questions.

 Let's start with the "not discussing implementation issues during code reviews". When I envision a code review, I envision a process where copies of code have been handed out to participants, they each individually review the code and record comments, and then a meeting is held where all of the candidate defects are collected with clarifying comments and given to the author/maintainer. It has been found to be a best practice to avoid discussing solutions to the defects during the meeting. It is the job of the author to understand what the criticism is, and then to decide what to do about it. If the author needs help, he/she can of course engage a peer for that help, but it doesn't happen during the meeting. The reasons have to do with efficiency and psychology. The meetings can drag on forever with a couple of people dominating the discussion, and the author can begin to feel personally attacked.

 Now, it sounds like that isn't what you're talking about. It sounds like you are simply asking for a peer to informally review your work before it gets delivered to test. So, the code review "rules" don't really apply. It all depends on what your process is, what the intent of these peer desk checks are, and what kinds of pressures your project is experiencing. If you think you're "done" with the code, and the peer desk check is a process gate to ensure no "obvious" defects sneak through that will frustrate the testing, then I'd frown on any implementation suggestions. If the code "works", then what would be the justification for re-implementing it a different way? If it's broken, then your peer ought to be able to explain what's broken, and you ought to be able to figure out how to fix it.

Now, I'm not saying that people shouldn't work collectively and discuss implementation options. It's just that I feel this is better done BEFORE or at least WHILE the code is written. If you're doing it after the code is written, then you're wasting a lot of time re-implementing stuff (or arguing about re-implementing stuff). So, by all means, try pair programming. Try pre-implementation engineering discussions. Try design discusssions and design reviews.

 Now let's discuss team ownership of code. I can understand your reticence to changing other people's code, because you're working in an environment of individual code ownership. In that environment, I wouldn't suggest that you go changing other people's code. But, I'll share my experience with you. First, there should be an environment of mutual respect. We all understand that we're working as a team, we're not competing to be the "best programmer". If you change code that I originally wrote, I understand that either requirements have changed or that I simply made some sort of error. I appreciate you "having my back", so to speak. We all make mistakes, and the sooner we gain the humility to accept that, the sooner we can move on to being a productive team. I should see this as an opportunity to learn, rather than a personal attack on my abilities.

But what is more difficult to explain, is that so much collaboration is going on that I don't feel like code that I wrote alone was really mine anyway. I've already discussed some design choices with other teammates. I've already immersed myself in existing code, and so I understand the architecture, coding standards, etc. And if this code is in a completely new module or seems particularly risky, I'll ask a teammate to pair program with me for awhile.

For a junior engineer, probably all of his programming should be done in a pair programming format for awhile. And his first "solo" assignments should actually be refactorings or enhancements of existing code. Not only is this good education, but it will make him feel less of a "target" for code changes.

My experience was that people didn't take offence. For one thing, I wouldn't rush to change code that I wasn't comfortable with. As projects develop, individuals tend to specialize as experts in certain areas of the code base. I wouldn't go changing web service code willy nilly if my baliwick was javascript. But, there will be times when I do need to change web service code. The web service "expert" is on vacation when we discover a defect. Or just simple load balancing--I'm the only one with any spare cycles at the moment. At those times, I just proceed with caution and try to get input from my teammates. I'll also take extra care to test the code. And I'll have it reviewed.

All of which highlights some benefits of collective code ownership. Each person tends to develop familiarity (not always expertise) in a large portion of the code base. This means that the team won't by stymied when one person leaves the company, or goes on vacation, or catches the flu. It means that tasks can be allocated effectively--one person doesn't become a bottleneck. It means that errors or oversights are detected quickly rather than festering in the code for months undiscovered. It means that designs improve as more smart people have a chance to work with the code. It means that integration is smoother, because modules don't evolve in isolation.

There is a lot more to say, but I hope this helps for now

Hey Eric,

Great stuff.

These things do calm down in a very high speed envronment. Strong programmers will tend to have the competitve streak, like any profession. This in my opinion is where a knowledgable manager makes a decision on which dev person has the best judgment, and goes with that opinion.

Over time a manager can learn to keep the train roaring along without undue strife. It is a skill like any skill, and the person running the show must learn it.

At the end of the day there are business drivers for these builds, and let's keep our eye on the ball.

All,

What the heck, I'll toss out my two cents...

One root issue here (IMHO) has to with "responsibility" and "authority". Companies are more than happy to make employees RESPONSIBLE for getting things done but are notoriously bad at giving them the AUTHORITY to make it happen. I think Howard's posting (the one that starts this thread) is just one example of this general responsibility/authority mismatch dynamic.

Clearly, the developer in question was given the responsibility of writing the code. OTOH, the mechanics of the review process described, usually ends up taking authority over the code away from the person with the responsibility. At one former employer (to remain nameless) the saying was "Everybody can say No, nobody can say Yes". Everyone has the ability to shoot down your idea but nobody has the authority to say, "yup, this is the way it's going to be done".

In order to function effectively, a person needs to be given an appropriate level of authority that would allow them to be successful in fulfilling their given responsibilities. In the code review case, here's how I'd handle it:

Simply, unless a reviewer of the code can prove that the code as written is wrong, then the code stands as is. The developer who wrote the code has to be the final authority on what the code looks like. Provided that the code actually meets the requirements, and provided the code complies with whatever design and coding standards are applicable then reviewers may make *suggestions* for improvement but the developer is not bound to do anything about those suggestions. If you didn't trust the developer to do a competent, professional job then I argue you shouldn't have given them the responsibility to write the code in the first place. The code review shouldn't be viewed as a mechanism to take authority away from the original developer even though as implemented that's what they usually do.

The purpose of the code review is to help the developer see problems in the code that they weren't able to spot all by themselves (we could get into a discussion of "cognitive dissonance" here). And, in pointing out those problems, we need to leave it up to that developer to fix those problems in they way they see best.

As I said, my two cents...

-- steve

Worth more than two cents if you ask me..

 Great stuff!