Comment by edw519 | Hacker News Reader

Comment by edw519 | Hacker News Reader

No real insight is offered here, just a list of "givens":

  1. Keep the changes small and focused
  2. Ensure logical coherence of changes
  3. Have automated tests, and track coverage
  4. Self-review changes before submitting for peer review
  5. Automate what can be automated
  6. Be positive, polite and respectful

Who could argue with these?

I've always believed there are 2 levels of code review, defined by their deltas:

1. Review against standards, defined by the delta between the code and shop standards, which should be well defined for everyone ahead of time. Things like formatting, white space, variable naming, conditionals, iterations, standard modules, etc. Every shop has (or should have) a standard way of doing a lot of these things. Much of this level of code review can be automated.

2. Review against expectations, defined by the delta between the code and the technical specs. Of course this requires technical specs which many shops don't do. If you write technical specs, and peer review the specs, agreeing that this is what is expected, then this level of code review becomes much more straight forward. This includes things like business rules, logic, data base updates, UX, interfaces and APIs with other apps, etc. This is much harder to automate, but offers a checklist to streamline the review and keep it objective. It also makes User Acceptance Testing easier down the line because code review has already eliminated a large subset of potential UAT rejections.

I'd love to hear how others handle these 2 levels of code review. Now that would be much more valuable than this list of feel good platitudes offered by the OP.

Hi, author of the post here.

Thank you for your feedback. The framework that you put down, of code reviews being of these 2 levels, is valid. The goal of this blog post, however, was to touch on the basic hygiene which makes the process more effective -- including both these two levels of review.

I would take a shot at writing a follow-up post to this, going deeper into the subjective aspects of peer code review. Thanks for reading!

> Who could argue with these?

Maybe not argue, but I still find this link useful. I will link it to more junior engineers in my team. Believe it or not, not everybody does self-review before they ask for peer review.

>> Believe it or not, not everybody does self-review

That's not hard to believe given the stream of shit that comes out of a feed, any feed.

But I'm appalled by y'all's assumption that #3 is a given. Why do you all believe that?

To me, including a test suite in your code base is a code smell.

Well depends, having 0 testing sucks too. I would argue 100% TDD can be a code smell, especially if you try to unit test everything. Use testing where it makes sense. Got some complex business logic in some function with several different edge cases? Cover them in test cases, it will also serve as documentation.

If you have a problem with #3 you might as well have the problem with #5. You don't want to automate everything, only where it makes sense.

>> Use testing where it makes sense.

I agree!

>> Got some complex business logic

So complex the only one who understands it is...who? A test?

i'm not sure what you're arguing here. if you have a complicated codebase you write tests so that when you add other code, you have a bit more confidence that you're not breaking things. furthermore, if a new person joins a team, they can use old tests to understand what the hell is happening in the code otherwise you just have an unadulterated mess of code to read through.

you're dealing with humans on a day to day basis that err very easily. the dumb mistakes i've made because there was a lack of tests are countless.

Why do you think having tests is a code smell? Sure, you can overdo it, but testing is useful and it's wasteful to have humans do it where it isn't necessary.

>> Why do you think having tests is a code smell?

Because it signals to me that the only one who understands this piece of business logic that we have applied to our code base is a test, which is a piece of code.

Why should I trust this piece of code but not this other piece? What pieces of code should I trust?

Me, I don't trust tests.

Do you trust any documentation at all? A test's just documentation a machine can read and, to some extent, validate. Other docs are necessarily even less trustworthy, if they can't be automatically checked for accuracy.

>>Do you trust any

The only test that matters to me is the one where a customer says "Yes, good!"

Sure, a test suite doesn't replace that. It helps ensure that test returns "Yes, good!", instead of "Why did you break this thing that worked last week!", assuming you know anything about what the customers consider good.

The parent sounds correct. One can have an extensive test suite and still turn out a product that is useless to the customer. These are 2 entirely separate concerns: testing is sanity checking that the code does what it is written to do, user validation is sanity checking that the code is actually useful to a customer (which most certainly cannot be automated or programmed).

> Because it signals to me that the only one who understands this piece of business logic that we have applied to our code base is a test, which is a piece of code.

That's a weird leap of logic. No, it does not mean that. It means that a human wo understood that piece of logic wrote a test, so the test can in the future warn about issues before a human spends time on checking if it still works like it should.

I disagree with the notion that this leap is weird.

>> No, it does not mean that. It means that a human wo understood that piece of logic wrote a test

I don't not trust a human to understand (1) a piece of logic nor to understand (2) a test written by someone who does not understand said piece of logic.

So how are you implementing code when you don't understand the logic of what it does? How do you make sure it does what it is supposed to do?

> Because it signals to me that the only one who understands this piece of business logic that we have applied to our code base is a test, which is a piece of code.

That...makes no sense. Tests do not understand rules, nor do they (or are they intended to) fully encapsulate them. They are intended to capture specific instances of application of the rules that have been verified with the person(s) defining expectations—the customer for acceptance tests or unit tests of key business functions, but possibly the team itself when the test is a unit test of an implementation detail.

The test isn't the only one that understands the ruler, it's the output of a process that validates that the test writers (who are humans on the team) understand the rule, and also concrete examples which aid anyone who comes.later in understanding the rule.

To me, not having defined test scenarios (whether automated and thus part of the code base or not) is a signal that the team probably doesn't (and almost definitely doesn't if there has been turnover so that the implementors of the functionality are no longer on the team) fully understand the intended function of the code.

> To me, including a test suite in your code base is a code smell.

You'd put it somewhere else, then?

My point is, you shouldn't need one.

- "But my code base is so big and complex!"

Make it smaller.

Some problems are irreducibly complex. There is no way to make the code smaller or simpler without losing essential functionality or performance.

This is so true. If the problem domain is so small that you can get away with no tests (only small programs or small teams can indeed get away with it, IMHO), then it cannot be a system of interest. For large systems (i.e. systems so large that any one human cannot fully grasp them), testing is required. Testing does not prove much about the system, it only encodes the assertions of the programmers about how it is meant to work. Testing is merely a harness that constrains the flight domain of the code, it does not guarantee that the code within the flight domain is correct, only that it presents a certain level of certainty about it.

Above some system size, the certainty of correctness is merely probabilistic, and automated testing is what gives you assurance. Programming without testing is like driving with a seatbelt: sure it is feasible, but it's insane to do it.

Tests aren't written to simply assert complex things behave as they should.

Tests are also used to enforce certain edge cases (e.g. return nil when the the input collection is empty instead of raising an exception), and in this regard allow you to assert stuff your language isn't able to express (because its type system is not as powerful as Haskell for instance).

Tests also allow you to avoid stupid mistakes like using a string instead of a symbol as well.

And anyway, tests are just a way to durably persist the testing that is always done by a programmer, be it in a REPL or with the few lines of codes at the end of a file used in the writing of code.

Yes, I agree completely with your #1 completely.

First thing before you do any sort of code review is that you NEED to have a well defined coding standard. This allows for rules of engagement. If you have no coding standard, then what happens is one person will say one thing, the other will say another, and the submitter is caught between two people giving contradictory comments based on opinion, not fact.

Having a single set of rules of engagement levels the playing field completely and makes it easier for everyone.

People love to write write write code and don't give a shit about size or how it impacts reviewers.

These are not givens. They're things that have to be drilled into newer devs.

Reply on news.ycombinator.com