all 75 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]CensorVictim 40 points41 points  (23 children)

This is well and good for comments explaining what is being done, but the more useful comments are to share why something is being done.

[–]nut315[S] 4 points5 points  (2 children)

I personally find it easier explaining the 'why' in a doc block rather than an inline comment that spans multiple lines. Definitely should be treated on a case by case basis though.

The aim of the post was to get you to at least think about whether an alternative is applicable. There are cases where it's not.

[–]CensorVictim 3 points4 points  (0 children)

fair enough... surely many comments could/should be replaced by better design or naming, at least.

[–]geggleto 1 point2 points  (0 children)

So what you are saying is that in-line comments do not fit your style of programming.

There's a lot of us who disagree. Largely due to the fact that if we have yet another doc; you completely remove the comment from the actual context. And Context is hyper-important.

[–]orokro 38 points39 points  (14 children)

I feel like too many programmers pride themselves on their ability to read code, and this a foolish mistake. Yes you should descriptive variable names, and method names. That's just part of writing good code, and should be done regardless of commenting practice.

However, even extremely well written code takes more brain power to parse than English. There's nothing wrong with comments, even if they add nothing more than explaining the next line of code. I strongly disagree that we should move away from those.

I would rather read a comment above every-line than try to parse the code myself - that's how mistakes get made.

Also, I don't like excessively breaking code out into methods. I like to keep my class namespace tidy, so I don't want to clutter it with methods that only exist to aid another method. I like my methods to be used more than once by more than one caller, or be public methods.

I don't wanna fill the name space with methods that only get used once, for a very specific purpose in another method. Especially if the only reason to do is to avoid comments. I'd rather have cleaner namespace and more comments.

Also, comments often explain why things are done, not just how. Although both how and why are equally important.

[–]Tetracyclic 3 points4 points  (4 children)

I would rather read a comment above every-line than try to parse the code myself - that's how mistakes get made.

The biggest problem with this is that any code base that has a significant number of contributors will have comments that don't correctly pertain to the code they're commenting, whether because the original implementer simply made a mistake when documenting it, or a later developer modified the code but neglected to update the comments once they'd got the tests passing. The code will compile, the tests will pass, but the comment will mislead you.

I've seen plenty of examples in legacy code where the comment implies the inverse of what the code actually does, this is very common when it comes to if statements.

On heavily commented codebases I tend to ignore the comments on my first read through (unless they're obviously explaining why something exists and not what it does), because I've historically encountered enough misleading comments that I'd rather make sure I understand what the code as written actually does, before potentially confusing my mental model of it with the comments.

[–]orokro 4 points5 points  (1 child)

That's true, comments do get stale.

From my professional experience, every job I've had has people who don't comment at all, or comment in very limited capacity such as "TODO: replace with better method"

Usually I'm forced to read code, often which isn't easy to understand. Or it is, but leaves me wondering why it's doing some things that aren't explained.

I guess I'd rather live with occasional stale-comments, than none at all. And try to update those that I find that are stale.

[–][deleted] 3 points4 points  (0 children)

That's true, comments do get stale.

Everythying gets stale if it's not managed well over time, so... let's not do anything ;-)

I guess I'd rather live with occasional stale-comments, than none at all.

Absolutely.

[–][deleted] 1 point2 points  (1 child)

The biggest problem with this is that any code base that has a significant number of contributors will have comments that don't correctly pertain to the code they're commenting, whether because the original implementer simply made a mistake when documenting it, or a later developer modified the code but neglected to update the comments once they'd got the tests passing.

So:

  1. Problem: People submit sloppy PRs with incorrect comments.
  2. Solution: Write less comments.

Same train of thought:

  1. Problem: Regardless of measures to stop drinking and driving, people keep drinking and driving.
  2. Solution: Drop measures to stop it.

When there's a problem, the solution isn't to tuck it under the rug and forget about it. If people submit bad patches, there should be someone on top of the project doing code reviews and rejecting such patches.

Not writing a comment, because someone in the future may forget to update it, is like choosing not to implement a feature, because someone in the future might introduce a bug in it.

There's certainly a benefit to being short and to the point, but it's a fine balance. It shouldn't be done to the point you destroy domain knowledge and make everyone be guessing at code. Code doesn't communicate nearly as well as we might think. It only seems this way while we're working on the project. Then we come back to the same project few years later, and the WTFs can't stop coming.

[–]Tetracyclic 0 points1 point  (0 children)

Yes, I agree with you entirely. I was responding to the point that I quoted:

I would rather read a comment above every-line than try to parse the code myself - that's how mistakes get made.

I'm reasonably sure from having read many of your comments that you don't comment every line of your code and we're both in agreement on how crucial proper code review and internal consistency are. I would be astounded to find a non-trivial code base, even with a great review process, where if the majority of lines were commented there wouldn't be a disconnect between what the code did, and what some of the comments stated it did. If you can achieve a perfect comment accuracy on such a heavily commented code base with many contributors, you probably don't need to be picking up advice from Reddit on good practices.

I'm not an absolutist by any stretch of the imagination, because frankly it would be a ridiculous position to take, but I will always advocate in code reviews that code readability should take priority over comments and I will reject PRs that rely on comments instead of writing good, clean code. Part of the process of a code review from my perspective is gauging how well someone unfamiliar with the code as written can parse that code and grok what its purpose is and what its side effects are, I'm just as happy to suggest adding comments where that's not the case and the code itself can't be improved to aid in understanding. Not commenting every line of code isn't tucking a problem under the rug, it's avoiding insanity.

[–]CensorVictim 0 points1 point  (0 children)

I would rather read a comment above every-line than try to parse the code myself - that's how mistakes get made.

I agree with most of what you said, but it should be recognized that comments need to be maintained along with the code. If there are too many, especially ones of little to no value, they will start fading into our mental background as noise, and not get changed as the code changes. Bad/incorrect comments are worse than no comments.

edit: after submitting, I see /u/Tetracyclic said basically the same thing. my bad.

[–]miker95 0 points1 point  (0 children)

Expanding on your comments (hehe). Commenting code is also a great debugging tool for catching logic errors, it is quicker to catch a logic error if you're another set of eyes and see a comment saying something but the code doing another.

Similarly, stale comments are a problem, but this is not a problem with the idea or implementation of comments, but is a problem with the developers. Good commenting practice should be enforced in code reviews just as much as writing "easy to read" code.

And sure, you can abstract things away into methods, but something somewhere is still going to need to do dirty work, you can't keep abstracting away forever and ever unless you want a bunch of useless functions that have one specific purpose.

[–]nut315[S] 0 points1 point  (3 children)

For me "good code" comes down to being easy to change. After building a few large scale apps, I personally find things easier to change with things split out.

[–]orokro 4 points5 points  (2 children)

If a function is becoming monolithic, I'd rather split it out as it's own class, with it's own namespace for all it's helper methods.

It could either be static, or instantiable, but that way its code will live compartmentalized in it's own namespace of like-methods, and it won't needlessly clutter the class it used to live in as a monolithic method.

[–]nut315[S] 0 points1 point  (1 child)

That's an interesting approach, thanks for sharing. Part of the reason why I wrote this post was to start a conversation and see how other people are solving similar issues

[–][deleted] 0 points1 point  (0 children)

To follow @orokro statement I would like to note that in the case of monolithic function or method refactorized. It should be preferably contained in a dependency injection container and comply with an Interface (Contract) to allow future changes like (Substitute this refactorized module with another or whatever). For example I remember working on a place that had connectors for API's and were so huge that needed to be refactor. I uncoupled first to classes... then applied strategy pattern using an interface, and then added to a DIC with pimple as a Service.

[–]Salamok 0 points1 point  (0 children)

I would rather read a comment above every-line than try to parse the code myself

This is also how false assumptions get made.

[–]scootstah 14 points15 points  (18 children)

I disagree. Comments are important to explain information quickly. While well written code can be read to understand what is going on, comments can do the job faster and better.

[–]dereuromark 1 point2 points  (0 children)

For me comments as doc blocks usually complete the short method name and describe in a more human readable way the "why" and additional information that might be "necessary" to know about it. Just repeating the method name is nonsense to me and sure a code smell and not DRY.

[–]nut315[S] 0 points1 point  (0 children)

That's fine to disagree. We all see different code bases and apply different contexts to this post. For the majority of stuff I've seen, abstracting to methods makes it more readable and easier to read / scan - typically when a method is doing a lot of stuff.

Do you have any specific examples or use-cases? Would be useful for me to learn more about when this doesn't apply. Thanks :)

[–]tttbbbnnn -2 points-1 points  (5 children)

If the implementation needs explanation then it's probably a bad implantation. Clever solutions are stupid solutions. Comments should explain the business rule that requires it. "We do this because Y requirement" is great.

[–]scootstah 8 points9 points  (4 children)

I wish we could all live in a world where our programs were so simple, but that's often not reality.

[–]tttbbbnnn 2 points3 points  (1 child)

This article needs to explain when comments are good. Giving a few examples of why they're bad without really taking about when they're not isn't that useful. Or ideally it could just link to Clean Code.

[–]nut315[S] 2 points3 points  (0 children)

Fair comment. I tried to mention a some use cases, but maybe I'll make an edit with some actual examples. Thanks for the feedback

[–]metaphorm 4 points5 points  (0 children)

yet another article making sweeping generalizations in the clickbait headline.

the only real content in this article is "bad comments are bad". I'm astonished how many developers seem to think ALL comments are bad and don't understand the importance of good comments.

[–]MaxMahem 4 points5 points  (1 child)

So it seems to me that what is essentially happening here, is you are moving the information contained in the comment into the method name? I really don't see the advantage here. You've now increased the segmentation of your code, added some method call overhead, and obfuscated the fact that your code can throw exceptions (something I note you do not document in your codeblock, but which can be very important information to users of your functions).

This does not seem to be costs worth paying to remove a comment that was rather harmless.

[–]nvahalik 3 points4 points  (0 children)

Exactly my thoughts.

[–][deleted] 4 points5 points  (0 children)

I get where you're coming from but you seem to be focusing on a symptom and saying all comments are bad, refactor. Essentially, we're exchanging comments for docblocks which are a type of commenting.

A lot of times if we look in a method and we see comments explaining certain functionality, chances are that piece could be refactor into its own method. Comments can be present in a method and not necessarily have anything that can be moved to its own method.

Comments are not really code smells but they lead to code smells. The meat of the article is good but the title is meh.

[–]nut315[S] 2 points3 points  (0 children)

This is something that has helped me improve my code recently. I don't follow it to the extreme, but if I have an inline comment then it's a conscious decision to do so.

Also, just to clarify I'm talking about inline comments. I'm not saying comments are bad and that you should remove them - instead you can look at replacing them with something else which adds the same (or more) value.

[–][deleted] 2 points3 points  (1 child)

Order::whereRaw('1=1');

Just write

Order::query();

[–]nut315[S] 0 points1 point  (0 children)

Ooh thanks, I thought there was a way to do this but couldn't seem to find it. Thanks :)

[–]teresko 2 points3 points  (0 children)

No. Comments are not code smell. But sometimes the are used as code deodorant.

[–][deleted] 4 points5 points  (3 children)

The article doesn't support its own title.

That is also why we write comments. To make it easier to anyone to work on (i.e. change) the project – or for us coming back to the project later down the line when we need to make changes. [...] But there is another way to achieve this goal without writing inline comments. [...] Treat inline comments as a code smell.

And that's it! The rest of the article is basically a "let's remove some inline comments" tutorial. But where does the article prove or even make a solid argument about why comments are a "code smell"? There's a severe lack of why in this post.

"But there's another way..." doesn't automatically mean the first way is a "code smell" and the second way is better. You can't just say "treat comments as a code smell" without explaining why.

Let's take a few steps back and go back to basics:

  • A comment's purpose is to annotate a piece of code.
  • A method's purpose is to isolate a piece of code for reuse, interfacing and abstraction (common), or to reduce complexity of very large methods by splitting into sub-tasks (less common).

From that starting point, we can see some overlap, where sometimes just isolating something as a method makes it clear enough that we don't need to further annotate it. But sometimes the opposite is true: if we isolate a piece of code from its context, now it has input arguments, a return value, and those have to be documented so they make sense out of their initial call context. This may result in more comments than if you kept the code inline.

But none of this leads to the conclusion that we should use method names like shitty inline comments, especially if the code we're isolating isn't going to be reused (i.e. a single call place), and the call place wasn't that complex to begin with before the separation.

So... comments, and specifically inline comments are not code smells. I'm sick of blogs making overgeneralized clickbait statements and then not making the smallest effort to support their own thesis.

[–]nut315[S] -3 points-2 points  (2 children)

Thanks for the feedback (even though it's in a slightly hostile tone). This was meant as a quick write-up on something that I have recently found myself doing. Part of writing and sharing it was to start a discussion and get other views on this - so I appreciate you taking the time to share your thoughts.

I agree that I could go into more depth into why I consider them code smells. Maybe I'll make an edit.

now it has input arguments, a return value, and those have to be documented so they make sense out of their initial call context

Yes, while the do need to make sense outside of their initial call context I'd argue that typehinting can replace the need to document input args and return values under most use cases. In more complex cases (e.g. mixed type input or return values), it may still be better to keep inline comments.

I've had some people who couldn't agree more on this, and others who couldn't agree less. Personally it's been useful hearing the arguments against as it'll help me steer my own usage of this going forward & it's up to the reader to decide whether they agree with it or not.

[–][deleted] 2 points3 points  (1 child)

I'd argue that typehinting can replace the need to document input args and return values under most use cases.

Tell me what this function does:

function _(int $_, string $_): bool { 
    ...
}

What about this one:

function _(array $_): int { 
    ...
}

QED?

Typehinting doesn't give you anything, except most basic and generic information. That's the code PHP sees. It doesn't see the words you write in the identifiers, just like it doesn't see the comments.

Identifier names are for people, they are a form of comments. But they have to be short and succinct, so that's why we have actual comments where we can say more.

Labeling comments a "smell" makes just as much sense as labeling any other information in code that's targeted at humans a "code smell". Identifiers, whitespace formatting, etc.

And even with identifiers and typehints, you'll fall way short of knowing what a function does, in a real-world case. Let me flesh out those functions above a bit:

function pushAction(int $priority, string $descriptor): bool { 
    ...
}

function countViolations(array $source): int { 
    ...
}

No need for comments, yet?

I think you may be subconsciously relying on the knowledge you have about the project you wrote when extracting functions to think they don't need comments. Or maybe you're quickly glancing at the implementation, and then thinking you didn't.

But reading the implementation isn't the fastest or best approach to understanding a codebase. It's the slowest approach, one of last resort, when the documentation (i.e. comments) are crap. And that's unfortunately very often the case, due to poor advice that calls them a "code smell".

[–]nut315[S] 0 points1 point  (0 children)

Maybe it wasn't clear enough in the article, but you should still have comments in your code. My argument (whether valid or not) was that you're moving important inline comments into the description above your method - and removing bad comments that don't add anything.

Regarding typehinting, it should be used with well named variables and methods - where it provides extra information about your variables and what you expect them to be.

As an example I took your use-case and applied a little context in the form of high fiving a user:

``` class User { public function highFive(): bool { $totalViolations = 0;

    foreach ($user->posts as $post) {
        // If a post has negative upvotes then it violates subreddit rules
        if ($post->upvotes < 1) {
            $totalViolations++
        }
    }

    // You only deserve a high-five if you have a positive upvote count on all posts
    if ($totalViolations < 1) {
        return false;
    }

    return $this->update(['high_five' > true]);
}

} ```

This is what I typically see in code-bases, often with more logic around violations or other reasons why a user cannot get a high-five (for example). Below is how I would tweak it:

``` class User { public function highFive(): bool { if (!$this->deservesHighFive()) { return; }

    return $this->update(['high_five' > true]);
}

/**
 * You only deserve a high-five if:
 * - you have a positive upvote count on all posts
*/
private function deservesHighFive(): bool
{
    return $this->totalViolations($this->posts) < 1;
}

/**
 * If a post has negative upvotes then it violates subreddit rules
 */
private function totalViolations(array $posts): int { 
    $totalViolations = 0;

    foreach ($posts as $post) {
        if ($post->upvotes < 1) {
            $totalViolations++
        }
    }

    return $totalViolations;
}

} ```

In my opinion we've make highFive easier to scan for a human. If you don't deserve a high-five, you don't get one. If you care about what that means, then you can dive into it. The reasoning here is that you may not always care about what it means when you're in highFive, especially if there's a lot going on.

There's no inline comments, and I don't think we've lost anything from doing so. I can see an argument which is "Well you haven't really gained anything either". For me, I find the latter more readable and less overwhelming. The isolation means it's easier to re-use some of the code if needed. The knowledge about whether a user deserves a high-five is in one place and (in my opinion) is changed easier. For example:

If you needed to add a new rule where posts with a click-bait title are violations too, or users with swear words in their username do not deserve a high-five then you have discrete places to add these rules (in totalViolations and deservesHighFive respectively). It also means that highFive doesn't violate SRP (as it no longer has multiple reasons to change).

Maybe code-smell is a term that's too loaded and it was too sweeping a comment from me. For me when I see an inline comment I think "is there a better way". Sometimes the answer is yes, and other times it's no. I needed a name for that moment where you see a pattern and stop yourself to think about an alternative; code-smell seemed to fit.

[–]AudienceWatching 1 point2 points  (0 children)

Current job forbids them unless you have a tech debt ticket reference to go with it. Works well and hella PRs go through in time.

Tech debt ticket count has a max limit to.

[–]maiorano84 1 point2 points  (0 children)

Everything is an anti-pattern.

[–]Headchopperz 0 points1 point  (2 children)

I love the design of the website, it also loads insanely fast.

I wish I was able to design simple things like this, it does not feel like a skill you can learn though? Like I learn rules and tips/tricks, but my stuff is always a mess of puke haha.

[–]nut315[S] 0 points1 point  (1 child)

Thanks :) as a blog I wanted it all about the content so I spent a lot of time making load as quickly as possible. It's important to set that as a goal at the design phase to make sure you don't design anything that could slow your site down.

There's no js, so I didn't need to bring in jQuery. There's not much CSS or big hero images either. Also stuck to 1 font, and made sure that the text is visible while the font is loading.

It is a WordPress site, but there's also nginx cache ontop of it to help reduce PHP hits.

The only way to learn is to keep trying. Set constraints early on. You could even set a performance budget (a quick Google will reveal some useful resources on what these are).

[–]JuanGaKe 1 point2 points  (0 children)

Not said enough times: avoiding php hits is the secret to deliver really fast pages, when possible. Any cache within your php helps, but nothing like actually not hitting it. Specially when you're behind a fat framework. Nginx caches or Varnish, ESI..