103

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.

Zalomon
  • 1,200
  • 3
  • 8
  • 18
  • 7
    How much time have you spent making sure he knows what your expectations are? – Blrfl Jun 06 '17 at 15:58
  • 5
    Possible duplicate of [How to stop gold-plating and just be content to release working developments](https://softwareengineering.stackexchange.com/questions/167752/how-to-stop-gold-plating-and-just-be-content-to-release-working-developments) – gnat Jun 06 '17 at 15:59
  • I made a document with software guidelines that included examples and counter-examples. Also, both when I ask him to change something or I change it myself I inform him what I'm trying to achieve. – Zalomon Jun 06 '17 at 15:59
  • 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 06 '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 06 '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 06 '17 at 22:02
  • 1
    accomplishment before perfection ! – Sikorski Jun 07 '17 at 07:08
  • 1
    Have you considered peer reviews and linters? Have junior developers review each others code, or even your code. When a learner looks for improvements in others code it can improve ability to see them in their own code. When the time is right, encourage use of a lint tool, preferably one that autocorrects or lints on the fly. Good linters like rubocop can even spot simple refactoring opportunities. This means the code review can concentrate on the big structural issues and defensive coding gotchas that newbies can't see, – dcorking Jun 07 '17 at 10:49
  • 1
    The only time you should have no mercy is if the developer writes an opening curly bracket `{` on its own line, when it relates to a `for`, `if`, etc. ;) – Andrea Lazzarotto Jun 07 '17 at 16:33
  • How much design is a remote junior dev doing? Usually with larger teams there would be some help with upfront design so things like God objects would never be considered. – JeffO Jun 07 '17 at 16:34
  • @JeffO Not much design, since the tasks that are handed to him are not huge. But he started to put helper methods on classes totally unrelated to what he was doing. That does not make a class a God class automatically but it's the first step. – Zalomon Jun 07 '17 at 16:40
  • +1 for linter tools, either on push or in CI. Most of these issues can be caught without human intervention. If they run alongside automated tests, you get much quicker feedback loops, and fixing these small coding style issues becomes much less tedious. Another nice factor is that it offsets the blame all around (there is less ego bruising when the machine tells you you're wrong than when a human tells you you're wrong) – tom Jun 07 '17 at 19:55

13 Answers13

87

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.

enderland
  • 12,091
  • 4
  • 51
  • 63
  • Thanks for the feedback. Regarding your questions: I'm not sure what you mean by CI; I do have a developer guide; and when I change something I do it only because it's clearly wrong but not an outrageous mistake such as variable names in Spanish or method names that don't state the intent of the method and when I do that I tell him what I did and why. – Zalomon Jun 06 '17 at 16:22
  • 67
    @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 06 '17 at 16:24
  • 33
    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 06 '17 at 16:24
  • I do think that too. But unless the name something awful like 'objNew' is enough to push back and make the junior fix it? Or should I inform him that his code is ok but I made a few minor changes to it and explain him what I did? – Zalomon Jun 06 '17 at 16:28
  • @Zalomon if it's not, then you shouldn't need to go back and change it. – enderland Jun 06 '17 at 16:33
  • 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 06 '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 06 '17 at 16:44
  • 1
    @dj18 that is 100% my experience too. – enderland Jun 06 '17 at 16:45
  • 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 06 '17 at 16:49
  • 3
    A "developer guide" might be helpful for formal stuff, but preventing *"violations of SRP, God objects, non-meaningful names for methods or variables"* requires a code review by an experienced dev, it is pretty ridiculous to expect a novice will fix these kind of issues just by writing a rule like "do not violate the SRP" into a dev guide. – Doc Brown Jun 06 '17 at 18:07
  • @DocBrown, for sure those things you need a code review and that stuff is pushed back and explained. – Zalomon Jun 06 '17 at 18:48
  • @dj18, I have to agree with you, when I mentored junior developer presentially for those things I'll call the junior to my desk and run him through the changes I want. I guess that with the guy being working on remote I can set aside a closeuot 15 minutes to guide him through this stuff, which I feel that is more productive than just pushing back the commit or change it myself. The remote team is bugging me a little bit, it's hard to grasp how someone is feeling, specially when they might be unsure about their own capacity (as we all were as juniors). – Zalomon Jun 06 '17 at 18:49
  • @Zalomon use video calls (or phone if you don't have video call functionality). Go visit onsite sometime as relationally that will be some of the best time/money spent. – enderland Jun 06 '17 at 18:50
  • @DocBrown the OP already addresses those issues with their colleague, it is the lesser "important" (for varying levels of important) issues they are concerned with here. – enderland Jun 06 '17 at 18:51
  • @dj18, your two-part comment is gold. Can you please post it in an answer so it can be properly voted up and linked to and so forth? – Wildcard Jun 07 '17 at 03:55
  • Architectural things like god objects should be handled in reviews (pull requests) but the minor things like code formatting should be handled by linters. A linter is an objective documentation of formatting rules. Just make sure it passes and you don't have to explain every minor formatting problem. – Sulthan Jun 07 '17 at 16:17
20

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.

Community
  • 1
Legato
  • 309
  • 2
  • 6
  • 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. – jscs Jun 07 '17 at 13:15
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.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92
  • 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 06 '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 06 '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 06 '17 at 18:08
  • IMO, It's unwise to end such a statement with "...but they aren't very important." That is only giving a junior dev—who may have all sorts of social incentives for ignoring your comments—license to ignore them, at his own career's peril. – Reverse Engineered Jan 07 '22 at 04:52
  • @ReverseEngineered Learning about what is important and what is not is a huge part of learning about development. By distinguishing between the aspects of your feedback that are critical and those that are not, you help the developer understand how to prioritize goals. And the more opinionated/preferential the guidance, the less it should be emphasized. – JimmyJames Jan 07 '22 at 15:20
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".

gnasher729
  • 42,090
  • 4
  • 59
  • 119
  • 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 06 '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 07 '17 at 00:05
5

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.

Machado
  • 4,090
  • 3
  • 25
  • 37
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.

John Wu
  • 26,032
  • 10
  • 63
  • 84
  • 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 06 '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.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
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.

closeparen
  • 646
  • 4
  • 9
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.

Kevin Krumwiede
  • 2,586
  • 1
  • 15
  • 19
  • 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 06 '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 07 '17 at 05: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.

zak
  • 19
  • 1
0

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.
BartoszKP
  • 248
  • 2
  • 10
  • 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 07 '17 at 18:54
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.

200_success
  • 1,568
  • 11
  • 20
-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.

acithium
  • 39
  • 1