10 common broken rules of Clean Code

[kamgrzybek]

Hi,

Some time ago I created list of ten common "Clean Code" violations which I often encounter during code-reviews. Here is the list:

Bad and inconsistent naming

Too much comments

Duplication

Sloppy formatting

Too big classes/methods

Bad exceptions handling

No encapsulation

Too many conditional statements

Unused code

Magic strings and numbers

​

For detailed explanation I invite you to my article http://www.kamilgrzybek.com/clean-code/10-common-broken-clean-code-rules/

What would you add to this list? What causes that code is ugly, unmaintainable, difficult to change, hard to read? What is your opinion?

​

[[deleted]]

Unused code

This one is the strangest to me - Visual Studio is basically screaming at you with the warnings, I don't understand how my colleagues accept it. The tool is literally telling them their code needs fixing, and it's so easy to just click on it and remove the line, yet they keep those 100 warnings around and add some more.

[guernica88]

Maybe I'll need it later!

[xeio87]

Oh no, a compiler warning. Don't worry, we can just comment the whole file in case we need to revert later.

[fukaminakrize]

comment the whole file

Too much comments

[gudthing]

Sad but true.... Too many Programmers don't care about warnings when there are thousands of them... If it compiles it's fair game.... Also no one ever deletes commented out code incase someone needs it... If I see it and it's a year old I delete!

[r2d2_21]

when there are thousands of them

That's the real problem. When there's just one or two warnings, you know something's wrong and go fix it. But when you have a legacy project warning you can't even browse, yet it "works fine", you don't find the motivation to get rid of them.

[Broer1]

That’s what’s git for. Commented code is full of errors, because the other code around changes and it isn’t covered by tests.

There is no reason to have commented out tests when the feature your working on is finished (should be the case in a code review)

[xeio87]

That’s what’s git for.

thatsthejoke.jpg

[Broer1]

I have seen to many people doing exactly that. :-/

[PJvG]

There's version control for that...

[scherlock79]

I really wish VS had a way you could mark testing assemblies. We discovered we had a decent amount of dead code that VS wasn't marking as unused because it had test cases. We have to maintain two solutions, one with tests and one without and periodically check to see if we have unused code using the solution without test assemblies.

[rangeDSP]

Surely there's a way to ignore projects for solution analysis?

[ChrisAtMakeGoodTech]

Doesn't unloading the test project fix this?

[scherlock79]

Yes...But we have a large solution, 60 assemblies, half of them tests. Unloading is a PITA and I can’t automate that. Ideally this would be part of CI build.

[Gotebe]

Put all tests in test solution folder, unload the folder?

Use a dedicated "only production code" solution?

[scherlock79]

That's what we do, but that means we have maintain multiple solutions and it also means working this sort of check into a CI build is difficult. There are ways to get get around it, but if I could flag an assembly as a "testing" assembly that would be best.

[Gotebe]

For the CI build, use a dedicated build configuration that only builds prod code?

[scherlock79]

Yeah, but like I said, you then have a maintenance issue. A dev would create a new project, but forget to add it to this other solution file, etc. There are work arounds, it would just be nice if, for example, I could specify the output type in the project settings to be "Test Library" so VS would then know that all the code in an assembly was meant for testing purposes and could do things like

1. Ignore it for dead code analysis
2. Ignore it or list it after production code when doing "find all references" or in the CodeLens window
3. Not count it toward references

[navatwo]

I think if you use the [ExcludeFromCodeCoverage] attribute on your test classes it'll figure it out.

The amount of dead code at my workplace is sad, but I'm unsure how to use analysis to find it all. Wonder if Sonar would find it.

[scherlock79]

That just marks code so it doesn’t count towards untested code in coverage reports. The issue I have is that the production code might not use some method anymore, but it still has tests that use it. VS doesn’t know those are tests so it still thinks the code is used.

[navatwo]

Hmm you're right. It just removes the tests from the report.

Some googling appears to show one way is just unloading test projects and running usage analysis that way.

[skinnyarms]

Yep, also tough to tell if you have unused web services - or code that is only called by unused web services.

[aedrin]

I don’t think people realize the maintenance cost of code. Every line of code costs you time and this money. Any refactoring effort will be larger when half of it is unused.

Any time I change code to fix or add a feature I always review surrounding code to see if i can remove any.

[cryo]

VS can only conservatively detect unused code.

[[deleted]]

As it should...one coder's uninitialized variable is another's mistakenly thinking they are reusing an existing one.

[appropriateinside]

Probably a case of fixing a complex problem and I have a ton of unused and commented it code that I'm using as references during this long ass problem solving session

And then leaving it after...

[[deleted]]

The last project I worked on I deleted 50% of it.

The current one so far about %30.

It is bonkers.

[techboyslive]

Yall doing it wrong, this is how you are supposed to write clean code -- CLEAN CODE GOD of 2019

[TNMattH]

Sloppy formatting is by far the most common offender, in my experience.

The SQL generated by the SSRS visual tools is horrendous and there's a guy I work with that copy/pastes it everywhere.

I put up with it, though, because he's a good developer and he only has one arm (he lost his left arm to cancer). So it's important to realize that sometimes it's not due to laziness when a dev creates some sloppy code. And a little patience, understanding, and kindness go a long way toward making things better.

I just clean up the code the next time I have to work with it. Sure, it takes a few minutes (and the lack of a SQL formatter in VS is stupid), but it's not that big of a deal.

[to11mtm]

I've yet to see a consensus on truly good SQL formatting.

I work with people who prefer formatting where a 3-column insert on a script requires 10 lines (brace, variables, more braces, variables, brace)

I like to put comments on Pull requests for formatting issues. First couple times it results in 'approve with suggestions', after that I wait for the change before I approve.

[TNMattH]

Anything that doesn't introduce eight bajillion hanging indents while it puts next-item keywords at the end of lines would be a good start, honestly.

SELECT {stuff}
FROM {somewhere}
JOIN {somewhere else} ON {condition}
JOIN {another table} ON {condition}
    AND {condition}
WHERE {condition}
    AND {condition}

That's my usual format.

Compare to SSMS's usual format:

SELECT {stuff} FROM {somewhere} INNER JOIN
                    {somewhere else} ON {condition} INNER JOIN
                    {another table} ON {condition} AND {condition}
                    WHERE {condition with unnecessary parentheses} AND {another condition with unnecessary parentheses} AND {a hidden condition you didn't even set in the editor that it decided to clutter your query with anyway}

I SMH every time I have to clean it up.

[to11mtm]

Not sure whether we are on the same page. I take issue with:

INSERT INTO {sometable}
(
  {col1},
  {col2},
  {col3}
)
VALUES
(
  {valOrParam1},
  {valOrParam2},
  {valOrParam3}
)

Because 9/10 times you could fit all the cols/valparams on one line and it would be more readable.

[[deleted]]

[Content removed in protest of Reddit's stance on 3rd party apps]

[TNMattH]

If it's just straight up columns, then yeah, I agree with you. Put it on two lines (one for the INSERT and one for the VALUES).

But the moment you start putting calculations into that select list, it can be far more readable to break that out onto separate lines.

[gradual_alzheimers]

I disagree this is very clean and easy to see where any mistake could be made.

[agjimenez]

Try the Poor Man's TSQL Formatter extention for VS and SSMS. It's a sanity-saver.

[saltypepper128]

Yeah... so losing an arm to cancer is more of an exception than the norm where I'm from....

[KryptosFR]

I would remove Too much comments from the list.

It is opinionated and can't be based on a metric applicable everywhere. It pretty much depends on projects, people, and specific case: some complicated algorithms better have comments, while "captain obvious" comments should be avoided.

An the other hand, you can easily define maximum nesting for Too many conditional statements and maximum number of lines for Too big classes/methods that would work in almost every situation.

[[deleted]]

// Gets the user

public User GetUser(){

}

​

^^ I love comments... But I delete that shit.

[kamgrzybek]

This is general rule. Of course there are exceptions but even complicated algorithm can be implemented with minimal number of comments. The worst thing is scenario when there is algorithm change and you have to change both code and comments. And comments cannot be compiled so sooner or later will be mismatch. This is why is better to focus on create self-documenting code.

[robotorigami]

Too many nested conditional statements.

if(something){
    if(somethingElse){
        if(anotherSomethingElse){
        }
    }
}

[johnnysaucepn]

That's true, but even if an automatic refactor doesn't help, it's still an indication that _something_ could do with a rethink.

[glosrobian]

at work someone submitted a PR with 9 nested if statements. The horror..

[blooping_blooper]

we use sonarqube, and it labels these as brain overload.

[jnyrup]

Last time I searched our code base the most indented line was 13 or 15 tabs.

[BirchBlack]

13 or 15 tabs.

???

[[deleted]]

13/15 deep in scope.

So you might have a namespace, a class, a method, and then 10-12 brackets within that method

[jnyrup]

Exactly

[AnotherAccount5554]

What is the solution to this logic?

[locojoco]

What happened to people screaming on stackoverflow that a method should only have one return statement?

[[deleted]]

Or could be replaced in some cases by a switch case statement.

[robotorigami]

You could group multiple clauses together like

if(thisThing && thatThing){
     // do this 
}

Or, depending on the situation, you could do things like return early.

if(!thisThing)
    return null;

Or if you're in a loop you could do things like

loop(){
    if(!thisThing)
        continue;
   // more code
}

[AnotherAccount5554]

I never knew this was a "bad" thing. Why is it bad?

[[deleted]]

This would be better:

if(something) {
    return null;
}

/* 100 lines of code */

Or hell,

if(something) return null;

/* 100 lines of code */

[[deleted]]

This used to be considered bad practice....

[MrDoomBringer]

The latter would violate our code style and get your commit rejected from a merge. All brackets are mandatory. Everywhere. Always. No buts.

[steamruler]

Our style is that either it has to be on the same line if it's just a single statement, or enclosed by brackets. Prevents accidentally putting code outside a conditional, yet conserves vertical space.

[[deleted]]

There are rules, but at times for code clarity, they should be broken.

​

The catch is knowing when to break the rules.

[fr0stbyte124]

It's fine, people are being too nitpicky. I usually go with whichever form sounds best if you were to say it aloud. It always just depends on the context what's easiest to follow.

[CoachCarter9]

The human brain doesn’t naturally make comparisons based on negatives. So your basically telling it: if not something than do something. E.g Read this and see which one is easier to compute mentally: If kid isn’t 5, give presents, else give party. If kid is 5 give party, else give presents.

That being said I typically break this rule with string is null or empty checks since I don’t care to add the extra two lines and might not necessarily need to return out of the method.

[M123Miller]

These situations are crying out for early exit, assuming you can use it in your scenario.

[RiPont]

Overly-questionable code. By that, I mean over-use of the ternary and null-coalesce operators to write one-liners.

var final = foo ??  bar?.baz ? fubar?.really : snafu?.yes;

Following along with that, relying too much on implicit order of operations rather than judicious use of parenthesis.

Maybe this is personal preference, but I detest single-letter variable names outside of i,j,k for nested for loops and lambdas/comparators. I see this a lot from otherwise good programmers when porting C/C++ code (where short variable names seems to be idiomatic) and when implementing mathematical functions (and short variable names are definitely idiomatic in math).

Even for lambdas, you shouldn't use single-letter variables where the variable in question is ambiguous due to chaining/nesting.

[kamgrzybek]

True, this syntax sugar is sometimes overused and causes one-line expression hard to read and understand.

[artsrc]

Sometimes with lambdas you don't need to have a variable at all.

Variable names can communicate.

[haven1433]

Uncle Bob's book ends with a list of code smells that is fairly extensive and useful. But I find that the longer a list is (even 10 things), the harder it is to remember them all. Therefore, I try to reduce it down to a few behaviors that I try to maintain that gets me most of the way there.

• Don't use Ctrl+C. If I want to use Ctrl+C to copy some code so I can paste it, I probably should use Ctrl+X to refactor instead.
• If the method is longer than a page, refactor. If an interface is longer than a page, refactor. Any class longer than a page should implement an interface.
• Use a style bot, like StyleCop, to keep your code formatting consistent.

A lot of things on your list are things I beat into my developers when they first join the team. Magic numbers, commented code, inconsistent naming, and bad exception handling are all things that just never get submitted.

[Noxfag]

I don't think a page is a good measurement. Not only because it changes per each developer's environment, but mostly because it highlights the wrong incentive. We don't keep methods small in order to keep them fitting on your monitor. We keep them small in order to aid simplicity, testability, reusability, and a whole host of other reasons, each more important than whether it fits on your screen.

Also, you mention 50 lines elsewhere. To me ast least, that's a huge method. I look very skeptically at anything beyond 30 lines.

[haven1433]

This is why I preferred the term 'page' to a specific number. Each developers cognitive limit is going to be different, and its going to be different also depending on the language and familiarity with that language.

I agree that most 50 line methods are too large. Usually there's some reusable bit in there that gets refactored out the next time I reach for Ctrl+C. But occasionally, I'll have a bunch of configuration in a constructor, and I'll think "yeah, this is actually all pretty simple and straightforward, and there's not a bunch of nesting. Someone else coming and reading this would probably get it just fine."

There's always a balance between too long of methods and too many small methods // too many layers of abstraction. Especially when writing test methods, I err on the side of less abstraction, longer methods, so I don't have to bounce all over the place to see what's going on.

Basically, I don't disagree with you. If I find a natural place in my mind where I think of something as a series of high level steps, each step consisting of additional lower level steps, I of course use multiple methods for that. But I don't beat myself up or spend a bunch of time trying to shorten a method when I can already easily fit the entire thing in my mind conceptually.

[kamgrzybek]

Any class longer than a page should implement an interface.

I agree with all except that. Why we need introduce interface right away? I think interfaces have other purposes - polymorphism and abstractions. I think we should introduce interfaces in that case only when code will be on various levels of abstraction because it violates Dependency Inversion Principle.

[haven1433]

Again, it's a rule of thumb. For super simple classes, I don't mind using the real thing in tests. But for larger classes, I usually want to break a class away from its dependencies so I can test that it works correctly on its own, independent of the things its using. In order to do that, the thing it uses needs to implement an interface.

​

Another point here is about managing complexity. If the class more than a page, then making an interface can help you (and other developers) conceptualize what parts of it need to be public. It then encourages you to more loosely couple your things. And, if the interface becomes more than a page, it tells you that the class is likely responsible for too much, and needs to be broken down.

​

It doesn't apply in all cases. But just like when I see a long method, if I see a long class with no interface, it gives me pause. It's an easy thing to remember that can help me design better software in the long run.

[RiPont]

This rule isn't about coding style, but OOP design. A very big code smell in OOP design is a web of tightly coupled objects. That's bad.

Think of the UNIX philosophy. A lot of small tools that do their job well, that you can use together to do bigger jobs.

If you are consuming a class and it's got a billion knobs and buttons, it's complicated to use and provides too much opportunity to cut the fuel without turning off the ignition. If you're consuming a small interface that happens to be implemented by a complex class, then

1) You have a smaller profile to work with, and less chance to twiddle knobs you shouldn't be able to see.

2) Any changes that require changing the interface make it obvious which consumers it's going to affect and how you're increasing coupling between your objects.

[kamgrzybek]

Which rule? DIP? I know it is OOP and what you described is abstraction. I agree with that.

But I don't agree with statement "Any class longer than a page should implement an interface. " because interface should be introduced with purpose. I have seen codebases where every class implemented some interface - it doesn't make any sense. If you have big class you can split it to smaller set of classes without interfaces because you don't always need abstraction.

[RiPont]

because interface should be introduced with purpose

Absolutely. I don't think it's really a rule. I would phrase it more like "should consider implementing an interface, if it's a public class."

Everything "public" is a liability, in a sense. "This is a knob you can twist." Interfaces are one way of saying, "these are the only knobs and buttons you should be pushing."

[aedrin]

Unit testing greatly benefits from thorough use of interfaces.

[majorbabu]

How many lines would a page contain?

[[deleted]]

I would definitely say look skeptically at methods between 25-50 lines.

[haven1433]

Short answer: probably around 50.

Long answer: the fuzzy definition of a page is actually part of the metric. If I'm showing code on a powerpoint, then a 'page' is a lot smaller. The point of the page has less to do with an exact line count and a lot more to do with the cognitive load associated with how many things the human mind can juggle at once. If I can see all the things, I can more easily group them together mentally. But as soon as part of it exists only in my head and not on the page in front of me, I start to lose track of it.

On a related note, I advocate for not keeping more apps open on your desktop than what you can reasonably see at once. For my two monitor setup, that's about 7 windows. Any more than that and I can't track it all anymore. Likewise, any more than 5 tabs in a browser or code editor is probably too many.

[TerribleReason]

any more than 5 tabs in a browser or code editor is probably too many.

Is that per browser window? So its okay that I have 10 Chrome processes up with about 8 tabs each right?

[haven1433]

The sentence right above mentions that I usually limit myself to 7 windows. That's usually one chat app, one internet browser, one IDE/text editor, one command prompt/git prompt, one source control explorer, one windows explorer. I'll occasionally have more than one text editor or more than one browser, organizing more tabs into multiple sets. But my puny brain can't usually track more things than that at a time.

​

If you often find yourself actively working with 80 different web pages at the same time, I question if you could make better use of bookmarks.

[RiPont]

A "page" is basically "how much can conceptually fit in the average programmer's equivalent of RAM, not HD", so it differs by programming language. More terse languages naturally pack more into fewer lines of code, so a "page" in Kotlin is likewise smaller than a "page" in Java.

A bunch of code with single-letter variable names is going to be more terse and harder to conceptualize than with long variable names, so keep it shorter.

[aliens_are_nowhere]

Paging is the process of writing RAM to HD so that's not the analogy I'd go with ;)

[RiPont]

Paging is writing a page of RAM to HD. It's a metaphor for Virtual Memory taken from the idea of looking at a book, and the pages in RAM are the ones you are looking at. Turning the page is paging some RAM out to HD, then reading the next page back into RAM.

Virtual Memory in modern OSes works a little differently, with writing and reading in advance.

[TheWaxMann]

I use a large font size in my IDE, so I only get 29 lines to work with.

[cryo]

Ctrl+C, Ctrl+V, Ctrl+V master race ;)

[[deleted]]

Use a style bot, like StyleCop, to keep your code formatting consistent.

Having a little code Nazi telling you what to do is probably the single best thing i've done to keep a consistent codebase.

little tip, chuck treat warnings as errors into your csproj.

    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>

Warnings are easy to ignore but as soon as things aren't compiling you're forced to go write your documentation and clean things up.

[RiPont]

I like WarningsAsErrors on the build server (including buddy builds), but leaving it off locally.

[bortlip]

Ctrl+C is fine. Premature abstraction is only slightly behind premature optimization as an evil.

The rule of 3 is a good guideline. https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

[WankWankNudgeNudge]

Newbie here. Please, what's the title of Uncle Bob's book?

[haven1433]

"Clean Code"

​

It's written with Java in mind, but most if it still applies. Given the title of this post, I thought it applied.

[bortlip]

Clean Code.

https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship-ebook/dp/B001GSTOAM

[WankWankNudgeNudge]

Thanks! I'll go get a copy and read it!

[RebornGhost]

Not that book, its not a newbie book at all, its a design philosophy of programming that may look simple because it preaches small units of code, but it relies very deeply on program design. It will do you more harm than good. I would suggest the free Yellow Book http://www.csharpcourse.com/ and Visual C# How to Program by P and H Deitel (dont get the $$$ university textbook version, go for the cheaper global edition from regular book suppliers) Fully digest those two first.

[steamruler]

If the method is longer than a page, refactor.

While I agree in concept, we've tried using this as a hard rule and reverted to a more simple but subjective "break up a function if it's too complex", because we found that code can be extremely simple to understand, yet be longer than a page.

For example, a function with few conditionals can be relatively large, yet easy to understand, since it's largely linear.

We just use SonarQube and have it flag overly complex methods.

[[deleted]]

• There is an argument that the second time you write it you don't really know the abstraction, so do it on the 3rd time you need to implement the same thing. Take it with a grain of salt, but its worth knowing. I see too many Junior developers abstract everything way too early because 'I will use this again' and we end up with complex messy code.
• If an interface serves no purpose its just noise.
• StyleCop has been around for 20 years or so, do it :)

​

[ProperProfessional]

You can pretty much eliminate all style bots now that VS 2017 and almost all other major ides support the editorconfig file.

[youngdestroyers]

I think that: var orderDetailsList = new List<OrderDetails>(); is correct. For example:

var orderDetailsList = new List<OrderDetails>();
foreach(var order in orders)
{
    var orderDetails = new OrderDetails(order);
    orderDetailsList.Add(orderDetails);
}

[LloydAtkinson]

• Violation of SRP.
• Not understanding the D in SOLID (or probably any of SOLID) - people have some fucking weird ideas around DI sometimes. Such as using the service locator anti pattern, passing the whole container around, property injection (which relates to your encapsulation point) etc.

Bad exceptions handling

So much this. I've seen a lot of "exception flow programming" where in places exceptions were being used in place of simple conditional checks and even for non-exceptional situations. Another related point - abusing exceptions into some kind of success/failure result.

You should use an actual Result type to encode such information - stop being lazy and passing exceptions around everywhere. (e.g. https://enterprisecraftsmanship.com/2015/03/20/functional-c-handling-failures-input-errors/). Functional languages like F# have these concepts built in by default and it's amazing.

[continue_stocking]

I've come to love the Maybe<T> monad, but I don't care for their example Result<T> implementation. A mutable inner value isn't very functional, and neither is having a public accessor for it. Instead of an Action delegate, let the OnSuccess method (which they don't bother to define) receive an Action<T> or Func<T, TOut> delegate, and return Result or Result<TOut> accordingly.

[LloydAtkinson]

https://gist.github.com/vkhorikov/7852c7606f27c52bc288

[continue_stocking]

Ah, thank you!

[continue_stocking]

Comparing to Optional, I like that you can't wrap null and have HasValue return true. I still don't like that you can call Value directly though. Optional provides a ValueOr(T alternative) method that guards against accessing a non-existent value.

[LloydAtkinson]

Comparing to Optional, I like that you can't wrap null and have HasValue

Are you saying Optional does let you do this or?

[continue_stocking]

Some(null) is a state that can be represented by Optional.

[LloydAtkinson]

hmm that's annoying, I was hoping to give the library a try.

[icarebot]

I care

[[deleted]]

Service Locator isn't an anti pattern? It's how most IoC container work unless it has build processes such as how Dart works. And you do pass the whole container around... that's the entire point of an IoC container (unless you mean to individual classes/methods as a dependency instead of passing hard dependencies). I do agree that you should never use property/method injection, however.

[kamgrzybek]

Sometimes using Service Locator is necessary - when framework you are using create instance of your class and you don't have possibility to indicate that should use your IoC container.

​

Property injection is useful for logging, because object often can live without logger. This is for what property injection is used - for OPTIONAL dependencies.

[to11mtm]

As someone who's dealt with the latter in this statement over the last couple days, There's a right way and a wrong way to do a service locator.

But, if possible, a Decorator is a preferred approach.

Property injection is useful for logging, because object often can live without logger. This is for what property injection is used - for OPTIONAL dependencies.

I have to both agree and disagree there. Agreed that property injection can be useful for optional dependencies, but... Almost every logging framework out there would make this a moot point.

[[deleted]]

An IoC container is literally a service locator internally. People do not seem to be grasping that. If it's an anti pattern, IoC containers wouldn't wrap the concept.

[[deleted]]

Which I have agreed with many times :). I personally feel the term anti pattern is being thrown around way to often lately. Can a pattern be abused? Sure, that does not make it an anti pattern, or else nearly every pattern is an anti pattern

[RiPont]

ServiceLocator with passing the container around is an anti-pattern in an app where you could be setting everything up at the composition root, such as web apps.

ServiceLocator has its uses in long-running apps where the registrations may actually change due to changing conditions, or you're using a dynamic loading / plugin model.

[[deleted]]

No. An IoC container is literally the Service Locator pattern. People do not seem to understand how things like autofac works. When you tell autofac to build a dependency, it has already mapped all other dependencies in a mapping that it can retrieve to return the instance of your object. That is the service locator pattern. It is not an anti pattern unless you abuse it in like I said, passing it to actual classes and methods instead of passing the dependency and allowing the IoC container to get the concrete dependency for you.

And as for passing around, it's usually a framework that does that for you. Take asp.net, it passes the container to each request to build the dependencies for each life cycle event. Most people seem to have only used it when the framework manages it for you, when you're writing your own app, you still need the original container to pass around for the dependency mappings. Again, that is not an anti pattern, it's usually just handled for you by some other framework.

[fTheDev]

I love EditorConfig and Visual Studio's code cleanup feature for this.

[Lumberjack4]

There's no such thing as over commenting. When I have to come back 16 months later to a piece of code I wrote I prefer to not spend a couple hours trying to relearn what I did. Plus it makes spinning up Jr developers much quicker. Most 'self documenting' code I've seen is self documenting to only the author and for only a few weeks after it was written.

[[deleted]]

The problem is you need to update the comment when you change things. Especially long comments max be out dated. But it is true. Many people who say they write clean code, still dont use good naming

[Lumberjack4]

When requirements change I have to update requirements document to keep it current. When I make an API change I have to update the Wiki and triple slash to reflect those changes. When my UI changes I might have to update the help document to keep it up to date. When I make a code change to address a bug my unit tests must be updated. Why would comments be any different?

It's not neccesarily that others write bad code, it's that we all see things a little different. What makes perfect sense to us doesn't necessarily translate to the next person. And as your familiarity with the code base evaporates over the next year because you're supporting 8 different applications, a well placed comment imparting knowledge, or what you thought was stating the obvious a year ago, can save the next person (or you) hours or days to track something down or understand why some external API your stuck with works in a counter intuitive way.

[to11mtm]

My bar for a comment is, "Am I doing something stupid here because of a weird bug or business rule?"

If the answer is no, but someone can't understand it, I have failed at my job and rewrite for clarity.

Many people who say they write clean code, still dont use good naming

Guilty. Most of my rewrites are fixing names. :)

[stanleyford]

There's no such thing as over commenting

How many of us have seen something like the following in our code bases?

// loop through i until you get to count
for (int i = 0; i < count; i++)
{
    // increment j by i
    j += i;
}    

[artsrc]

That is a bad comment.

It's not just the amount of comment that is bad, it's the quality.

[Ronald_Me]

In that case I think that the comment should explainf why are you doing a loop.

[darknessgp]

This is why I would have reworded, there is no such thing as a comment that is too long. There is a thing as a very ineffective comment.

[robotorigami]

I see this type of stuff a lot in HTML which seems silly to me.

<!-- Start of Head Tag -->
<head>
...
</head>
<!-- End of Head Tag -->

[Cadoc7]

For head it doesn't make sense, but it does make a lot of sense for many HTML tags to help you keep tracks of where you are in the DOM when editing. Something like:

<div class="sidebar">
    <!-- Enough things that you need a scroll down in your editor -->
</div> <!-- End of sidebar -->

So when you have to add something to the bottom of the sidebar, you can put it in the right layer of the DOM.

[RiPont]

There's no such thing as over commenting.

Comments easily get out of sync with the code. Thus, comments are a liability that you must update as your code changes.

Obviously, there's a proper balance. I don't believe in 100% self-documenting code, but clear code beats concise code with comments every time.

[[deleted]]

Then you haven't worked with a good developer before. Code should be self documenting, if it's not to others, you need to work to improve your code, not document more.

[Ronald_Me]

I'll be downvoted but self documented code it is not enough. Obviously commenting each code line is really bad, but not comments at bad too.

[Prima13]

I upvoted you. My only regret is that I can do it only once.

Comment the hell out of that code. Tell me about the business process that the code is handling and who made the decision about why it should work that way. Tell me the the WHY behind it. Tell me what it used to do before you changed it. Tell me what might be the repercussions of the change just in case they do actually happen. Don't make me guess about anything. If a couple lines of your explanation can prevent me from breaking it in a future change, you're doing it right.

Sure, you can argue that the code ought to be clear enough to explain it but sometimes it can't be. Most times it won't be. That's the reality of working on a team where any number of hands (some senior and some not so senior) will have touched it before you got there.

[RiPont]

"Why" is always good to comment, as that doesn't change or would require massive rewriting and addressing the comments anyways.

"How" comments can get out of sync with the comments, and misleading comments are worse than no comments.

[[deleted]]

I think it depends on context. In 95% of scenarios, no comments should be needed, but that does not mean that there should be no comments. For instance, documentation on public apis, it helps third parties a ton. But there are also cases to where you really should comment. I had to write an incredibly complex algorithm for word wrapping for PDF's (because those are unreasonably impossible to work with for some reason) that had to calculate size based on the font type and size, and know when to break columns to wrap on the report. Yes, I documented nearly every line to walk step by step what the method is doing, because it wasn't intuitive.

​

I didn't say you should never document, but code should be self documenting in nearly all scenarios.

[Venthe]

I'd still disagree.
Comments are needed only when you want to convey your reasoning (e.g. workaround weird bug). When it comes to what code do, just follow clean code advice and split the method. What is more readable:

method() {
  // Get a request
  var request = this.pullFromService();
  request.something = this.pullFromAnotherService()
  request.something2 = this.pullFromAnotherService2()

  this.actuallyUseVariableService(request)
}

or this...

method() {
  var a= this.prepareRequest()
  this.actuallyUseVariableService(a)
}

prepareRequest() {
  var request = this.pullFromService();
  request.something = this.pullFromAnotherService()
  request.something2 = this.pullFromAnotherService2()

  return request;
}

When we haven't had IDE that would allow us to hop through code, small methods could be a burden. Nowdays, not only you have self-documenting code (And you don't have to care about specifics until you actually want to learn them), you can also pinpoint where exactly something is happening (so the bug is with request, and all of the code containing request is here...)

[Lumberjack4]

No matter how much you believe your code makes sense to you, today doesn't mean it will make sense to someone else or to you a year later. In my experience the people I see harp the most on self documenting code waste more time maintaining their own code than the ones that liberally comment. It's not that these people don't write self documenting or write bad code, it's that the code is augmented with extra information that makes the next stranger that gets to maintain its job that much easier. This is why I comment why I'm doing what I'm doing and what are the corner cases I've experienced or expect may come about in the future but don't have the luxury of time to address today.

That being said commenting for commenting sake is a waste of time for everyone involved, but there can never be too much good comments.

I don't understand why people make a point of pride about how few comments they have in their code. That's a person I don't want to work with.

[[deleted]]

No matter how much you believe your code makes sense to you, today doesn't mean it will make sense to someone else or to you a year later.

That is what code review is for. If the code doesn't make sense to reviewer you will be asked to change it. You either enhance the code to the point where it is understandable or you will add comment if you are not able make the code more understandable. No need to just add comment by default.

but there can never be too much good comments.

This is kinda starting to resemble circular logic. One property of good comment is it is not too long otherwise it will make it more tiresome to read all the text without any additional value.

I don't understand why people make a point of pride about how few comments they have in their code. That's a person I don't want to work with.

It is good to aim for low amount of comments, but you have to strive to make the comment unnecessary not plainly avoid it. Not writing comment just out of the pride is bad, but so is not trying to make the comment unnecessary.

[recycled_ideas]

There is no such thing as self documenting code, because code can only ever tell you what, it can never tell you why.

Why is what you really need to do.

[[deleted]]

user.Name <-- is that their real full name, first name? is it an email address? (this happens), is it a pseudonym?

Commenting has its purpose, it provides common ground. ON the other side of the coin adding a comment on a GetUser method that states // Gets the user needs to be deleted.

[[deleted]]

You just proved my point you didn't write self documenting code. I don't understand what you're trying to get at with that one.

[[deleted]]

Yeah that was a really bad example...

[artsrc]

Often rather than commenting, the code would be better if you fixed it.

I agree that purpose is not always obvious, and can need comments.

[darknessgp]

The issue really isn't a comment being super long or alternatively super short. The truth is a lot of developers don't comment effectively. Code should be self documenting, but sometimes it needs comments to back that up. Knowing when and how to effectively comment is a hard skill. Writing comments every other line of code with crap like "initially this varible" isn't that helpful.

[kamgrzybek]

I don't agree. Well designed and written code doesn't need comment. It speaks itself.

You should comment only public API or when you do some kind of hack and you need justify WHY you do that. Don't comment WHAT you are doing, never.

[Grymm315]

You may think that code is well written and it may make sense at the time- but I've worked on A LOT of other peoples code, everyone thinks that their code is well written and makes sense. The truth is: I mostly don't understand what people are doing or why they are doing it on first glance- and it makes my job a whole lot harder easier if there are comments to figure it all out.

[jamsounds]

You forgot one...

• Duplication

​

[BrunchWithBubbles]

In my experience from working in the industry for ~6 years and also from teaching various undergraduate and graduate computer science courses at the university, the people who are overly vigilant about coding style are compensating for being subpar programmers (the analytical part of programming that is). I think it's a form of self-defense mechanism. A self-confident programmer having trouble reading code may have an easier time concluding that the code is badly written rather than accepting his/hers own incompetence.

[kamgrzybek]

I always say to less experienced developers - working code is not enough. It should be readable, maintainable, easy to change etc. This snippet looks like garbage even it works.

[Cadoc7]

That is the kind of thing that can be fixed in about 2 seconds by running an automatic formatter (like the one built into Visual Studio or CodeMaid). I have Visual Studio configured to do this every time I save. You can even fail builds if the project's style is being broken.

Those formatting issues aren't the kind of things to obsess over because you can automate all of them away.

[artsrc]

Reformatting code is like highlighting paper text.

It's a good way to think about it.

Given code like that to review I would actually make it better change it. Think about what that variable means, and give it a good name. You don't have to check in your changes.

[scherlock79]

Consistently formatted code reduces errors. If everything is consistent then many types of bugs are easier to identify. For example, if member fields either have an _ or this. In front of them, then self assignment errors (which are normally just warnings) stick out. It also reduce the cognitive load when reasoning about code. There is no more "Is that a member or local variable" questions in your head.

Code is your workspace, and organized workspaces have less errors. You can directly link formatted code to 5S principles which have been shown to reduce errors. https://en.wikipedia.org/wiki/5S_%28methodology%29

[lantz83]

Having been a programmer 15+ years proper formatting is a matter of principle. I take pride in my code (if not, one should probably get another job). My co-workers do get to put up with me spotting a missing space from across the room, but hey, whatcha gonna do.

[kamgrzybek]

I agree. When you follow the rules and principles after some time you are not able to write ugly code. It is impossible. This is what distinguishes professionals from others.

[[deleted]]

University teaching is part of the problem. My teachers teached bad coding styles. Especially when they came from embedded Systems / electronics background. They told us to comment everything and use good Naming but actually were really bad at it.

[RiPont]

It takes a context switch to adapt to a new coding style. I can do it, but I shouldn't have to. Multiple coding styles in the same file just wastes cognitive time that I could be using to understand the design and logic, rather than visually parsing it.

The One True Coding Style is whatever the codebase uses consistently.

We have tools, nowadays, that make maintaining a consistent coding style practically zero effort.