The other day I reviewed code someone on my team wrote. The solution wasn't fully functional and the design was way over complicated-- meaning stored unnecessary information, built unnecessary features, and basically the code had lots of unnecessary complexity like gold plating and it tried to solve problems that do not exist.

In this situation I ask "why was it done this way?"

The answer is the other person felt like doing it that way.

Then I ask if any of these features were part of the project spec, or if they have any use to the end user, or if any of the extra data would be presented to the end user.

The answer is no.

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

My view is that it is not done, it's buggy, it doesn't do what the users want, and maintenance cost will be higher than if it were done in the simpler way I suggested.

An equivalent scenario is:
Colleague spends 8 hours refactoring code by hand which could have been automatically done in Resharper in 10 seconds. Naturally I don't trust the refactoring by hand as it is of dubious quality and not fully tested.
Again the response I get is "well it's already done."

Questions on Programmers Stack Exchange are expected to relate to software development within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.
If this question can be reworded to fit the rules in the help center, please edit the question.

Sometimes you have to do things you don't like, such as telling someone who put a fully day's work into writing complex code that "it's no good, roll it back and start over" or something along those lines. It sucks but you'll be thankful you did.
–
joshin4coloursJul 18 '12 at 13:27

2

A simple answer that sums up the situation exactly. Your other response to "It's already done" is to explain that an over-complicated solution will cost lost of time in maintenence, and reworking it will save time in the long run.
–
DJClayworthJul 18 '12 at 13:35

20

+∞ for "If it's too late to change anything anyways then why are you doing a code review?"
–
mskfisherJul 18 '12 at 13:37

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.)

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.

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.

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.

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.

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".

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.

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.

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.

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.

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.

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.

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.

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.

Can't say I agree. What results do you expect to get from your people, if you "treat them like children"? There are other approaches which, IMHO, are more constructive.
–
Jon of All TradesJul 18 '12 at 15:50