95

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.

closed as off-topic by gnat, GrandmasterB, Thomas Owens Jun 7 '17 at 23:27

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions seeking career or education advice are off topic here. They are only meaningful to the asker and do not generate lasting value for the broader community. Furthermore, in most cases, any answer is going to be a subjective opinion that may not take into account all the nuances of a (your) particular circumstance." – Thomas Owens
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 7
    How much time have you spent making sure he knows what your expectations are? – Blrfl Jun 6 '17 at 15:58
  • 5
  • 13
    @gnat I think it's not the same case, my problem is not that we're exceeding estimates or overdoing code. Matter of fact we're ahead schedule. My question is how to 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. – Zalomon Jun 6 '17 at 16:03
  • 4
    I edited this to make it more on topic. I believe this is different than the linked duplicate question as well. – enderland Jun 6 '17 at 16:16
  • 3
    Some things I've tried that help with this: pair programming - gain insight into how he works and thinks and maybe expose him to your thought processes as you go through your code. Find the tasks he's good at - sometimes you'll get better results throwing a bunch of small bugs at him vs large feature work. Tech designs - give him first crack at designing software without touching the code. This lets him think about structure and process vs putting his head down and churning out code. Lastly, good luck. It sometimes takes more persistence than expected but is worth it if he becomes productive. – Sandy Chapman Jun 6 '17 at 22:02

13 Answers 13

84

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.

  • 65
    @9000 it's amazing how often people are frustrated about people not contributing according to their standards, when those standards aren't even documented anywhere... – enderland Jun 6 '17 at 16:24
  • 32
    Variable names are utterly important in describing the intent of the code; method / function names, even more so. Getting the names right is not useless perfectionism, it's required for maintainability. – 9000 Jun 6 '17 at 16:24
  • 9
    @Zalomon Definitely mention it to him; more, turn it into a discussion. As a junior dev I worked with a few different senior devs. My best experience was with a dev who talked me through all his recommendations - even "small" ones like a slightly better name for a variable. Not only did I learn a ton from him, but I felt so good that he was listening to my thought processes and including me in the discussion - it made me want him to review my code more so that I could learn more. (continued) – dj18 Jun 6 '17 at 16:44
  • 6
    @Zalomon (continued) On the flip side, a senior dev who made changes behind my back (even if you tell him afterwards, it's still behind his back) was totally demoralizing - it made me feel like I was working with an autocrat that I had to figure out how to please. – dj18 Jun 6 '17 at 16:44
  • 6
    My (small) experience of mentoring junior devs shows that making a dev think hard about the proper name and express the purpose of the data in the variable name is worth the while. It helps the dev actually understand what their code is doing, in a much less hand-wavy fashion than they are used to. It sometimes leads them to find logical errors in the code involved, or just better ways to express things. (Same thing works for me; the difference is that I have the discipline internalized, thanks to strict code reviewers I had in past.) – 9000 Jun 6 '17 at 16:49
19

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.

  • 2
    On the verbiage point: when you have to use a personal pronoun, simply choosing "we" instead of "you" also depersonalizes critique, in my experience on both sides of code review. – Josh Caswell Jun 7 '17 at 13:15
6

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.

5

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.

  • One of the ways to avoid endless cycles of code reviews is to make changes small. No pull request is ridiculously small if it makes a beneficial change (I made my share of 1-char PRs). The smaller the better. Mercilessly cut the scope of tickets handed to junior devs, and keep them getting the things done. Once they are good at the whole read-understand-code-test-review-deploy process, the scope can increase. – 9000 Jun 6 '17 at 16:56
  • 1
    I don't think that it's a good idea to tell the dev "they aren't very important" if they are important enough for the OP to go back and change later. – enderland Jun 6 '17 at 17:51
  • 3
    @enderland In my experience, there's no such thing as perfect code. You can always improve upon it later so that's not really relevant here. The OP says these are things to "fix it if I have any spare time". There are two kinds of problems. Things that must be fixed and things than don't need to be fixed. The latter may or may not be fixed and these sound like they fall into that category. It's a judgement call at some level but I think if, as an experienced dev, you aren't sure it's worth calling out as a must change, it probably isn't. – JimmyJames Jun 6 '17 at 18:08
5

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

  • 3
    Or "(b) And I know you can do better.". If he asks "teach me" you know he's in the right path. :) – Machado Jun 6 '17 at 19:26
  • 1
    In my previous position, we used Stash/BitBucket as our code review tool. It had the option to block a pull request by either setting an outstanding task or outright declining the pull request. I'd only use these for necessary changes. If there was a cosmetic change (eg. minor code readability, better data structure, refactoring) I'd point it out with suggestions, but not add a blocking task, do it if you have the time. This also prevents missing deadlines over minor fusses. – Hand-E-Food Jun 7 '17 at 0:05
4

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.

  • 3
    I like the idea of the peer review, for the moment it's just two of us on the team but I can have him review my code. If he can understand what I do and find some inconsitencies it can be helphul, both for the code quality and his morale. It helped me when I was learning seing that I was not the only one making mistakes +1 – Zalomon Jun 6 '17 at 18:52
2

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.

2

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.

1

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.

  • Matter of fact he told me he was feeling dumb. I'm trying to keep criticism in the positive side and I told him I was happy with his work, I also explained him that I had this kind of doubts when I was starting, but I must assume that some of the feedback on giving him to improve his work it's coming out wrong. – Zalomon Jun 6 '17 at 21:29
  • 2
    Explain that you wouldn't waste your time criticizing his code carefully if you didn't expect it to payoff in him becoming a better programmer. And be scrupulous in distinguishing between "you need to fix this for the code to be acceptable" and "here's something you can do even better next time". Providing large amounts of insightful and accurate criticism is the biggest gift you can give a junior programmer. Failure to do that makes them a worse programmer. The criticism should start tapering off. You only have to make every mistake once, maybe twice, and there are only so many mistakes. – David Schwartz Jun 7 '17 at 5:08
1

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.

0

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.

-1

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.

-1

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 learn how things should 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 showing people that they are respected and the criticism always concerns only code - regardless of who the author is.
  • I'd be happy to learn what's wrong with my post. A downvote is an understandable sign of disagreement, but delete votes?! – BartoszKP Jun 7 '17 at 18:54

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