As others have said, there's a difference between API-documenting comments and in-line comments. From my perspective, the main difference is that an in-line comment is read alongside the code, whereas a documentation comment is read alongside the signature of whatever you're commenting.
Given this, we can apply the same DRY principle. Is the comment saying the same thing as the signature? Let's look at your example:
Retrieves a product by its id
This part just repeats what we already see from the name GetById plus the return type Product. It also raises the question what the difference between "getting" and "retrieving" is, and what bearing code vs. comment has on that distinction. So it's needless and slightly confusing. If anything, it's getting in the way of the actually useful, second part of the comment:
returns null if no product was found.
Ah! That's something we definitely can't know for sure just from the signature, and provides useful information.
Now take this a step further. When people talk about comments as code smells, the question isn't whether the code as it is needs a comment, but whether the comment indicates that the code could be written better, to express the information in the comment. That's what "code smell" means- it doesn't mean "don't do this!", it means "if you're doing this, it could be a sign there's a problem".
So if your colleagues tell you this comment about null is a code smell, you should simply ask them: "Okay, how should I express this then?" If they have a feasible answer, you've learned something. If not, it'll probably kill their complaints dead.
Regarding this specific case, generally the null issue is well known to be a difficult one. There's a reason code bases are littered with guard clauses, why null checks are a popular precondition for code contracts, why the existence of null has been called a "billion-dollar mistake". There aren't that many viable options. One popular one, though, found in C# is the Try... convention:
public bool TryGetById(int productId, out Product product);
In other languages, it may be idiomatic to use a type (often called something like Optional or Maybe) to indicate a result that may or may not be there:
public Optional<Product> GetById(int productId);
So in a way, this anti-comment stance has gotten us somewhere: we've at least thought about whether this comment represents a smell, and what alternatives might exist for us.
Whether we should actually prefer these over the original signature is a whole other debate, but we at least have options for expressing through code rather than comments what happens when no product is found. You should discuss with your colleagues which of these options they think is better and why, and hopefully help move on beyond blanket dogmatic statements about comments.
Taking the code examples first. You favour:
if (isApplicationInProduction(headers)) {
phoneNumber = headers.resourceId;
} else {
phoneNumber = DEV_PHONE_NUMBER;
}
function isApplicationInProduction(headers) {
return _.has(headers, 'resourceId');
}
And your boss would write it as:
// Take the right resourceId if application is in production
phoneNumber = headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER;
In my view, both have problems. As I read your code, my immediate thought was "you can replace that if with a ternary expression". Then I read your boss' code and thought "why's he replaced your function with a comment?".
I'd suggest the optimal code is between the two:
phoneNumber = isApplicationInProduction(headers) ? headers.resourceId : DEV_PHONE_NUMBER;
function isApplicationInProduction(headers) {
return _.has(headers, 'resourceId');
}
That gives you the best of both worlds: a simplified test expression and the comment is replaced with testable code.
Regarding your boss' views on code design though:
Writing small functions is a pain because it forces you to move into each small functions to see what the code is doing.
If the function is well-named, this isn't the case.
isApplicationInProduction is self-evident and it should not be necessary to examine the code to see what it does. In fact the opposite is true: examining the code reveals less as to the intention than the function name does (which is why your boss has to resort to comments).
Put everything in a main big loop even if the main loop is more than 300 lines, it is faster to read
It may be faster to scan through, but to truly "read" the code, you need to be able to effectively execute it in your head. That's easy with small functions and is really, really hard with methods that are 100's of lines long.
Write only small functions if you have to duplicate code
I disagree. As your code example shows, small, well-named functions improve readability of code and should be used whenever eg you aren't interested in the "how", only the "what" of a piece of functionality.
Don't write a function with the name of the comment, put your complex line of code (3-4 lines) with a comment above. Like this you can modify the failing code directly
I really can't understand the reasoning behind this one, assuming it really is serious. It's the sort of thing I'd expect to see written in parody by The Expert Beginner twitter account. Comments have a fundamental flaw: they aren't compiled/interpreted and so can't be unit tested. The code gets modified and the comment gets left alone and you end up not knowing which is right.
Writing self-documenting code is hard, and supplementary docs (even in the form of comments) are sometimes needed. But "Uncle Bob"'s view that comments are a coding failure holds true all too often.
Get your boss to read the Clean Code book and try to resist making your code less readable just to satisfy him. Ultimately though, if you can't persuade him to change, you have to either fall in line or find a new boss that can code better.
Best Answer
Good answer here already, but let me say a word about your butterknife example: though I have no idea what the code does, at a first glance, it does not look really unmaintainable to me. Variables and method names seem to be chosen deliberately, the code is properly indented and formatted, it has some comments and the long methods at least show some block structure.
Yes, it does in no way follow Uncle Bob's "clean code" rules, and some of the methods are sure too long (probably the whole class). But looking at the code I still see enough structure so that they could be easily "cleaned up" by extracting those blocks into methods on their own (with a low risk of introducing bugs when using refactoring tools).
The real problem with such code is, adding one block and another block and another block works to some degree, sometimes over years. But every day the code gets harder to evolve a little bit, and it takes a little bit longer to modify and test it. And when you really have to change something which cannot be solved by "adding another block", but requires restructuring, then you will wish someone had started to clean up the code more early.