
Ask HN: Why code “post-reviews” isn't a thing? - pacavaca
Regular code-reviews are supposed to catch errors, bad style, or bad architectural decisions early on in the process. For it to work, however, everyone should be super disciplined and spend a lot of brain-power reading through each diff. Usually, it&#x27;s really hard to dig into someone&#x27;s code until you&#x27;re fully int the cotext (read: working on the same piece of code). On the other hand, I often find myself in the situation when I&#x27;m dealing with someone else&#x27;s code and think, &quot;Oh god, how could I even approve this PR?&quot;. The next thing I usually want to do is let the team, and specifically, the code author, know that this is how we should NOT DO things. The problem, however, is that the code-review window has closed and there&#x27;s no other standard place (besides GitHub PRs) to comment on the code.<p>I&#x27;m wondering if commenting on the code AFTER it has been merged is standard practice on some project and if there are any good tools for that.
======
byoung2
Why can't you say "this is how we should not do things" during the code review
window? You should have a policy in place that all comments need to be
addressed before approval, and with your comment in there (e.g. "we should not
hardcode api keys" or "pull default params from the config file, not ENV
variables"), others should not approve. Your CI process should require
approval before merging. Now if it makes it past review and approval and gets
merged, you can create a "tech debt" issue to refactor. You should also have a
meeting of the minds to put these standards in a style guide and make sure it
is followed when reviewing PRs.

~~~
pacavaca
Yeah, but the problems is that I (and I'm sure many other people) just can't
pay enough attention to every detail DURING the code review. Yet after the
review, when you start using the code, issues just reveal themselves without
any extra effort. I'm just looking for ways to prevent them from reproducing I
think.

------
cjbprime
Just file a new bugtracker ticket or whatever that says "Clean up foo.js, it
does Bad Thing A and Bad Thing B"?

There are processes for sustaining code architecture reviews and paying back
tech debt, but if you're in a team that's merging obviously terrible code you
might not be large enough to want that kind of structure.

~~~
pacavaca
I've probably over-exaggerated a bit how "terrible" our merged code is but
anyway, my point is that at the "review" phase the code is still "imaginary".
We think that it'll work, but it hasn't been integrated with anything yet, it
hasn't been battle-tested or, often, even used. Obviously, we find a lot of
issues after the merge and filing tickets allows us only "not to forget to fix
them". I feel like there should be a standard practice aimed at preventing
those issues from RE-occurring.

------
vegetablepotpie
I have the same problem at my job and a very similar process (biggest
difference is that we require unanimous consent before merges). My overall
opinion towards code reviews is that they don’t work, software should not be
seen as a static entity that is released and completed (like a building), but
as something that requires constant upkeep (like an garden). Since our process
was established 20 years ago (while I was in elementary school) time has been
cemented it by bureaucratic inertia. So my approach is not what you can do to
your process, but what you can do as an individual within your existing
process.

The idea behind my approach is to convince the person to do the right thing on
their own. First step is to mentally prepare, as if you were an actor
preparing for a roll as a villain. Your attitude should be “any code not
written by me is garbage, and should be completely rewritten.” This attitude
(which you should not adopt in your general professional life) will help you
get through this ordeal. Next step is to poke as many holes in the code under
review as you can in the time you have. Don’t stop on the diffs, do it for the
whole code base. If the person touched the code they are responsible for the
whole thing. Send as many issues as you can back, cc their management. This
will make some people angry, but it will be temporary and most importantly buy
you time.

Management will be aware that their are issues with the merge, but they don’t
have the time to review your comments and understand the issues, they have
other problems. They don’t want doubt in the work going out and will do
anything to make themselves feel confident. They will put their trust in you
and will not allow the merge until you say it’s good. You are the gate keeper,
you hold the key.

Now it’s time for the real code review. The person having the review now
depends on you. This is where you get as friendly as possible. Complement
sandwich works here “I like your code, there’s just a few things that need to
be changed, this looks like it’s going to be a slam dunk, we’re almost there,
good job so far buddy.” The most important thing now is to identify specific
issues that have bounded solutions (not wild goose chase ordeals). The more
specific and more bounded, the requests are, the happier people will be and
the faster people can move on.

The whole idea here is to develop an understanding with people. You’re going
to tell people what you expect, and make it as easy as you can to carry it out
without significant impacts to their schedule. As soon as they deliver on your
requests, you approve. You go to management and say the code looks a lot
better and you approved the peer review. Everyone looks good, everyone is a
winner here.

~~~
pacavaca
Wow, thanks for the extended answer! I liked the part about "any code not
written by me is a garbage", although I'm mostly trying to drop this attitude
:D Jokes aside, I got your point, but it's still about catching errors
upfront, during a more tedious code-review.

