Tag Info

Hot answers tagged

268

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.


178

Code with spelling and grammar errors is unmaintainable. People won't remember the bad grammar, so they'll try to call the function as it should have been written, and that's how bugs happen. You can't grep for something in the code if you don't know how it's spelled. Most people who make grammar/spellings do so inconsistently, so they'll introduce many ...


170

The truth is that probably in 2 years when you will see your current code you will agree that it was a mess. Learning programming is a never ending process and there will always be someone who is better at it than you. So if person who said that your code is a mess is not just mean and it is not another case of "I would do it better" disease common among ...


147

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


142

I've found that people who don't like code reviews will do their best to work around whatever you put in place. The best way to make sure that the code you work with is code reviewed properly is to work somewhere that treats that as the normal way of coding, and that only hires developers who are likely to fit into that environment well. If you can't ...


131

Double-check your motivation. If you think the code should be changed, you ought to be able to articulate some reason why you think it should be changed. And that reason should be more concrete than "I would have done it differently" or "it's ugly." If you can't point to some benefit that comes from your proposed change, then there's not much point in ...


108

The first rule of any professional software engineer is to write code that is comprehensible. The second example looks like an optimized example for an older, non-optimizing compiler or just someone who happens to want to express themselves with bitwise operators. It's pretty clear what's going on if we are familiar with bitwise operations but unless you're ...


102

I personally think that every piece of code should go through a code review, it doesn't matter if you are junior or senior developer. Why? For starters your title doesn't state anything about how you develop, and a senior developer could learn something from the junior. At our company we shift around so one of the other members of the team review your ...


85

First of all, make use of tools to check as much as you can. Tests (backed up with some reasonable code coverage) will give you some confidence of the correctness of the code. Static analysis tools can catch a lot of best practice things. There will always be issues that you need human eyes on to determine though and you will never do as good a job reviewing ...


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


81

I agree with your code reviewers, but with an asterisk. Each statement that you write in your code is a technical liability -- it's a potential failure point. If you write a method with 10 statements and your coworker writes one that achieves the same functionality with 5 statements, his is likely to be 'better' as measured by likelihood of issues (there are ...


74

Ask him to explain his code to you Tell him you've never seen X programmed that way before, and ask him why he codes it that way. Show him the way you code it, and tell why you do it that way (best practices, better performance, less chance of errors, easier for other programmers to read/maintain, etc). Be sure to prepare all your arguments in advance, ...


72

Should junior programmers be involved as code reviewers in the projects of senior programmers? Yes they should. It is a good learning experience to read other peoples' code. (And that applies both for good code and bad. Though one would hope that a senior developer's code wouldn't be bad ...) Obviously, it is unwise to only have juniors doing the ...


68

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


67

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


66

Don't take pride in how well you code. Take pride in how well you learn. Then learning that your code needs improvement provides you with an opportunity to demonstrate how good you are at learning, instead of coming across as criticism of how bad a programmer you are. Read http://www.perlmonks.org/?node_id=270083 if you have no idea what I'm talking ...


65

The problem with measurements, no matter how well intended they are, is the very act of measuring the item makes it important, and the corollary, the act of not measuring an item makes it unimportant. It is absolutely essential to measure what is important, and not measure what is unimportant. Measuring SLOC (Which is effectively what your reviews are ...


64

Unless you will be making changes to this "old code" to fix bugs or add features, I wouldn't bother improving it just for the sake of it. If you do want to eventually improve it, make sure you have unit tests in place that will test the code before you start refactoring it. You can use the "old code" to learn better practices. If you do this, it's a ...


62

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


54

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


51

Code reviews are a great practice. It is probably the best way to learn from mistakes and to see how certain problems are solved by others. It is also one of the best way to maintain quality in a code base. Code reviews happen in many companies, though it is difficult to say that there is a specific process that they all follow. In more formal code ...


51

Since it's not clear from your question, I just want to point out that a gatekeeper workflow is by no means required with git. It's popular with open source projects because of the large number of untrusted contributors, but doesn't make as much sense within an organization. You have the option to give everyone push access if you want. What people are ...


51

The primary purpose of a code review is to find defects or potential problems. The required participants in the review should be the people who are best suited to identify these problems, regardless of their title or seniority. As an example, if an application is being developed in Python and the junior engineer has more experience with the Python language ...


50

Take a look into the Code Review Stack Exchange site. It is for sharing code from projects you are working on for peer review: Code Review Stack Exchange is a question and answer site for seeking peer review of your code. We're working together to improve the skills of programmers worldwide by taking working code and making it better. If you are ...


50

Like Simon Whitehead mentions in his comment, it depends on your branching strategy. If the developers have their own private branch for development (which I'd recommend in most situations anyway), I'd perform the code review prior to merging with the trunk or main repository. This will allow developers to have the freedom to check in as frequently as they ...


50

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


47

Code Transfer Advantage Following patterns provided by a library, React in your case, means that the product you deliver will be easily picked up and maintained by other developers who are also familiar with React. Potential Backward Compatibility Issues Some libraries would have a new major version out, and backward compatibility might be compromised if ...


46

Make a good impression Take some of the well-known books, e.g. Clean Code, Code Complete, Coders at Work, The Clean Coder: A Code of Conduct for Professional Programmers etc. (see here for full lists) and give them a couple of days to read one - at work, in a private office. Space those out, say 1 book a month or a quarter. They will see from the ...


45

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


44

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



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