Mentality/attitude

• Lead by example
• Admonish in private (one-to-one, outside the code review)
• Encourage a Keep-it-simple mentality among team members

Team management

• Spend more time on the specification of a work item (such as architecture, algorithm outline, UI wireframe, etc)
• Encourage team members to seek clarification about the scope of a work item
• Encourage team members to discuss ways of implementing a work item
• Make reasonable estimates for each work item before starting, and make best effort to meet them
• Monitor the “improvement” of team members.
  • After being admonished or being shown the right way to do things, see if the team member improve.

Skill level

• Allocate some time for pair-programming sessions or one-to-one training sessions for making the best use of developer tools (refactoring, code-review)

Project (risk) management

• Conduct code-review more often, asynchronously (Note)
  • Note about “asynchronously”
    • The code reviewer should get notifications / invitations to review changes as soon as being committed
    • The code reviewer should have a chance to review the code before any meeting with the developer.
    • If clarification from the developer is needed, do it informally on IM/email without casting a negative opinion

What do you say in a code review when the other person built an over complicated solution?

You say: “you built an overly complicated solution.”

So then I suggest that he delete all the unnecessary complexity. The answer I usually get is “well it’s already done.”

If it’s too late to change anything, why are you doing a code review?

“It is already done” is not a satisfying answer. Done means tested and working. Every extra code that is not doing anything useful should be maintained the proper way (deleted).

Assign him this task again asking to refactor and optimize his solution. If he doesn’t do that, assign him a pair programmer and hope he’ll learn something from the colleague.

So then I suggest that he delete all the unnecessary complexity. The answer I usually get is “well it’s already done”.

That is not an acceptable answer:

• If it is really too late to change, then the code review wss largely a waste of time, and management needs to know this.

• If that’s really a way of saying “I don’t want to change”, then you need to take the position that the extra complexity is BAD for the codebase BECAUSE of the problems / cost it is going to incur later. And reducing the potential for future problems the real reason you are doing the code review in the first place.

And …

… the solution wasn’t fully functional …

That is quite possibly a direct result of the unnecessary complexity. The programmer has made it so complex that he no longer fully understands it and / or he has wasted his time implementing his complexity rather than the function points. It would be worth pointing out to the programmer that cutting out the complexity may actually get him to a working program faster.

Now, it sounds like you don’t have the power (or maybe the confidence) to “push back hard” on this. But even so, it is worth making a bit of noise about this (without personalizing it) in the hope that the offending coder will do a better job … next time.

What is an appropriate response to this attitude?

Ultimately, bring it to management’s attention … unless you have the power to fix it yourself. (Of course, this won’t make you popular.)

You were right, they were wrong :

• broken YAGNI principle
• broken KISS principle
• is code fully tested? If no, then it is not done

What is an appropriate response to this attitude?

Do the proper code review. If they refuse to implement suggested changes without a reason, then stop wasting your time one code reviews. You can also escalate the problem to their boss.

One action which our team took, which dramatically improved the situation in such cases, was the move to much smaller changesets.

Instead of working on one task for a day or more and then having a (large) code review, we try to checkin much more often (up to 10 times a day). Of course this also has some drawbacks, e.g. the reviewer needs be very responsive, which decreases its own output (due to frequent interruptions).

The advantage is, that problems are detected and can be solved early, before a large amount of work in the wrong way is done.

You should focus on the root cause of the problem:

1. Education of programmers focuses on increasing complexity given to programmers. Ability to do this was tested by the school. Thus many programmers will think that if they implement simple solution, they did not do their job correctly.
2. If the programmer follows the same pattern he has done hundreds of times while at the university, it’s just how programmers are thinking — more complexity is more challenging and thus better.
3. So to fix this you’ll need to keep strict separation of what your company requirements are relative to complexity compared to what is normally required in programmer’s education. Good plan is a rule like “highest complexity level should be reserved only for tasks designed to improve your skills – and it shouldn’t be used in production code”.
4. It will become a surprise to many programmers that they are not allowed to do their craziest designs in the important production code environment. Just reserve time for the programmers to do experimental designs, and then keep all the complexity in that side of the fence.

(in code review it’s already too late to change it)

I don’t know anything that works after the code is written.

Before the code is written, people can discuss alternative ways to do it.
The key is contributing ideas to each other, so hopefully a reasonable one will be chosen.

There’s another approach that works with contractors – fixed-price contracts.
The simpler the solution, the more $$ the programmer gets to keep.

You can’t fix the world.

You can’t even fix all the code on your project. You probably can’t fix the development practices on your project, at least not this month.

Sadly, what you are experiencing in code review is all too common. I have worked at a couple of organizations where I found often found myself reviewing 100 lines of code that could have been written in ten, and I got the same answer you did: “It’s already written and tested” or “We’re looking for bugs, not a redesign.”

It’s a fact that some of your colleagues can’t program as well as you can. Some of them may be pretty bad at it. Don’t worry about that. A couple of classes with bad inplementations won’t bring down the project. Instead, focus on the parts of their work that will affect others. Are the unit tests adequate (if you have them)? Is the interface usable? Is it documented?

If the interface to the bad code is ok, don’t worry about it until you have to maintain it, then rewrite it. If any one complains, just call it refactoring. If they still complain, look for a position in a more sophisticated organization.

There should be a standard policy in the project that controls deliverable quality checking procedures and tools used.

People should know what they should do and what tools are accepted for use in this project.

If you have not done this yet, organize your thoughts and do it.

Code review should have a check list of standard items. If you get “it is already done” and it is not, then personally, I would not want to be responsible for this developer’s work as project manager or senior developer. This attitude must not be tolerated. I can understand arguing about how to do something or even every thing, but once a solution is accepted, lying should not be tolerated and that should be clearly stated.

Your shop needs to enforce some design methodologies.

• Requirements need to be defined clearly.
• You need to develop use cases which support the requirments.
• You need to specify functions needed to implement the use cases.
• You need to specify any non functional requirements (response times, availability etc.)
• You need an RTM (Requiements Tracabilty Matrix) to map each system function back to a use case and a real requirement.
• Drop any function that is not supporting an actual requirement.
• Finally in your code review flag any code that does not directly implement or support the defined functions.

Probably not that it’s overly complicated because that makes most people feel bad afterwards. I assume that when this happens already a lot of code has been written without talking one word about it. (Why is that so? Because that person has enough authority so his code doesn’t have to be reviewed in reality?)

Otherwise I guess to make code review less formal but more frequent. And before writing large modules maybe you should quickly discuss what approach to take.

Saying “this is too complicated” gets you nowhere.

It’s unfortunate, but Code Reviews are, a lot of times, more for the future than for the present. Especially in an enterprise/corporate environment, shipped code is always more valuable than unshipped code.

This, of course, depends on when the code review is completed. If it’s part of the development process, then you could gain some benefit right now. But if the CR is treated as more of a post-mortem, then you’re best off pointing out what could be done better in the future. In your case (as others have said), point out YAGNI and KISS in general, and perhaps some of the specific areas where these principles could be applied.

What does overly complicated mean? You make an ambiguous statement then you will get an ambiguous/unsatisfying answer in response. What is overly complicated to one person is perfect to another.

The purpose of a review is to point out specific problems and errors, not to say you don’t like it, which is what the statement “overly-complex” implies.

If you see a problem (overly-complicated) then say something more concrete like:

• Wouldn’t changing part X to Y simplify the code or make it easier to understand?
• I don’t understand what you are doing here in part X, I think what you were trying to do is this. Present a cleaner way of doing it.
• How did you test this? Did you test this? If it is overly-complicated this will usually lead to blank stares. Asking for tests will then frequently get the person to self-simplify their code when they couldn’t figure out how to test the original code.
• There seems to be an error here, changing the code to this would fix the problem.

Anybody can point out problems, especially ambiguous ones. There’s a much smaller subset that can present solutions. Your review comments should be as specific as can be. Saying something is overly complex doesn’t mean much, it may even cause others to think YOU are incompetent for not being able to understand the code. Keep in mind that most developers don’t have a clue of the difference between a good or bad design.

Sometimes it is worth it as a group to focus on some of the “Agile” principles–they can help a group or individual who seems to be a little off course.

Focusing doesn’t have to mean some great big re-working of your team, but you should all sit down and discuss what practices are most import to you as a team. I’d suggest discussing at least these (and probably quite a few more):

• Do the simplest thing that could possibly work?
• You ain’t gonna need it (are you solving problems that weren’t in the spec)
• Write the test before coding (Helps you focus your code)
• Don’t repeat yourself

Also occasional (Weekly?) reviews of what works, what doesn’t and what’s still needed can be really helpful… If nothing else, why not commit to an hour a week to discuss team values and practices?

Escalation, if you have a technically minded manager. This sounds like a habit that needs to be broken.

If the code is not built to spec, then by definition it should fail code review. I don’t understand the concept of “well we’ve done something that no one asked for, and it’s not working so we’ll leave it there instead of doing something someone asked for that works”.

This is a bad habit for any developer to get into. If he/she was working to a design spec then not matching it without good reason is a no no.

One word: agile

It certainly doesn’t solve everything. But by reigning in your iterations (1-2 weeks, for example), limiting work in progress and leveraging sprint planning/review, you should avoid these waterfall-like mistakes. You need better visibility into what’s actually being done – while it’s being done.

For normal project-based development, I’d recommend adopting a Scrum approach. For continuous development/integration environments, and especially if you have many developers working on the same or related projects, consider incorporating elements of Kanban. Another effective approach is to leverage pair programming, a defined practice of Extreme programming.

Your situation is hardly unique. And even with small teams, process can go a long way to helping avoid the situation you’re in now. With proper visibility and a reasonably groomed backlog, questions like these become sprint planning decisions — saving you from managing technical debt.

What I have said in the past is “this code is complex and I am not confident in what it is trying to do, is it possible to either simplify it or write it more clearly?”

You to coder after deleting/rolling back their code: “Oops, your code has gone. Please rewrite it. As you have already written it once you will need less than twenty minutes to provide ONLY the code required by the spec.

“My next review is in 20 minutes.

“Good day.”

Do NOT accept any arguments!

Done, IMHO

Chris