38

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

What is an appropriate response to this attitude?

Improve this question
16

19 Answers 19

Reset to default
25

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
Improve this answer
69

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?

Improve this answer
4
  • You are basically saying that code review only works with nice, always reasonable and rational characters. The real world looks different...
    – Philip
    Jul 18, 2012 at 12:29
  • 3
    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.
    – joshin4colours
    Jul 18, 2012 at 13:27
  • 3
    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.
    – DJClayworth
    Jul 18, 2012 at 13:35
  • 31
    +∞ for "If it's too late to change anything anyways then why are you doing a code review?"
    – mskfisher
    Jul 18, 2012 at 13:37
18

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

Improve this answer
2
  • If it's really a fight to get rid of the extra code, then you might let him keep it in, IF AND ONLY IF he can produce a full unit test suite to make sure that it keeps working. Either way, he's got to actually FINISH the job.
    – Michael Kohne
    Jul 18, 2012 at 15:19
  • +1 for the simple fact the code has ( obvious ) bugs and thus has not been tested.
    – Ramhound
    Jul 18, 2012 at 18:38
8

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

Improve this answer
7

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.

Improve this answer
5

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.

Improve this answer
2
  • I would say that 10 times in a single day is a little much. If you really wanted to push it, 3 or 4 checkins would be alright, this means a checkin on average every 2 hours giving a typical 8 hour day. But 10 checkins seems like there is no time to actually review anything, report back, or implement changes based on the review itself.
    – Ramhound
    Jul 18, 2012 at 18:41
  • @Ramhound Yes, 10 checkins is an extreme case, 3 to 4 times is much more typical. And it needs some time to get used to it...
    – stefan.s
    Jul 18, 2012 at 19:22
2

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)

Improve this answer
2

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.

Improve this answer
2

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.

Improve this answer
0

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.

Improve this answer
0

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.
Improve this answer
0

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.

Improve this answer
0

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.

Improve this answer
0

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.

Improve this answer
2
  • The code has obvious bugs. The fact the author also thinks the solution itself is incorrect highlights the fact, there are bugs. If you have bugs in your code, and I am not talking about non-obvious bugs you can't catch without full regression test, there is a problem with said code.
    – Ramhound
    Jul 18, 2012 at 18:45
  • @Ramhound:If there are bugs then point out the specific bugs. If fixing bugs is not part of the review process then what is the point of holding the review? As I said, overly complex is not a bug. It is certainly a short-coming but if the only person who believes it is overly complex is the OP and nobody else does then oh well. Work hard, become the lead and decree quality to your standards at that time. I can sympathize with the OP, I went through the same issues, now that I have the authority to direct people to make changes I want, I find that other things end up being higher priorities.
    – Dunk
    Jul 23, 2012 at 18:18
0

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?

Improve this answer
0

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.

Improve this answer
0

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.

Improve this answer
-1

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

Improve this answer
-2

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

Improve this answer
7
  • I'm glad my boss doesn't operate that way.
    – user25946
    Jul 18, 2012 at 15:25
  • @Jon: When people respond unprofessionally as in "well it's already done", like my six year old would say, then you have to treat them like children.
    – cneeds
    Jul 18, 2012 at 15:31
  • 2
    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.
    – user25946
    Jul 18, 2012 at 15:50
  • I'm not advocating treating professionals like cildren. In the example given we have a stubborn, petulent somebody who writes buggy code with uncalled for functionality and returns childish answers to legitimate questions. Dan is asking for the best way to deal with this. Not the most popular way.
    – cneeds
    Jul 18, 2012 at 16:27
  • Fortunately I don't have "children" in my team so it is unnecessary to treat them as anything but the professionals that they are. They do not add unasked for functionality (wasting my time and money), they write pretty solid code and, when they are requested to revisit or revise something, they do so, and they learn from the experience.
    – cneeds
    Jul 18, 2012 at 16:28

Not the answer you're looking for? Browse other questions tagged or ask your own question.