0

I am in this situation where I have to review pull requests (PRs) from one of my colleagues, who has a very broad knowledge and years of experience, she is well respected and we all know that the quality of her results are very good.

However, from time to time, her PRs are just to complex for what we want to achieve, in my humble opinion and considering the sector we operate in.

We should imagine a PR that uses extremely advanced techniques to achieve something (e.g., zipping multiple stream, mapping objects to pairs on the fly, computing results based on left or right positions in them, generifying functions used in only 2 locations, or applying "crazy" SQL just to save 20 rows out of 250). We should also imagine that all of these objectives could have been done alternatively in some other form, for sure more basic, like for example streaming multiple times and holding the results in intermediate variables with nice names, or using 20 rows more in SQL but with a clearer intent.

I am able to follow them and I am able to test them, but I think that the effort to maintain them is just irrational. Additionally, I think that because of this huge intellectual work she puts in the code and because of its complexity, my colleagues also feel "castrated" when they have to review it (used figuratively, she may or may not be a woman).

I understand that if there is some security or performance related issue, then complexity is welcome, but most of the time it seems that the complexity is there just to show off and prevent the others to comment her work.

Would it be correct to comment one of this PR about the fact that advanced code is nice, but code maintainable and reviewable by the whole team without blowing your mind would be even better? Could I put a limit to the complexity that is introduced?

I am absolutely respectful of others' coding styles, and I pay attention to make sure that my comments could be identified as not very important, preferences, or something I expect is changed (a logical error, a bug, something serious). Here the problem is that the complexity seems gratuitous.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92
Pampa Nello
  • 233
  • 1
  • 2
  • 8
  • 1
    Have you talked to the rest of your team about this and established a consensus, or is this currently just your own personal view of the world? – Philip Kendall Mar 02 '22 at 19:37
  • Your code should be as simple as possible, and no simpler. (I’m quoting some very clever guy here). – gnasher729 Mar 02 '22 at 19:40
  • 2
    Did you talk to your colleague and asked her if she is fine when make some suggestions to detangle some of their complex statements for the sake of better readability and maintainability? Or is the real problem that you think she is the kind of person who takes every suggestion for changing her code as a personal insult, regardless how constructive it is worded? – Doc Brown Mar 02 '22 at 20:08
  • To @PhilipKendall, it is a shared feeling, but I would say that the rest of the team is more conflict adverse and I also suspect that they are afraid to appear incompetent if they ask to prioritize clarity over gratuitous complexity – Pampa Nello Mar 03 '22 at 07:50
  • @DocBrown I tried in the past and I got as reply "it is only because you are not used to", so implicitly that everything is fine. She is definitely the kind of person that will take any suggestion for changing as a personal insult. – Pampa Nello Mar 03 '22 at 07:55
  • 1
    @PampaNello: I recommend you search for "code review" at "The Workplace.SE", there are tons of Q&As about people problems in code reviews there, like [this one](https://workplace.stackexchange.com/questions/81148/how-to-handle-a-highly-productive-employee-but-who-reacts-extremely-emotionally) – Doc Brown Mar 03 '22 at 10:59
  • 1
    ... or this one: [Providing constructive feedback for someone who interprets it as personal criticism](https://workplace.stackexchange.com/questions/42264/providing-constructive-feedback-for-someone-who-interprets-it-as-personal-critic) – Doc Brown Mar 03 '22 at 20:26
  • Curious, would you say that the code this person authors follows the SOLID principles? https://en.wikipedia.org/wiki/SOLID – Jason Weber Mar 05 '22 at 03:55
  • @JasonWeber I asked my self the same question, I would say that she usually does not follow the first and the second one, usually putting before the need to keep the code short or reducing the size of a PR. – Pampa Nello Mar 06 '22 at 09:28

2 Answers2

6

Make her explain it.

Do this regularly and soon she'll write code that's easy to explain.

I run into this same problem daily. I work and work on something delving deep into the details. I finally master them. I make them do what I want. Then I emerge, proud of having defeated my foe, only to watch as tales of my exploits make eyes gloss over. No one understands what I've done.

Just because I finally understand how to solve the problem doesn't mean anyone else does. If I write code that solves it that no one can follow I have only solved it for myself (and maybe the CPU). I haven't solved it for anyone else.

Thing is, I'm a computer nerd. I didn't go into this field because I was good with people. Which is why they invented agile. It makes us introverts look at other peoples shoes regularly. It makes us talk about our work. It makes us try to get other people to understand it. They aren't mindless ceremonies in a check list.

Here's the real reason why a team is too large if takes more than two pizzas to feed. It isn't because lines of communication grow by n(n+1)/2. It isn't because 2 for 1 pizza deals are popular. It's because teaching your team how your code works shouldn't feel like public speaking.

So make her explain it. But take her communication issues seriously. Find what she's comfortable with and use it. Talk one on one. Pair program together. Be willing to set your ego aside and just learn. Try explaining what you've learned to others. Show her how you struggle. Invite her to help.

Also, don't wait. Code is like quick drying cement. If you really want her to change this code, don't bring up issues a week later when she's working on something else. You need to catch her when this is fresh in her mind. The longer you wait the more she'll resist changes.

This is why pair programming is so effective. You get to comment on the code when it's brand new. When you can't do that the next best is something I call code stalking. It's where you use your tools to poke around in other repositories before they are submitted, just to see what's coming. Sometimes you find something worth having a chat about.

All too often reviews become mindless check lists that produce comments no more helpful than: "You forgot to comment line 42". No, you need to feel comfortable with the code. Do the work to make that happen. Even if it's not on the check list. Don't settle for leaving her code an impenetrable mystery. Because someday it will need attention and she wont be there.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
2

I see two things that are potentially at play here: an expert who needs recognition and power and a fundamental change in computer architecture that as forever altered software engineering: 64 bit systems.

Up until the time that 64 bit memory addressing became the norm, it was basically always the case that you were constrained by memory. There were other concerns, for sure but memory was the one that would get you in the end. Then came 64-bit architecture. Effectively, you are only constrained now by how much memory you can afford and BTW, memory is super-cheap. It might be useful to discuss this with the expert if they 'came up' in a different era.

Another thing you might want to consider is that pushing the limits of the team's understanding is not necessarily a bad thing. First of all, your team will become more skilled and/or you will find their limits. Secondly, your high-potential people should want this and you should point that out to them. If this individual has the chops, let them mentor people. Zipping streams and mapping don't seem crazy to me. They just seem like effective techniques that more developers should learn.

If this person feels the need to prove themselves valuable and is over-doing things to make that point, you can resolve that. Let them be a leader. If they are given responsibility over results, they should find the happy medium.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92