3

I am a senior software engineer in a company who has the role of code reviewing. Last week I have rejected two PRs from juniors, one of them was simply duplicating a lot of code (~5K) lines with the names of the functions changed. The junior engineer urged me and other reviewer to accept the PR and I quote I want merge it and close the PR and the ticket as it has been open for a while, then we can make your requested changes later., and was very upset as me and the other reviewer rejected the code to be merged as we see it will cause huge problems in the future.

The other PR was not that bad, but it's introducing rigid inflexible APIs that will break compatibility in the future. ( FYI, existing APIs are very inflexible and we want to change them to more flexible generic solution ). We have had 2 design meetings before about these APIs in 6 months time-frame. I quote the following

Me: Adding these non-generic inflexible APIs adds very small value and may not serve the requests from other teams. We will also remove it later.
JE: Adding these APIs is not a problem, as we will remove them later and provide generic APIs. We already have inflexible APIs
Me: But this will make the effort on the users high, as it breaks compatibility
JE: We will break compatibility in all cases, so we can add it and remove it later. This is highly requested feature and had been requested a year ago.
Me: Okay, whatever you see.

We were very considerate in our words that we only talked about the code itself and not the engineers in any way.

Our scrum master asks us that we shall coach the juniors behaviorally and I disagree.

Is it my role as senior engineer to teach people that this is normal in the software industry ? To teach them how to act behaviorally in design/review meeting.

PS: My company goes for no-blaming culture at all, and I feel that it's more like no-accounting culture instead of no-blaming.

Improve this question
8
  • 4
    Why do you disagree? Why do you think a Senior dev should not be coaching or teaching Junior devs?
    – DarkCygnus ♦
    Commented Sep 23, 2022 at 0:29
  • 5
    "we can make your requested changes later" & "so we can add it and remove it later" - speaking from personal experience, "later" never happens. I think you can coach behaviorally though, but more by example than explicit "coaching".
    – brhans
    Commented Sep 23, 2022 at 0:47
  • 1
    What exact behaviour do you think should change in each case? Duplicating code? Writing inflexible APIs? Getting upset?
    – matt freake
    Commented Sep 23, 2022 at 5:03
  • 2
    Behavior tends to affect the way people code. Not addressing potential behavioral issues risks you revisiting the same 'technical' problems over and over
    – morsor
    Commented Sep 23, 2022 at 9:47
  • 2
    I would agrue that it is one of the most important responsibilities of a senior software engineer to train, guide, and mentor junior developers. I am very surprised that this doesn't seem to be the norm in all companies.
    – spickermann
    Commented Sep 23, 2022 at 13:59

3 Answers 3

Reset to default
12

Is it my role as senior engineer to teach people that this is normal in the software industry ?

Every company defines the role differently.

Some companies expect their senior engineers to simply crank out code. Others expect more.

When in doubt, ask your boss about your role.

Improve this answer
2
  • 2
    @ShadyAtef This answer hits the nail on the head. There is no hard and fast rule about what a role should entail. If there is confusion about what a role entails, speak with your manager, and they will give you clarity. This SE is full of answers where the answerer has assumed that their way of working is The One True Way, but in reality, it is far more nuanced.
    – Gregory Currie
    Commented Sep 23, 2022 at 12:56
  • 2
    ...and some of the worst problems are caused when there are differing expectations and no communication to clarify them
    – matt freake
    Commented Sep 23, 2022 at 12:57
7

Written communication, especially in the form of a pull request, can come across as (passive) aggressive, even if it's not inteded to be. This also goes for emails and Slack/Teams chats. But the whole point of a PR is to put someone's work under scrutiny.

Employ a little empathy and think back to when you were starting out in the industry: you're out to impress everyone. Your peers, your boss. You may even think you know it all. You're under pressure to prove yourself. All developers have been here at the start of their careers - generally, the 'bad attitude' you're describing comes from a good place, all in all. It's important to recognise this. Everyone wants to do a good job.

Couple all of this with the fact that software developers, in general, are not exactly known for diplomatic skills. This is what sets apart the Junior from Technical Leadership.

However, keeping the quality of the code-base up to snuff is your role, and you shouldn't compramise on this.

Give the Junior developer a call (if in seperate offices/home working) or go over to their desk for a 15 minute chat. Keep it informal, and don't 'book in time' or say this is 'training'. That will only make the situation worse.

Firstly, explain that your feedback isn't aimed at them personally, then convince them round to your way of thinking: go through each point on your PR, and give concrete examples as to why you want it changed, and explain the logic behind it. If you can allow the Junior to come to his own conclusions (as well as your own), even better. This is all part of the learning process, at any level.

Some people get on with written communication better than 'the personal approach' - I'm somewhere in the middle, I like a bit of both. Sometimes, comments on PRs can come across as "I want this changed because I don't like it" - and no one likes that, right?

Improve this answer
1
  • I don't necessarily agree that constructive criticism in a review (even if it means "rejecting" the PR) requires talking to the person. That's an expected outcome of a PR, and the comments should only be about the code and never attack the developer in the first place. However, once there's some kind of disagreement or misunderstanding, a face-to-face talk can make a ton of difference.
    – Llewellyn
    Commented Sep 24, 2022 at 20:11
0

I think as a professional you should say what's wrong with the junior. The junior should have the attitude to correct himself. If things persist you need to escalate or involve HR --who can send the person to a behavioral training-s.

Improve this answer

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .