Colleague wrote bad source code, what do do?

Question

The situation is the following.

I have been asked to review the source code written by a colleague.

He is a novice in this particular field.

This colleague has been working with a client for more than a month, basically solo on the platform he is responsible for.

He does not have enough experience and the code is not very good:

• not very maintainable,
• little testing,
• questionable choices,
• far from best practices...

it does work though...

In my opinion it was a mistake to send him solo to a client.

• What should I do now?

• What should I tell to management?

• What to do with all the mediocre job done so far?

• What do do from now on, for all the code that will be written in the future?

Answer 1

If you've been asked to review his code, then you should review his code. Code review is part of the process of being a professional engineer, and your colleague should get used to it. Point out all the mistakes, even if there are a lot of them and it feels like you're being harsh, but remember to always be constructive. If there's a mistake made, or a bad practice, or something else, you should call it out, but you should be mindful that maybe he doesn't understand the code review process yet, so you don't want to come off as harsh or aggressive, but helpful. But in the end, the bottom line is, if his code is bad, you should tell him it's bad, and you should expect him to fix it.

As for what to tell management, nothing. He is a novice in the field. Management knows he is a novice (presumably they read his resume). The best that can happen (for you) is you report this guy, he is reprimanded, and he ends up getting fired for nothing more than the "crime" of being a novice. That's not how you train novice engineers, that's how you get them to give up and move to other disciplines. First, you should try to teach him and build him up. Eventually he won't need any more hand-holding, or if he continues to need hand-holding then you should report him to management at that time. But give him some time to learn and grow, and you should help him do that. If you absolutely want to report something to management, what I would say is something like this:

I have reviewed Joe's code, and it looks like there are a lot of issues with it. Being that Joe is a new engineer, these mistakes are understandable, but unfortunately it presents a bad image of the business for Joe to be working solo with this client. I would like to take responsibility to work with Joe on this project to try to train and teach him best practices so he can be more productive in the future.

That's the best thing you can say to management, imo.

Answer 2

With coding (as with basically every profession!) it's pretty normal to produce to very ropey work when a novice - especially when first "flying solo" and pretty much everything you list can be explained by inexperience.

What should I do now?

If feasible I would suggest having a session with your colleague where you can take him through the ways the code could be improved. Make sure to also stress any good things about it as well!

Don't

Say things like:

What you did wrong here...

&

Your code sucks!

&

This isn't best practice

Do

Say things like:

If this section was written like this it would be more maintainable

&

What was you reasoning behind doing this section like this?

&

While the approach you took here works, best practice is usually to do XY&Z because of AB&C

Basically keep the conversation focused on the code and positive steps that could be taken to improve it, rather than negative statements and references to him.

What should I tell to management?

Give them your honest assessment, if you think he needs more training before future solo work then say so. You won't be doing anyone any favors by fudging it. But make sure to stress where "issues" have arisen through simple inexperience, and if they "got it" during your review with them mention this too

XYZ was a common mistake by novices, when I talked it through with [colleague] they totally understood and know how they improve next time.

What to do with all the mediocre job done so far?

Ultimately it's a management call to decide what to do with it - you say the code works so at this point it's about cost/benefit for any refactoring. It would be a great learning experience to have the colleague work on a version 2.0 that incorporates your feedback but if the code is functional enough to "release" to the client this might be a tough sell in short term ROI - it's not unreasonable to take the position that the colleague can take the lessons-learned into the next paying project. While not as an effective learning strategy it can still be more cost-effective in the long run.

What do do from now on, for all the code that will be written in the future?

Again this is a management call - it might not be a bad idea to have intermediate reviews with either yourself or another experienced developer to spot and correct any issues before the project is complete.

Answer 3

The junior's colleague code is reviewed after one month. That is a very long time. So I'm wondering what the purpose of this review is. If the purpose is to fix all the faults in the code that was created during one month, that will be an awful lot of work and nobody will be happy with either of you. If the purpose is to teach how to do things better, that's a bit late, but better late than never.

Assuming that nobody wants to spend weeks on fixing everything, I would look at the code, find strategic and tactical mistakes that were made, and then spend a good amount of time with the colleague to discuss these things.

Strategic mistakes are things that you can (just about) get away with in a project that took a month to create, but that will kill you in a one year project. Tactical mistakes are things that are localised and can be fixed in a localised way.

For the future: The best time for a code review is before the code is written. Figure out what strategic plan there is (if any) and fix it if there are problems. And then there is the question: Do you want good code quality (review tasks that didn't take more than a day), or do you want your colleague to learn (every week, spend time reviewing, find problematic code, and discuss how to fix and avoid such code.

Answer 4

Every human have a threshold, the guy is a novice. I think the session to review the code must be fair and related to his level, underline the bad and good parts. In some bad cases, it is possible to guide him by asking questions like: how your code will react is this event happen or how flexible is your code if the client ask you to add this feature?

I would take note of the advance topics that were not mastered and I would introduce them gradually trough lunch and learn, pairing, later code review, session training, books, tutorials, etc.

If you are hard on him or criticize everything, probably the guy will start to ignore your feedback after a couple of months of code review and may end to hate his job if he feel depreciated.

Secondly, I would write down a list of what need to be addressed, I will prioritize that list then plan what the review can cover by the available time without exhausting the reviewer and the reviewed. A programmer repeats his errors when they do not understand a topic. So I would not feel the need to review everything in detail because he will probably repeat the same mistakes and you will have another chance to pick them up in the next code review. This will spread your recommendations trough multiple reviews and it will lightening the process. Also, there is probably uncovered topics that the guy will pick up by himself in the following months.

Finally, on the side, I would track the technical debt in the company planning tools.

Working solo with a client as a novice is usually a stressful task. The code works, that means he understood the requirement, the business domain and able to demonstrate leadership.