4

I see a lot of advice lately about making code reviews more granular, and as small as possible. For example, a lot of recommendations say "no more than 400 lines of code for a code review". This sounds like fine, idealistic advice, but I have run into situations where I don't see this as practical.

For example, say you're developing a new application. It easily can get to 3k-5k lines of code before you have anything intelligible and it still may not even work at that point. If you code review in smaller/more granular steps, you might have people reviewing things that will change heavily because you're fleshing out the design as you code, so they are reviewing things that you didn't necessarily need their input on yet.

To be fair, I tried this and a lot of the questions would revolve around why I'm introducing a certain class, because it wasn't clear without seeing where it was used (even with a unit test). A good portion of the time, I'd wind up reworking how that class worked, removing it or performing other refactorings at a later step that invalidated the initial purpose of the earlier review.

I think overall, the recommendations don't seem to fit all cases and seem like a poor use of resources. Batching seems like a more reasonable approach to me, but obviously this has the downside of large code reviews.

Please let me know the best practices for handling these sorts of situations.

10
  • 3
    Why does it take 3K to 5K lines of code to develop something substantial in your shop? We have entire subsystems that are that size. Commented Aug 8, 2016 at 15:17
  • Well, developing in C++, so it can get pretty verbose. Oftentimes, adding some reusable classes because there are gaps in the standard libraries that are not issues nearly as often in other programming languages. Also, consider working on applications that are x00,000 - x,000,000 lines of code. I agree this is not an issue with applications or subsystems that have a small scope.
    – Lawrence S
    Commented Aug 8, 2016 at 15:19
  • 2
    I guess it kinda depends on what your goal is in code review. Are you reviewing code or design? It is clearly possible to review single classes in isolation, and if your classes are approaching 5K lines of code in size, maybe it's time to start subdividing responsibilities. Commented Aug 8, 2016 at 15:21
  • 2
    If you have a very disciplined team, your code always follows some definition of best practices, and you have automated tools like linters that enforce some or all of those guidelines, it's entirely possible that you don't need code reviews at all. It sounds to me like your team sees code reviews as design reviews, but if you wait until the application is substantially written to perform such a review, it's probably too late. Commented Aug 8, 2016 at 15:31
  • 2
    If you need to write 3k to 5K lines of code until your design stabilizes, you should probably switch to more code reviews - ideally pair programming, to have always someone at hand to discuss the design.
    – Doc Brown
    Commented Aug 8, 2016 at 21:00

1 Answer 1

2

From personal experience, I could tell you how I usually manage my code resp. the code of our team, when I am in charge.

Recently we finished a big application for the public sector, gathering data from participants (employment status, marital status, etc.) in projects. The next part of the project was developing a reporting application, which does reports on cummulative data. Overall it could reach 2k LOC, what makes it comparable to your (typical) projects.

We were two developers in charge of that application, me beeing the "leading" one.

For this project I set several goals:

1) Produce fast visible results - emphasis on the keywords »fast« and »visible« in exactly this order.

In order to be fast you have to make up things in a quick and dirty way. E.g.: I made up a HTML mockup of a final result, including select boxes, date selectors, and copied a bloody hell of HTML together from similar applications.

2) Produce only a minimal working result for a given Iteration. As I said, for a first draft, there was only awfully hacked together html and a "base"-application serving the html.

Next iterations were step by step introducing dynamics: Autofill selectboxes, partially read data from DB and display on page etc.

3) After this "dirty" phase, you have to have the discipline and rework your code: refactor methods, take care of cohesion in your classes, write tests. This only works if you have enough discipline and know how to shape your code.

This is not for freshmen, since you have to have some knowledge and discipline to clean up afterwards. (Did I mention discipline? ;))

For example, say you're developing a new application. It easily can get to 3k-5k lines of code before you have anything intelligible and it still may not even work at that point.

From what I said above, it is absolutely possible to have less than 1k LOC and get an impression of a "working" application. It depends on your style of development. If you have problems reviewing code at an earlier stage, you should try to make clear tasks for each reviewing stage. Try to make your code produce something. Results are testable - testability is the premise of a code review. Code which is testable is reviewable.

If you code review in smaller/more granular steps, you might have people reviewing things that will change heavily because you're fleshing out the design as you code, so they are reviewing things that you didn't necessarily need their input on yet.

This involves two problems:

When to review what?

It doesn't make sense to review a mock like the production ready version. According to my example above in Phase (1) and (2) it doesn't make sense to do any code review. I heavily worked in (3) on the code.

But during (1) and (2) we did some kind of review:

  • Is the code performant enough ?

As rotten as the code was, the pillars were set, so, that we could do some testing with a dataset bigger than production size. How long does the code take to do CSV-exports of several hundreds of thousands of entries? This lead to dropping some architectural decisions in favour of others.

  • How does the application look at this early stage.

These are questions, which could be answered in (1) and (2).

Code reviews are only meaningful, when the code is in shape. The faster you get results from (1) and (2), you could go on to (3) and stabilize your code. If (3) is done, then there should be room for a code review.

To be fair, I tried this and a lot of the questions would revolve around why I'm introducing a certain class, because it wasn't clear without seeing where it was used (even with a unit test). A good portion of the time, I'd wind up reworking how that class worked, removing it or performing other refactorings at a later step that invalidated the initial purpose of the earlier review.

The problem here is clear: the code/team is not focussed (enough).

It should be clear, what every part of your application should do in a certain stage of the development process. What I see from what you wrote is, that you spent a lot of time and energy in engineering for a use case which is too abstract. You should focus more on simple understandable results.

Your problem is not reviewing code per se, your problem is lack of focus, which makes your code unspecific and unreviewable.

1
  • 4
    ++ You should never be writing thousands of lines of code before having some small part of the system working.
    – RubberDuck
    Commented Aug 9, 2016 at 0:18

Not the answer you're looking for? Browse other questions tagged or ask your own question.