251–260 of 304 posts
Disagree with "Don't teach during code reviews." I've found them to be invaluable opportunities for knowledge exchange
But...this article isn't about not teaching. It's about not trying to be coy, hinting that the code needs some unspecified change.
Absolutely do tell me how my code is flawed, and how it could be better. This is what I would call teaching.
Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. My job is to ship software, and as an engineer/team lead it is my job to ensure the code we ship ( I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line and when I work with peers or more experience people, I barely leav…
> I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line I feel this, and have personally experienced submitters taking feedback far too personally, to the point of even using sunken cost as justification of their design, completely overlooking valid points of feedback. I’m looking for basics. Will it work? is the code clear? is it commented/documented…
Thirty years later, egoless programming is one of Robert Glass's favorite fallacies in 'Facts and Fallacies of Software Engineering.'
IOW, it is a hard problem!
The SE research community has been looking into this for awhile (e.g. Greiler, Bacchelli)
[0] 'The Psychology of Computer Programming' (1971), Gerald Weinberg
[1] 'Facts and Fallacies of Software Engineering', Robert L. Glass
Earlier quoted context omitted.
If I thought the junior was not up to the task I assigned them, I would have had them consult with a senior developer during the task. I'm literally talking about the case where a senior developer decides that this task was not implemented to their personal standards AND makes it a "teaching moment" that means it doesn't get delivered this sprint. Saying "this can't ship without significant rework" is something that…
This absolutely does happen and is a problem. Funny seeing so many replies mischaracterizing what you're saying.
Earlier quoted context omitted.
I just strongly disagree with this. The context of a code review is the perfect opportunity to teach, much like a technical design review is for system designs, and having them sit as an observer during an incident response is for triage/debugging a live system under stress. There are no better settings for teaching these skills to juniors. And you should really let your senior engineers take advantage of these times…
> The context of a code review is the perfect opportunity to teach I think we are just talking about different scales and different senior developers. Every code review is either teaching something, finding missed bugs, find better ways to do things that a senior developer makes full use of. If not, your code review is just a preformative waste of time. I'm talking about simple PRs, the kind I would assign to a junio…
There is a balance to be found between pointing out what is going to cause problems and what you find personally inelegant. What I find personally inelegant I typically leave as a suggestion, while approving the MR as a whole. The contributor - who has spent more time than me thinking about his code in the context of the issue - can finish things up on their own, taking my feedback into account where they see fit. Th…
> what you find personally inelegant. I call this "reviewing for accent". Like, it probably doesn't matter if you use !!foo or Boolean(foo), we all know what they both mean.
I believe she had a good intention but her idea is not good at all. The Socratic Method is a fantastic way to trigger a health conversation about a topic without forcing a "senior" idea over a "junior". In my experience, asking someone to explain their intention behind a piece of code is a good way to: 1) Help the individual validate that the code is communicating their intentions 2) Discover if there is an different…
What, exactly, is wrong with "I think the current name of this function fails to encapsulate what it does. I think openRequest() would be a better name."
[1] At least, that's how it comes across when reading a lot of his articles.
Earlier quoted context omitted.
> You see a clear dichotomy between well written code and what your senior engieers see as enforceable best practices. No, in general, I agree with code review comments my senior developers make. I'm talking about the difference between "best practices" and actual best practices or in the difference between "this works, but next time, a better architecture would be x, let's refactor next time we come back to this" an…
“this works, but next time, a better architecture would be x, let's refactor next time we come back to this” In my experience there is rarely ever a “next time” and “fix it later” becomes “fix it never”. It’s always cheapest to fix worst practices up front rather than letting them metastasize into a huge pile of technical debt later. It’s also rare to find anybody interested in or willing to go back and fix old stuff…
Then the code that gets touched the most gets touched up the most. Code that doesn't ever get touched is fine being kinda crappy.
I disagree with the title but found myself agreeing with many points in the article. “Don’t be condescending” seems like generally applicable advice. But IMO, sometimes you just know something the code submitter doesn’t (or vise versa) and discussing that can be useful. And i think that’s pretty much teaching!
Yeah, and it doesn't take much to convey that this is a conversation between peers. A couple simple changes I get a lot of mileage out of. Where once I'd have written: "Do [X]" Now I write: "I see [problem Y], consider making [change X] to improve it" If the reviewee agrees then the change is easy and straightforward to make, but if they're unconvinced then the phrasing invites a dialog. Or if I think I see a bug, I'…
Earlier quoted context omitted.
> While we're extrapolating, are you the type that makes junior developers rewrite their loops as list comprehensions or the type that makes them rewrite their list comprehensions as loops? Because I've seen both in code reviews. One of them is right, because a codebase that consistently uses one or the other is better than a codebase that mixes the two. If the team hasn't made and communicated a decision then that s…
That doesn't really seem like the sort of thing that needs to be standardized at all, though. They're both valid, they're both readable to anyone competent, just do whatever feels right.