221–230 of 309 posts
For non-critical stylistic or refactoring I prefix with "Consider". > Consider extracting this to a separate function to make this more re-usable. This gives the developer the option to disregard the comment. Essentially I am trusting that they will consider it. It shows that I trust their judgment instead of ordering them. For stuff that is more important but not critical I prefix with "I'd recommend". I'd recommend…
What do you mean to say when you want to review code? Personally, I have roughly three distinct levels of things that I comment on in code reviews: 1. Does not need response, but is my personal opinion about how the code probably would look nicer. In this case, I would do #2. If you just say "no," that's fine. I can revisit the issue later. Some people think these comments don't actually belong in a code review, and…
> Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason. This. We do it on our team as well. Especially whe…
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…
You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like once you get over the hump you could just slowly add rules as you get tired of forgetting about them/people pointing them out to you.
> And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!
This is a concerning attitude. If the team has agreed on a set of style guides to follow then you should follow them, argue to change them, or move on.
If you don't have time to adhere to the style guide now, why do you think you will have more time to go back and fix it later?
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…
Just wanted to chime in and say that I still remember the first code reviews I received and while I didn't take any of the comments personally, they certainly didn't feel great to receive. (I should add that I don't begrudge the reviewers, they were nice people). These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you a…
Earlier quoted context omitted.
I also think it depends a bit on the other person and the rapport one has with them. At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone. But for someone I've worked with for years we're often a bit shorter and to the point, and…
The best way I've found to introduce junior developers to code reviews is to pair on it. Just go side-by-side down the diff and hop into an editor when necessary. It also helps teach a developer how to review code and what to look for--which has knock-on benefits of its own.
I was junior for most of that time and I think "use a totally different approach" or "change these 10 things" can sting a lot less that way. Your mistakes aren't all in black and white for the world to see, people can tell when you get something and don't end up patronising you, but you can also blur the lines a bit between "hey, what do you think of this, not sure" and "I think this code is 100% ready for merge and officially request permission to do so" - like, I know there are going to be comments, pretending otherwise will just make it worse.
Most of the time you'd get a review immediately too, less complicated structure then so one approval was usually enough, and it made sense for that to be high priority in my mind - this task is one step away from being done, I don't mind being interrupted.
The only thing is when I was the reviewer, it maybe went a bit too far one-sided learning experience, sometimes comments would get brushed under the carpet without much explanation/there was an assumption I'd approve.
“What might happen if the input is a null value?”
Sometimes they realize things on their own after one or two questions. Sometimes many. But it’s always so neutral and keeps them in the driver’s seat.
I do code reviews as the meat of my job and have personally benefited from GitLab's handbook article on this topic [1] - No ego (Don't defend a point to win an argument or double-down on a mistake.) - Assume positive intent (If a message feels like a slight, assume positive intent while asking for clarification.) - Get to know each other (Building a rapport enables trust.) - Say thanks (Taking every opportunity to sh…
Ego isn't bad. To use it in a context directly indicating it's bad is wrong. If the world agrees to do away with the word ego then it must be replaced by something that also means "a person's sense of self-esteem or self-importance.".
A coder without any ego is a coder who doesn't care, at all, about anything.
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…
Crying? Did you leave some Linus Torvalds level feedback, like tell them to find a new career or something?
"You wrote the code, and you seem to be unable to admit that your code was buggy. It's not a compiler bug. It's your bug. Stand up like a man, instead of trying to flail around and blame anything else but yourself."