271–280 of 309 posts
But I see code review as a conversation. Quite often it calls for a bit more discussion, and, depending on how the team deals with this, I sometimes reach out to the developer whose code I reviewed over a separate channel to discuss the issue a bit more.
Earlier quoted context omitted.
> because "I hadn't had my coffee yet har har" is a pretty thin excuse. I'm pretty blunt/rude in the mornings, within half an hour of waking up, even these days. Back then, I did not know that about myself -- or rather, just how rude I came across. Most people I interact with never see me like that, but it generally takes me 10-15 minutes to get some coffee going. Hence why I said "before coffee" since I assume nearl…
I think you're receiving pushback in the comments because it comes off as though you think it's OK to be rude in the morning. Additionally, if you made someone cry I think most of us draw comparisons to similar situations we've seen where, generally, the person who thinks they're just being blunt is actually very offensive and usually the most sensitive to critique.
However, I think it’s worth pointing out, that at no point in the mentioned code review was I mean, or disrespectful. I was just direct vs. my usual indirect self. I never said anything like “this is stupid” or anything personal. I didn’t have any malicious intent nor did it come off that way, unless you had gotten a review by me before and knew how I usually code review:
“I think this would be better expressed as X for performance, wdyt?”
became:
“Express this as X for better performance”
I didn’t use my lack of being awake as an excuse at work. I apologized and just said I didn’t realize how wording things differently would affect my coworker and it truly did upset me as well. It probably wasn’t until I worked with someone who always reviewed like that did I realize how annoying that kind of wording actually was, but that wasn’t until years later. Anyway, it was a weird day, because I wasn’t intentional in my wording and I paid for it. It’s especially weird when you know exactly what happened and you want to explain it so that maybe the other person feels better about the situation but doing so would just come across as being an ass or just trying to make an excuse. It’s a terrible feeling when it’s a real issue in your own life, but other people don’t have a basis to relate. Further, it a terrible feeling to see someone suffering from it and all you can do is feel shame, and so sorry for the person you’ve said something bad too without even being aware of saying it, or unable to stop yourself from saying it.
So yeah, I don’t think it’s ok, but I’ve accepted it at this point in my life. Whatever bs comes out of my mouth, I accept responsibility for that bs.
My rules for reviews (which influence the language used) are: * Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here"); * Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning; * Don't use questions that invite discussion if you're not inviti…
> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect…
I hated it. It absolutely should be automated, but I could not find any tools for formatting Cypher query strings inside Javascript code. I'd probably have to write it myself.
Earlier quoted context omitted.
To me those questions could come off as condescending. I say could because I wouldn’t think that’s your intent, but the tone is there.
That depends entirely on whether you add the question as a formality or if you are genuinely interested in why the other person might disagree.
If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.
I feel like this is often overlooked. One morning, I did a code review before having any coffee. I basically used the latter two throughout the whole thing. I came into the office to find my coworker literally crying. Later in the day, someone else said it was the best code review they’d ever read… and asked me to come to their team… There was so much drama from that code review. That code got merged as-is to appease…
People are sensitive. Directness is underappreciated. Inability to take critique is killing code quality. Try going to an art school. Critique is how we get better. Check your ego at the door and you'll go far as a programmer
I wouldn’t be happy working in the team where people constantly consider #6 as rudeness. This looks so unproductive and unprofessional to care too much about the tone vs content in programming business.
That's fair if you have the same stance for all social interactions. If you don't then it's odd since nothing about programming makes people less human and less impacted by the usual social considerations. Although even if that is your stance for everything it's likely not the stance of everyone on the team.
I put a clear line between social and work interactions.
And from my experience just being direct (not intentionally rude of course!) in work environment pays off dramatically versus wasting time trying not to hurt someone’s feelings.
I feel like this is often overlooked. One morning, I did a code review before having any coffee. I basically used the latter two throughout the whole thing. I came into the office to find my coworker literally crying. Later in the day, someone else said it was the best code review they’d ever read… and asked me to come to their team… There was so much drama from that code review. That code got merged as-is to appease…
People are sensitive. Directness is underappreciated. Inability to take critique is killing code quality. Try going to an art school. Critique is how we get better. Check your ego at the door and you'll go far as a programmer
I've seen modern art - it's not working.
I’ve introduced Must, Should, Could at every company I’ve been at (except the one where I picked it up myself) and it works wonders. Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone. Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if t…
So "Green: Why did you do it this way?" immediately comes across differently to "Red: Why did you do it this way?"
We don't have any junior devs at my current place so I'm reviewing code from devs with similar experience as myself, therefore I ask more questions. "Could this be improved by doing x?", "Was there a reason you did x over y?", etc. Then when I'm done with the review I normally send a message to the reviewee just letting them know I'm done and happy to discuss if they have any questions or thoughts. Most of the time I find there is some reason why decisions were made and we can discuss that if we need to. Alternatively they'll agree with my suggestion and just go ahead with making the changes.
The reason I like questions is because I'm not perfect and the person who wrote the code spent a lot more time thinking about it than I have. As a reviewer of another senior dev's code I see myself as a fresh set of eyes, not as someone with more ability or authority.
Some obvious things like typos or formatting issues I'll just note without a question. "Typo here", etc. There's really no discussion to be had in those cases. What I wouldn't do is say something like, "Typo here. Fix this please". The "fix this" is unnecessary and comes across as a bit rude imo. I know when my code is being reviewed I just want to be shown what I've done wrong / what I can improve. I don't want orders. If I've made a mistake I'll fix it and I trust others will do the same. I think that's generally the kind of mutual respect experienced devs expect from each other.
With junior devs there's room to be more authoritative, although tone is important. When I'm working with a junior dev and I think they've gone about something the wrong way I prefer to just have a chat with them and explain how I think they could have done something better. I think it's good to make it clear that you're not trying to be difficult, but you know there's a better way to do something and you want to help guide them. I know when I've been a junior dev on projects I've received some overly critical and demoralising code reviews. Realising the thing you've spent the last week thinking about and working on is wrong isn't a fun experience as it is, but a review with an overly critical and unconstructive tone can make it even worse.
That's just my thoughts anyway. What's right in your case likely to depend on the context of whos code you're reviewing and what you're reviewing. The only constants I'd suggest are in tone. If you're not being perceived as being respectfully constructive in your code reviews you're probably doing something wrong.
I’ve introduced Must, Should, Could at every company I’ve been at (except the one where I picked it up myself) and it works wonders. Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone. Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if t…
The word "must" should only be used in the Military. Anywhere else, it is profusely out of place. In its stead, I recommend the term "requirement" and then cite an actual reference by the PO or someone officially representing the business requirements.