How to deal with 'almost good' code from a junior developer? [closed]

softwareengineering.stackexchange https://softwareengineering.stackexchange.com/questions/350268

Question

I got a question about team managing. Right now I'm dealing with a junior developer who's working remotely from a coding factory. The guy is open to criticism and willing to learn, but I got some doubts how much should I push some stuff.

Right now when something is straight and obvious a violation of good practices: like violation of SRP, God objects, non-meaningful names for methods or variables; I point out what he has to fix and try to explain why it is wrong.

My question is: when do I stop? Right now if there are some minor violations of the coding style like variable names in the wrong language (previous team mixed Spanish and English and I'm trying to fix that), or some minor structural issues I'm letting go and fix it if I have any spare time or happen to need to modify the problematic class. I feel this is good for team morale so I'm not pushing back code constantly on what to a novice might seem like minor details, which can be quite frustrating, but I'm also worrying that being too 'soft' might prevent the guy from learning how to do some stuff.

How do I balance the line between teaching the guy and not burning him out with constant criticism? For a junior it can be frustrating if you tell him to redo stuff that to his eyes is working.

Was it helpful?

Solution

If you think the code should be fixed before merging, make comments. Preferably with "why" so the dev can learn.

Keep in mind code is read far more often than written. So things which seem "minor" can actually be really important (variable names for example).

However, if you find yourself making comments which seem tedious, perhaps consider:

  • Should your CI process catch these?
  • Do you have a clear "developer guide" to reference (or is everything documented in your head)?
  • Do these comments actually contribute to code quality?

A lot of people sacrifice productivity at the altar of process or perfection. Be careful you don't do this.

Try to visit your colleague in person if possible. Or use video calls. Building a relationship makes criticism (even code reviews) easier to manage.

If you find that a piece of code has too many back/forth on issues, request the review over smaller pieces of code. Incremental changes are more likely to avoid some of the more significant design problems, because they are by definition smaller.

But absolutely do not merge stuff and then go back and fix it. This is passive aggressive and if the developer finds you doing this, you will kill their morale.

OTHER TIPS

Keep the criticism on the code rather than the writer.

Any work produced comes with inherent emotional attachment. Consider easing this by disassociating the code from the writer as much as possible. The code's quality should be consistently established as a mutual goal that you both face, together, rather than a point of friction between you.

One way to achieve this is to wisely choose your words. Although STEM individuals like to think themselves as highly logical, emotions are a part of human nature. The verbiage used can be the difference between hurt or spared feelings. Saying "This function name would be more consistent with the conventions if it was in English" is preferable to "You wrote this function name wrong and it should be in English." While the latter is still tame and alone would seem fine, in comparison to the former its faults become obvious -- when preparing what to say either in person or an email examine whether or not your context, words and focus are on the issues rather than the person.

Body Language

While words are important, most communication is non-verbal. Pay attention to body language, even seemingly insignificant subtleties like orientation mattter e.g. Many senior-junior interactions occur face-to-face, yet a side-by-side approach would be much more conducive to your desired outcome.

Give honest positive feedback

A multitude of studies show that information is learned more quickly and retained better when we reward good behavior rather than punishing bad ones. Sometimes just noting that the performance has been improved with a simple "good job" or a more specific "I noticed lately that your style has been matching our standards to a tee, great work!" even supplementing this recognition of improvement by having them, in turn, advise someone else on the issues they've amended can make all the difference to your junior dev and his work.

Based on your description, I would leave it at: "this is good. there are a few things I would have done differently but they aren't very important."

As you seem to grasp, criticism has a cost and if you spend a lot of time on niggling details, it becomes a morale issue. Ideally all the coding standards are checked automatically and you can't build unless you are following them. This is a huge time saver and lets you get down to business. If you reserve your critiques for 'things that matter', your advice will have much more impact and you will be seen as a valued mentor. It's really crucial to distinguish between things that are not good and things that aren't exactly way you would do it.

I believe in the concept of the teachable moment. If the developer has enough mental capacity, (s)he might ask for the details on what you would do differently. (S)he might not and that's OK. People get burned out and early in a career it can take a lot of mental energy to accomplish things that seem simple later.

I would consider accepting his work when it is acceptable, not perfect. And then the next task is after a discussion to refactor it immediately by making all the small but important changes you want.

So when is work is first accepted, your message is that it wasn't bad, and some places would have accepted it as good enough - but not places where you would want to be as a junior developer who wants to learn his trade properly.

So you don't say "I reject your work because it's not good enough". You say (a) "I accept your work because it's good enough", and then (b) "But I want it better".

The guy is open to criticism and willing to learn, but I got some doubts how much should I push some stuff.

Push everything you can. If the guy is learning, and it's your job to review his code, you both have much to gain if he does a good job.

That'll mean less code for you to review in the future, and possibly give your team a hiring target.

Also, if you hold back, you're not helping, but patronizing.

Just by the fact that you posted your question here, asking if you're doing too much, already signs me that you don't need this specific advice, but for others, here it comes: Just remember that pushing hard doesn't mean being a jerk.

Being a mentor isn't an easy task. You'll also have to give some space for him to make some mistakes and correct them on his own, just be sure that he'll do that somewhere that doesn't do real damage.

Pretty wide open question, but here are a couple ideas.

  1. Peer reviews (by the junior developer)

    The best way for someone to learn the "right" way is to see others do it. Do all of your developers perform code reviews? It might not be a bad idea to let your junior developer perform them too (although you should also require at least one review from a senior developer too). That way he will see good coders in action, plus he will observe that there are review comments directed at engineers other than himself, meaning that it aint personal.

  2. Early feedback/Task reviews

    Allow the developer to participate in his own task breakdown. Have him record his intended design in the task notes, and submit a "code review" with no changeset and just the task. That way you can review his plan before he has written a single line of code. Once his task has been reviewed he can start coding (after which he will submit another code review). This avoids the demoralizing situation where the developer has written a bunch of stuff and you have to tell him to rewrite it.

If the code is objectively violating a written, unambiguous standard, then I think you should just keep pushing back until every issue is fixed. Sure, it might be slightly annoying for the developer the first few commits, but they might as well learn the guidelines sooner rater than later.

Also, if you allow a few breaches of standards here and there then they will set a bad precedent - see broken windows theory. Also, it is a lot easier to remember to follow the standards if it is already consistently applied to the codebase. So really, everybody wins, including the junior developers in question.

I don't think morale is a big issue as long as the coding standard is written down. Only if it gets into more subjective "well, I would have done it differently"-territory, then you should be concerned, since the developers approach might be just as valid.

Consider adopting a code review workflow, where developers post their proposed commits to a tool like Github Pull Requests or Phabricator Diffusion and get peer sign-off before landing their changes onto the shared master branch.

This way, rather than retroactively criticizing or asking someone to redo what's already done, work is just not done yet until it passes peer review. The back-and-forth with peers is as much a part of the software engineering process as the back-and-forth with the compiler.

You can post your concerns as comments on particular lines and have threaded discussions about them. He can do the same to your code. The discussion stays focused on the specific proposed code changes, and not someone's performance or competence in general.

Even brilliant senior engineers at my company are grateful when code reviews catch typos or force them to make things more clear. It's totally normal for new hires to require more rounds of iteration. Over time, you start reflexively fixing the kinds of problems you know will attract comments before posting a diff. Getting a higher percentage of your diffs accepted on the first try is how you know you're improving.

I'm not pushing back code constantly on what to a novice might seem like minor details, which can be quite frustrating, but I'm also worrying that being too 'soft' might prevent the guy from learning how to do some stuff.

These are both real possibilities and valid concerns. Ask the developer how they feel about it.

Assuming that you have some pull request or code review workflow, and it appears that you do, I would recommend noting things which are "noncritical" or "preferred".

If you see a PR in a similar state to what you are describing, with some minor stylistic issues or in need of noncritical refactoring, leave a comment, but also feel free to approve. Saying something like, "In the future, maybe try to avoid method names like this in favor of something like descriptiveMethodName" documents your opinion without forcing them to change it, or blocking their development.

Now they know your preference, and if they are trying to improve, would hopefully notice such a situation in the future. It also leaves the door open to them actually changing it at that time, should they agree with you and see it as critical enough.

I'm dealing with a junior developer who's working remotely from a coding factory.

That is, unfortunately, not an ideal situation: one advanced programmer in charge of one novice programmer, with geographical separation. It is not surprising that there is some tension, but the disparity can be mitigated.

I'm curious, what doe you mean by "coding factory"? That terminology, to me, indicates a troubling attitude that may be exacerbating some of the management issues you have mentioned.

… violation of SRP, God objects, non-meaningful names for methods or variables; I point out what he has to fix and try to explain why it is wrong.

The problem, I think, is that your junior developer is spewing code without having gone through a proper design process. This is a failure on your part, as the manager and senior developer, to provide guidance and teach good software development habits. You can prevent bad code from being written in the first place if you first work together to sketch an outline. That would be preferable to criticizing and rewriting code after it has been produced, in terms of both efficiency and morale.

You'll probably need to readjust your workflow. It sounds like you are currently expecting him to deliver products to you. What you need is closer collaboration, so that you can provide guidance at every step of development. Talk about the design and interface before coding starts. While coding is happening, do more frequent checkpoints, to catch problems earlier. If possible, try pair programming, via screen-sharing with an audio channel.

All of this will require more effort on your part, but it will probably be worthwhile, considering the problematic relationship that you currently have.

Another idea for dealing with "too much criticism" is to do a task by yourself from time to time, and let the a junior developer do a code review for you. This has at least two advantages:

  • they can see how you expect things to be done.
  • in cases when there are multiple good solutions or variable names I accept suggestions of different but (almost?) equally good approaches. When I fix my code because of someone's comment, I'm trying to show people that they are respected and the criticism always concerns only code - regardless of who the author is.

If it is a violation of coding standards, show him/her where it is so he/she knows. If you have to keep showing him/her the same mistake then you might have an issue with someone who either can't conform to the rules or refuses to. Don't use your spare time to fix the mistakes. This person should fix their own errors so they don't make them again.

Always tell them what they did right and how they can improve next time. We can always get better in some area. Feedback is critical in becoming better at anything.

Licensed under: CC-BY-SA with attribution
scroll top