New answers tagged

2

In the places where I've been doing code review, I would accept and OK the code submission, if it comes with a commitment to do (at least) some refactoring. Be that as a filed bug, as a story, or a promise to send another review with (some) refactoring done. In these cases, if I am the person writing the code to review, I usually prepare two changes, one ...


4

Your main problem is that a code review on a significant new feature is the wrong time to have this discussion. At that point it's too late to make anything but minor changes. The right place is in the planning stages, or in a preliminary design review at the latest. If your company isn't doing those early reviews at least informally, you should work on ...


3

If you are doing code reviews as part of your development process; then you must set the rules against which you 'judge' the code you are reviewing. This should go into your 'definition of done' and could be a style guide, architecture document for the codebase or static analysis, checking for legal requirements, whatever the company decides is its ...


23

Simple: 1. Document your technical debt You've identified a piece of code that works but has some technical issues with it. This is technical debt. Like other kinds of debt it gets worse over time if not dealt with. This first step in dealing with technical debt is to document it. Do this by adding items in your issue tracker which describe the debt. ...


5

As a side note: search for a new job. This one wouldn't get any better. The goals of the code you are reviewing are: To ship a feature, which should work according to the requirements. To reduce the growth of technical debt. The first goal is reviewed by checking that the unit, integration, system and functional tests are here, that they are relevant, ...


0

As @EricLippert points out in his excellent answer, this kind of change needs more attention, not less. If you realise a change you are working on is going to become such a change, there are a few strategies that might help: Commit to version control frequently. Review can progress on a commit-by-commit basis, and might be more understandable when you ...


0

Do the best one can and try only to spot any obvious flaws (perhaps this is the most code review should aim for anyway)? Don't underestimate the potential value of code revues. They can be good at detecting bugs: Find bugs that would be difficult to detect though testing Find bugs that would be difficult to identify/fix though testing They're also ...


1

I don't know why it hasn't been mentioned yet, but these 2 are the most important pieces: You split up the changelist into multiple smaller changelists, which you then review one after another.* If the review of a changelist doesn't result in a decision that the changelist seems to be good, you obviously reject the change. *Example: You replace library ...


0

Plenty of code is written and merged in without proper code review. It can work. There's a reason why it's called code smell not "broken code" or something to that effect. The lack of code review is a warning sign, not a harbinger of doom. The solution to this problem is that there is no one solution to fit all cases that we can pack into a StackExchange ...


-1

Like a multiplication, the code review gives zero result when applied to zero. It does not increase the value in such case, while in most other cases it would. The code you need to work with is too badly designed to benefit from the code review process during further development. Use the code review process to refactor or re-develop it. It may also be that ...


29

Welcome to the world of legacy software development. You have 100s of thousands, millions, 10s of millions of lines of code. These lines of code are valuable, in that they produce a revenue stream and replacing them is infeasiable. Your business model is based off of leveraging that code base. So your team is small, the code base is large. Adding ...


0

More answers are addressing how you got to this point. Many of them give some suggestions to remedy the situation, but I'd like to throw my answer in to give the short answer. What to do when code reviews are "too hard?" Go back to the main line code branch Write tests for the functionality you've refactored (e.g. functional tests) Get the tests to pass ...


0

Changes to the tests should be part of the same review as changes to the system under test. If they can't be, for organisational reasons (e.g. you're using a tool that works on only one repository at a time, and your tests are not in the same repository as your functional code), then the two reviews should be linked in some way. This might be as simple as ...


3

If you're not content to ship with buggy/non-functioning software and fix it later, then V&V effort SHOULD be longer than development effort! If existing code is fragile, then a first question is "should you even be changing it?" Management need to make a call on whether the cost/risk of redesigning and reimplementing this code is greater than the cost/...


14

In this situation, the amount of time it would take to verify the safety of the changes, absence of regression, etc. is excessive. Code reviews shouldn't be primarily aimed at correctness. They are here to improve code readability, maintainability and adherence to team standards. Finding correctness bugs during a code review is a nice byproduct of ...


11

If you think that the code review is too hard, because it changed brittle code that is near impossible to change without breaking it, then you have a problem. But the problem is not with the code review. The problem is also not with unit tests, because brittle code cannot be unit tested! If your code was unit testable then it would have been split up into ...


1

From my experience, I would strongly recommend you to cover your code with a fair amount of tests, both unit and integration, BEFORE any changes are made to the system in question. It's important to remember that nowadays there's a very good number of tools for that purpose, doesn't matter the language you're developing with. Also, there's THE tool of all ...


276

The premise of the question is, frankly, astounding. We suppose that there is a large change to fragile, complex code, and that there is simply not enough time to review it properly. This is the very last code you should be spending less time on reviewing! This question indicates that you have structural problems not only in your code itself, but in your ...


11

You can send the code review back and tell the developer to break it up into smaller, more incremental changesets, and submit a smaller code review. You can still check for code smells, patterns and anti-patterns, code formatting standards, SOLID principles, etc. without necessarily going through every detail of the code. You can still perform tactical code ...


24

Solve the larger problems that are causing code review to be too hard. The ones that I've spotted so far: No unit test suite Complex code merges that could be avoided by more sensible code structure and delegation of coding duties An apparent lack of rudimentary architecture


3

Unfortunately, there's not really much you can do about this at the point of code review other than get another cup of coffee. The actual solution for this issue is to address the technical debt you've accumulated: fragile design, lack of tests. Hopefully, you at least have some sort of functional QA. If you don't have that then there's always praying ...


91

One of the primary goal of a code review is to increase quality and deliver robust code. Robust, because 4 eyes usually spot more problems than 2. And the reviewer who has not written the additional code is more likely to challenge (potentially wrong) assumptions. Avoiding peer reviews would in your case only contribute to increase fragility of your code....



Top 50 recent answers are included