hussein cheayto

Posted on Aug 18, 2019 • Updated on Aug 30, 2022

10 CODING MISTAKES THAT MAKE YOUR CODE SMELL

#programming #coding #advice

I've created this list by walking through several different programs, coding books (like "The Pragmatic Programmer" by Andy Hunt & Dave Thomas and "Clean Code" by Rober C. Martin) and refactoring them. As I made each change, I always think why I made that change. The result is a very long list of things that smell bad to me when I read code. In this article, I have mentioned the top 10 coding mistakes that I've encountered when reading code.

1- Redundant Comment

A comment is redundant if it describes something that describes itself.
For example:
i++; // increment i

2- Poorly Written Comment

Take your time writing comments and make sure to take the time to make it is the best comment you can write. Use correct grammar and punctuation and be brief.
For example:

Public void DisplayTutorial() //when used, the tutorial window will be opened to show the tutorials

Correction:

Public void DisplayTutorial() //Displays window

3- Commented-Out Code

How many times have you seen hundreds of code commented?? Who knows how old it is? Who knows whether or not it’s meaningful? Yet no one will delete it because everyone assumes someone else needs it or has plans for it.
That code sits there and rots, getting smelly more and more with every passing day. Solution??? Very simple. just DELETE IT!! Don’t worry, if anyone really needs it, he/she can go back and check out a previous version.

4- Too Many Arguments

Functions should have a small number of arguments. No argument is best, followed by one, two, and three. More than three is very questionable and should be avoided with prejudice.

5- One Function Multiple Tasks

Ideally, every function should serve only ONE task.

For instance:

public void Calculate(float a,float b,float c)
{
if(Add)
{
a=b+c;
}else if (Substract)
{
a=b-c;
}else if(multiply)
{
a=b*c;
}
}

Correction:
public void Add(float a, float b,float c)
{
a=b+c
}

public void Subtract(float a, float b,float c)
{
a=b-c
}

public void Multiply(float a, float b,float c)
{
a=b*c
}

6- Dead Function

Methods that are never called should be discarded. Keeping dead code around is wasteful. Don’t be afraid to delete the function. Remember, your source code control system still remembers it.

Related Article: Get Rich While Sleeping

7- Inconsistency

Consistency, when applied as it should be, can make code much easier to read and modify.
Inconsistency is the habit to do something a certain way. Once done, do all similar things in the same way. Be careful with the conventions you choose, and once chosen, be careful to continue to follow them.
For example:
Using variable names as 'Quit' and 'Exit' instead of sticking to either 'Quit' or 'Exit'.

8- Duplication

Duplication is a very serious problem. Almost every author who writes about software design mentions this rule. Dave Thomas and Andy Hunt called it the DRY3 principle (Don’t Repeat Yourself). Kent Beck made it one of the core principles of Extreme Programming and called it: “Once, and only once.”
It's simple, find and eliminate duplication wherever you can.
For example:
Duplications appear when using the switch/case or if/else chain that appears again and again in various modules, always testing for the same set of conditions.
Correction
Think about using polymorphism.

9- Misplaced Responsibility

One of the most important decisions a software developer can make is where to put code.
For example:
where should the PI constant go? Should it be in the Math class? Perhaps it belongs in the Trigonometry class? Or maybe in the Circle class?

Correction
Code should be placed where a reader would naturally expect it to be. The PI constant should go where the trig functions are declared. We’ll put it in a function that’s convenient for us, but not necessarily intuitive to the reader.

10- Function and Variable Names

Function names should say what they do. Always choose Descriptive Names and take your time picking a name, it is worth it. You will spend sometime thinking, but it will definitely save you time later, when you read your code after a month or a year.
Look at this code:
Date newDate = date.add(5);
Would you expect this to add five days to the date? Or is it weeks, or hours?
You can’t tell from the call what the function does.

Correction:
If the function adds five days to the date and changes the date, then it should be called IncreaseByDays or AddDays.

Top comments (35)

Ben Sinclair • Aug 19 '19

I don't think that the corrected comment in point 2 is any better than the one it replaces.

Public void DisplayTutorial() //Displays window

That's redundant, and if it's not then it's a smell that the method should be called DisplayTutorialWindow or something instead.

I'm not sure any of your "related articles" are related in any way other than they're written by you? Are they just spam?

hussein cheayto • Aug 19 '19

They are related to programming and how to succeed in life.
Check them out and see 😉

Ben Sinclair • Aug 21 '19

Methods that are never called should be discarded. Keeping dead code around is wasteful. Don’t be afraid to delete the function. Remember, your source code control system still remembers it.

Related Article: Get Rich While Sleeping

Hmm.

hussein cheayto • Aug 21 '19

The title is tricky.
If you read it, you will find that in the article, all I talk about is programming.

Ben Sinclair • Aug 22 '19

The title is tricky.
That's not exactly true. It is a link to a page about getting rich quick, though that page doesn't offer any real advice.

in the article, all I talk about is programming.
That's not true at all. You have three sections, each of which exists to provide a link to another unrelated get-rich-quick page, and only one of which is called "programming".

And it doesn't talk about programming. It says you can make money in your sleep if you publish an app. You can do the same if you publish a book, by the way, this isn't related to programming.

This linked page has nothing whatsoever to do with your preceding paragraph, and calling it a "related article" is misleading to say the least.

hussein cheayto • Aug 23 '19

hmmmmmm, I appreciate your comment, I will take it into consideration in my next articles.
If you didn't like it, you can simple close the tab and skip it :)

Tech Suman • Aug 19 '19

"No argument is best" What's your rationale behind this? If a function isn't receiving any argument, then either of three things can be said about it. One, it's not receiving data from other parts of the program and is directly interacting with the user to get it's inputs. Two, it's interacting with global variables. Three, it's a routine task, that doesn't need any data. If it's return value is session invariant, or is using some other library like datetime, then it makes sense, otherwise, function with no argument seems a little funky to me. The first two possibilities are a big no no.

Alexander Holman • Aug 19 '19

There are a few more than 3 ways to peel a potato! e.g. where the data owns the method, and the methods mutate the instance directly without any input, see the example below:

class Account {
    private LocalDate expires; // e.g. 2019-08-19
    public void renew()
    {
        this.expires = this.expires.plusYears(1); // now 2020-08-19
    }
}

Eljay-Adobe • Aug 19 '19

I agree with this. Having a function with no arguments means the function only has a side-effect, or operates on variables external to the function. It is far better to pass in all parameters to the function than having a function that is entangled with external variables (including member object variables, which are "global" to the function, even if the function is a member function of the same object).

Srini Vasudevan • Aug 18 '19

Thanks for sharing. Good article. One thing to add about comments is, it is better to avoid all together unless it's helping with auto generated documentation. Methods/functions should be self explanatory, and as you have pointed out, they should do one thing. In that case, name it accordingly and you wont need comments. E.g rather than calling a function Increment(), if it increases a number by one, better name could be IncrementByOne(). Trivial example but shows the intention.

In my years of development, I've always found comments always confuse developers more than they help as the time they stay in the source code grows. They can rot easily as code changes but devs can forget to remove or update comments. Best to avoid them all together :)

hussein cheayto • Aug 18 '19

Absolutely correct!!
Why need comments if variables and function names explain it all ;)

Wouter • Aug 18 '19

Formatting tip: if you put 3 backticks, it will create a code block for your code snippets.

Like so:

```
your code here
```

You can even set a language for syntax highlighting:

```javascript
your code here
```

hussein cheayto • Aug 18 '19

The list goes on and on... Thanks for pointing that out

zakwillis • Aug 19 '19

Can agree with these. Sometimes they can't be avoided.

hussein cheayto • Aug 19 '19

True, but try to think on how to reduce them.

zakwillis • Aug 19 '19

I would say this. In simple applications, following something like what Uncle Bob adheres to, it is easy to do what you suggest. However, some code is just complicated. Ideally, maybe 80-90% of code follows what you suggest, but sometimes it is too much to expect a developer to break everything up into functional units. It can be done but perhaps it isn't worth spending another week on an application that has taken three weeks.

Right now, I am building a publisher application and everything has been broken into simple objects with one method and using dependency injection but do I care enough to make the fact that it will only call four main functions follow the open closed principle;
NewReportPublication
SaveReports
PublishReports
NotifyEndPoint

Or can I get by with an enum and switch?

Anyway, open for discussion and good post.

hussein cheayto • Aug 19 '19

Theoretically, it is easy, practically, it needs a lot of practice and logic.

Planning is important. I believe that spending 2 or 3 days brainstorming and planning will save you much time later on.

Goodluck with your project, once done, share a link so that I can check it :)

You're a developer and you know that time is crucial.

Have you ever worked on a project where the previous developer had a smelly code? Like bad names, comments....

zakwillis • Aug 19 '19

I don't know. I don't like to pretend my sh!t doesn't stink. We all make mistakes when working and time pressure.

What appears readable and perfect to one person is not to another. Some patterns seem like nonsense to me, whereas others love it. I don't know if you are a C# developer, but as an example - should Entity Framework's linq expressions get exposed to the Business Layer? Or should it be abstracted away? I avoid using Entity Framework unless for clients but the answer is, of course - we should put a layer in-between. Many though would see this as overkill.

Anyway, you aren't talking to a new developer who struggles with these things, just I think we need to be a bit more balanced. But again, thanks for your article and it is useful for many.

hussein cheayto • Aug 19 '19

I guess there's a misundertanding here.
The point in my question was: always think that there's another one that will read your code (it might be yourself after a couple of days, months or years).

No matter how experienced we are, we still make some mistakes, that's for sure. That's why we keep learning ;)

Thanks and glad you like my article Zak

zakwillis • Aug 22 '19

All good. And keep it up.

Ahmad El Harake • Aug 19 '19 • Edited

I knew you were a French educated Lebanese because of "substract" instead of "subtract". Great article keep it up. However, I should point out that since you mentioned "consistency" as one essential point, this would contradict with the idea 4. As sometimes for the sake of dependency, we keep functions with x arguments calling an overloaded one with x-1 arguments and etc.

hussein cheayto • Aug 19 '19

You have the eyes of a legendary developer 👀
Thanks, I have fixed the function name.
It's an open discussion, there's no true or false question. Appreciate your comment :)
Glad that you like my article.

Ali Al-Kaissi • Aug 18 '19

On the point Hussein, if I may add to this is the code repetition which is seeing the same block of code again and again in same/different places rather than using Functions/Classes to minimize code as much as possible.

Cheers!

hussein cheayto • Aug 18 '19

Thanks for pointing that out Ali. Yes, you're right.

Firoz Ansari • Aug 19 '19

Some good pointers! I stick with a great quote by Cory House when it's come to providing comments to the code:

Code is like humor. When you have to explain it, it’s bad.

hussein cheayto • Aug 19 '19

Hahahahaah I like that

Alex • Aug 19 '19

Bad commit messages. I can't express how much I hate commit messages which only say "fixed". It's fine if it's a small private repo, but if you are working on a large project with others involved... No, just no!