Confessions of a programmer: I hate code review (2010) • Hacker News Board

Re: Confessions of a programmer: I hate code review (2010)

#161

Post by Vinnl » Tue, Oct 30, 2018, 10:32 AM UTC

cimmanom wrote: post #131

Vinnl wrote: post #124

Earlier quoted context omitted.

Well, I guess we'll have to agree to disagree there. It's far easier to continue working on the same thing, even if I have to make a pull request (or simply have to write a commit) in between, as long as that's still related to the same change. If I have to switch between entirely different problems, that has more impact on my productivity than having to switch tasks while still working on the same problem. Again, in…

This still sounds to me like it might be a process issue around code review rather than code review actually being the problem. What else would you be doing on the same problem post-PR if you didn’t have to wait for code review?

Otherwise I'd be working on the next step of the problem? Which I can still do but, as OP mentioned, does lead to more work if feedback comes in on the first part while I'm already far underway with the second part.

Re: Confessions of a programmer: I hate code review (2010)

#162

Post by RandomInteger4 » Tue, Oct 30, 2018, 2:32 PM UTC

dredmorbius wrote: post #125

RandomInteger4 wrote: post #58

Earlier quoted context omitted.

My CSS-foo is fairly weak; what problems would em cause such that they disallow it in the code base?

Both em and rem refer to element sizing. Rem is relative to the base page font size , while em is relative to the containing element font size . The consequence is that nested em specifications are cumulative; 1.2em of 1.2 em of 1.2em gives 1.2^3, while 1.2 rem is fixed , antwhere it is used on the page. This is not of itself good or bad, it is behaviour . Use it to desired effect. As examples, I prefer to specify fo…

So in essence, styling a component with em can reduce reusability of components more and more as em gets introduced in various places?

Re: Confessions of a programmer: I hate code review (2010)

#163

Post by dredmorbius » Tue, Oct 30, 2018, 8:35 PM UTC

RandomInteger4 wrote: post #162

dredmorbius wrote: post #125

Earlier quoted context omitted.

Both em and rem refer to element sizing. Rem is relative to the base page font size , while em is relative to the containing element font size . The consequence is that nested em specifications are cumulative; 1.2em of 1.2 em of 1.2em gives 1.2^3, while 1.2 rem is fixed , antwhere it is used on the page. This is not of itself good or bad, it is behaviour . Use it to desired effect. As examples, I prefer to specify fo…

So in essence, styling a component with em can reduce reusability of components more and more as em gets introduced in various places?

It's not that clear cut.

The 'em' sizing is inherited by child elements, which can be either a feature or a bug, depending on interactions.

If you're aware of this and design for it it can be a useful feature, and enhances reusability. If you're unaware of this, or your design is complex, it's much more likely to be a bug. "Bug or not?" is entirely dependent on that context.

Re: Confessions of a programmer: I hate code review (2010)

#164

Post by cimmanom » Tue, Oct 30, 2018, 8:48 PM UTC

Vinnl wrote: post #161

cimmanom wrote: post #131

Earlier quoted context omitted.

This still sounds to me like it might be a process issue around code review rather than code review actually being the problem. What else would you be doing on the same problem post-PR if you didn’t have to wait for code review?

Otherwise I'd be working on the next step of the problem? Which I can still do but, as OP mentioned, does lead to more work if feedback comes in on the first part while I'm already far underway with the second part.

Ah, we don’t do code review mid-project except for very large projects. For those large projects, we just keep working in the same branch and apply the code review changes when they come in. It’s not much of a disruption.

For preliminary foundational work on a big project, where you’d have to change everything if the early work changes in response to code review, one good solution is to do an informal design review before jumping in to get feedback on plans for data model, architecture, boundaries, and APIs from a high level.

Re: Confessions of a programmer: I hate code review (2010)

#165

Post by goto11 » Mon, Nov 05, 2018, 11:58 AM UTC

regularfry wrote: post #145

goto11 wrote: post #41

Earlier quoted context omitted.

What kind of stylistic changes are you talking about? If it is things like whitespace and brace placement style - yeah you are wasting your time. Decide on a style and have a tool enforce it. But code reviews are for other things than just catching bugs, it is also for sharing knowledge, learning useful idioms for each other, discussing ideas and so on. I have learnt a lot from code reviews, even when the reviewers w…

> What kind of stylistic changes are you talking about? It's the level above the decisions a tool can enforce which I've seen tending towards bikeshedding. > But code reviews are for other things than just catching bugs... I don't dispute that there is value here. I think that value is tremendously overplayed in comparison to the costs incurred. Or rather, I think the costs are underplayed. > And why is it a bad use…

Code reviews should not devolve into bikeshedding. That is a problem of people not understanding the purpose and value of code reviews, not a problem with the concept of code reviews. Ignore the issue if it is not important or make a ruling and move on.

If your code reviews find bugs left and right I would say it indicates a deeper problem: Why are there so many easy-to-spot bugs in the first place? The amount of bugs found is definitely not a measurement of success. The purpose of review is to improve overall readability, competence, and code quality to level were fewer bugs are introduced in the first place.