People who code: we want your input. Take the Survey
326

I can give you an example of a corner case that could never occur that caused a disaster. When the Ariane 4 was being developed the values from the lateral accelerometers were scaled to fit into a 16-bit signed integer and because the maximum possible output of the accelerometers, when scaled, could never exceed exceed 32767 and the minimum could never fall ...


308

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 ...


279

Ask for a test case that fails without the change that succeeds with the change. If he can't produce one, you use that as justification. If he can produce one then you need to explain why the test is invalid.


247

If it cannot be reviewed, it cannot pass review. You have to understand that code review isn't for finding bugs. That's what QA is for. Code review is to ensure that future maintenance of the code is possible. If you can't even follow the code now, how can you in six months when you're assigned to do feature enhancements and/or bug fixes? Finding bugs right ...


228

an obscure case that is extremely unlikely to happen--in fact I'm not sure it is even possible to occur Not having untested behaviors in code can be very important. If a piece of code is run e.g. 50 times a second, a one in a million chance will happen approximately every 5.5 hours of runtime. (In your case, the odds seem lower.) You may talk about the ...


179

How can I justify and defend the thesis that: The merge button should be enabled by default The code review should be a recommendation , but not mandatory The code author should have the right to merge the code within 6 hours lets say of the pull request creation no matter if there is aproval or not. I don't think you should try and justify any of those, ...


152

Reviewers should be objective. It's clear that you've formed an opinion about the code in question before you've even reviewed it, and it sounds like you and the fixer have staked out positions. If that's so, then you're going to have a difficult time appearing objective, and an even more difficult time being objective. None of that helps the process, and ...


139

As a reviewer, your job is to check if a piece of code (or a document) meets certain objectives that have been agreed upon before the review. Some of these objectives will typically involve a judgement call whether the objective has been fulfilled or not. For example, the objective that code must be maintainable typically requires a judgement call. As a ...


126

How to find positive things in a code review? After some serious quality problems in the last year, my company has recently introduced code reviews. Great, you have a real opportunity to create value for your firm. After the first few weeks my colleague started to let things slide, to not cause trouble with the co-workers (she told me herself, that after a ...


120

Any sense? Yes. Even if you know nothing about the semantics of a programming language, you can still read characters and notice inconsistent formatting, missing comments, badly chosen identifiers, obvious duplication etc. Much sense, or enough sense to repay the cost of your time? I'm not sure. This depends on your position, the importance of code reviews ...


106

Don't bother picking something good unless its a solid concise example and is directly related to the focused issue. I won't sugar coat it - from the sounds of it you are dealing with at least one person who is insecure with their abilities and is handling being challenged about their work in an immature way. They are also likely bad at their job - a good ...


103

Feature envy is a term used to describe a situation in which one object gets at the fields of another object in order to perform some sort of computation or make a decision, rather than asking the object to do the computation itself. As a trivial example, consider a class representing a rectangle. The user of the rectangle may need to know its area. The ...


99

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....


96

Code reviews can be toxic, time wasting, will to live-sapping nerd wars. Just look at the divergence of opinion on things like clean code vs comments, naming conventions, unit and integration testing, check in strategies, RESTfulness, etc., etc. The only way to ensure you avoid this is to write down the rules for passing a code review. Then it's not a ...


86

Since it wasn't handled before, it's out of scope for your effort. You or your colleague can ask your manager if it's worth the effort to cover this case.


84

There are several questions that you raise. 1) Is this a clear sign that the coder is not cut out for professional programming? No. Developers often go through stages where they learn about an idea and want to apply it. Do they always apply these ideas efficiently and/or effectively. No. Mistakes are made, and it is part of the learning process. If ...


78

There are several relevant trade-offs here: Review complexity. If a branch has more than one functional change commit or more than one refactoring commit it becomes time-consuming to review the result, since now each commit has to be reviewed separately. Risk. Any refactoring, no matter how well the code is tested, has some non-zero risk of breaking things. ...


75

There are multiple reasons why you would want to conduct a code review: Education of other developers. Ensure that everyone sees the modification associated with a defect fix or enhancement so that they can understand the rest of the software. This is especially useful when people are working on components that need to be integrated or on complex systems ...


73

The problem with this approach is that while exceptions never get thrown (and thus, the application never crashes due to uncaught exceptions), the results returned are not necessarily correct, and the user may never know that there is a problem with the data (or what that problem is and how to correct it). In order for the results to be correct and ...


72

I'm going to offer a different take from my fellow answerers. They are right - be involved if you want to see how things go. If you want more tracability, there are tools for that. But in my experience, I suspect that there's something else going on. Have you considered that your team may feel that the process is broken/stupid/ineffective for most commits? ...


70

So my code is late too. No, it is not your code, it is the code of you and the senior. You are working as a team, you have a shared responsibility, and when you two miss a deadline, it is the fault of both of you. So make sure the one who makes the deadlines notices that. If that person sees that as a problem, too, he will surely talk to both of you ...


68

GitHub allows for PR to be in a "draft" state. Your team can see the differences, and even comment on it, but it's still obviously a work-in-progress, and cannot be merged until you click a "ready for review" button, which makes it mergeable. I'd also say that if it's a work-in-progress, give them a clue as to what you want them to focus ...


67

are there any inherent problems or considerations with raising a ticket off of the back of a review, instead of failing it? Not inherently. For example, the implementation of the current change may have unearthed a problem which was already there, but wasn't known/apparent until now. Failing the ticket would be unfair as you'd fail it for something ...


63

This seems to be a pretty common prevailing attitude among some developers. Everyone seems to feel that a code review is some challenge to their work, and that makes no sense to me. A code review is a quality assurance mechanism that has the added bonus of education to go along with it. We implement code reviews extensively where I work, and I've fostered ...


57

As a regular contributor over at Code Review Stack Exchange, I encounter a lot of questions suffering from Language-agnostic issues, for example: Formatting, indentation Scope Loops Type operations and the list goes on. However, while I don't need to know the language, I can still review those issues / points. A few of our top users have top answers in ...


57

IMO the best documentation is the documentation you don't actually need. I also hate writing documentation and comments. With that being said: Pick readable and talking names. Don't use n, but instead numberOfItemsFound for example. Don't shy back from storing parts of a calculation in a constant variable rather than pushing everything into one line. Move ...


54

Inconsistencies make you stop and think why, which and where: When you read part of the code and see that it uses a different style from the rest of the code, it makes you wonder - why is this particular part different? Does it have a special reason that I need to be aware of? This is dangerous, because if there really is a reason for that part to be ...


53

With complex algorithms, it's very difficult to prove you have thought of every test case that will come up in the real world. When you intentionally leave a test case broken because it won't come up in the real world, you are potentially leaving other test cases broken that you haven't even thought of yet. The other effect that often happens is when you ...


52

It's important to highlight positives as well as negatives. I know if I were reviewing the refactor of a particular hellish subsystem into something neat and clean, I'd probably buy the programmer a pizza for his efforts. If you're using reviews as training, it's doubly important - highlighting a good piece of code will be helpful for the junior programmers ...


Only top voted, non community-wiki answers of a minimum length are eligible