[adrianN]

I'm an overly pedantic reviewer because I feel the review is one of the few places where I can counteract the accumulation of technical debt. Once the less than ideal code is in the codebase it's there to stay. I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.

[crdoconnor]

>I'm an overly pedantic reviewer because I feel the review is one of the few places where I can counteract the accumulation of technical debt.

Pedantic is the wrong approach to technical debt though. Almost every aspect of technical debt is about trade-offs.

If what you're doing appears pedantic it should come accompanied by an explanation of why the trade off the developer took is not ideal.

>I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.

Your code should really come accompanied by tests which you can read and use to understand what the precise interpretation of those requirements was.

[watwut]

> Once the less than ideal code is in the codebase it's there to stay.

That is not objective fact across projects. That means either the process or culture is bad.

[adrianN]

I've never seen a project where you go back to fix something just because its implementation is not ideal. If it works it stays, because there is always something more important to do.

[watwut]

I do that regularly. Not really when it is less then ideal, I don't think less then ideal necessary constitutes technical debt. But when it was hard to read and I just had to read it, when it was hard to modify and I had to modify it. Or when I was modifying something nearby and this was for free. We also have occasional actions like "remove this particular code smell from everywhere where you find it" and explicit "refactor that part" issues that are routinely assigned as part of sprints.

Obviously, we do weight how much time it takes to fix it vs how much of the debt it actually is. We would not had that much independence in these decisions if we did not.

[LandR]

This, this and this.

You get told to quickly write something as a Proof of Concept. Then get told, it works so put it in the code and reelase it. We will rewrite it later.

You never get to rewrite it later.

Over time the code base goes to hell. Every single time

[watwut]

I have hard time to believe you never refactor function you come across. I see how refactoring whole architecture or something major is avoided, but when it comes to smaller functions and pieces of code, refactoring it as you go takes around same amount of time as refactoring it during that detailed code review (e.g. often very little).

The really big mess tend to be emergent - when features and code base grew too much for original architecture over time. You cant prevent that one by detailed code review and it takes a lot of effort to fix it.

[LandR]

At my last job, if you had unecessary changes for the work item (e.g. refactoring) you failed the code review.

But then we had 5% unit test coverage on a 5million+ LOC code base, they were terrified that refactoring (with no tests to back it up) might break things.

We were writing medical software and they were very risk adverse (obviously), so if i refactored a method I would have to raise that and would mean test cases would have to be written and manually run to ensure I hadn't broken anything.

So refactoring hardly happened (The code was an absolute mess), when i left they were working on a project to fix it but the timescales for it were 5 to 10 years.

[lukevers]

Not exactly. In a lot of fast paced environments it’s rare to get a chance to rewrite something that functions properly just because it could be labeled as technical debt. It’s not about culture, it’s about moving on to the next thing. I code review hard to make sure we have the smallest amount of technical debt possible because it’s usually there to stay for a considerable amount of time.

[watwut]

That would fall under "culture".

[carlmr]

Exactly, just sounds like good review practices to me.