A code smell<\/em> is a hint that something has gone wrong somewhere in your code. <\/p>Wards Wiki<\/a><\/cite><\/blockquote>\n\n\n\n Yes, but also no. Code smells are “most likely” bad, but the term is often misused and misunderstood. Not all “signs of code smells” are necessarily “signs of bad code.” <\/p>\n\n\n\n The code smell<\/strong> metaphor originates from Wards Wiki<\/a>, and they stress:<\/p>\n\n\n\n Note that a CodeSmell is a hint that something might be wrong, not a certainty. A perfectly good idiom may be considered a CodeSmell because it’s often misused, or because there’s a simpler alternative that works in most cases. Calling something a CodeSmell is not an attack; it’s simply a sign that a closer look is warranted.<\/p>Wards Wiki<\/a><\/cite><\/blockquote>\n\n\n\n So while not all of these common characteristics may always indicate that a code smell is present, it is very important that all software engineers be very familiar with the different types of code smells. By knowing what code smells are and how to sniff them out, you can help yourself write clean code<\/a> by ensuring you’re not introducing these 31 code smells into your software. <\/p>\n\n\n\n\n\n Dispensable Code Smells are generally unnecessary code that should be removed from the source. They range in the form of:<\/p>\n\n\n\n Are you commenting your code? <\/strong>There’s no question it’s important to comment your code, but it’s more important to know why you’re commenting your code. <\/p>\n\n\n\n Are you commenting your code because the algorithm you created is confusing as all hell? (come on, we’ve all been there). <\/p>\n\n\n\n That right there is NOT <\/strong>the correct way to comment your code. In fact, that’s not even the correct way to code. If you need instructions to follow along with your confusing code, then it’s confusing code that needs to be refactored. <\/p>\n\n\n\n Comments in code should not have to explain how<\/em> your code works. Your code should be easy enough to understand for anyone with a bit of programming knowledge to be able to follow along. <\/p>\n\n\n\n So when should you use comments <\/strong>then? Comments are meant to explain why<\/em> your code is doing what it’s doing. Why<\/em> a certain algorithm is there or doing something may not be as clear to other programmers if they aren’t as familiar with the system. <\/p>\n\n\n\n Common concerns during a code review might reveal:<\/p>\n\n\n\n The answers to many of these types of questions may not always be clear, even with the cleanest code. Answering these types of questions or describing this type of rationale is not only completely acceptable in comments, but also encouraged to help future programmers of your code. <\/p>\n\n\n\n\n We’re all familiar with the D.R.Y. principle, which stands for Don’t Repeat Yourself. Yet many times we still find duplicate code in our code bases. Why is this?<\/p>\n\n\n\n One of the most common scenarios I’ve found is because of a rushed deadline. Often times programmers will need a “one off” solution of a function they’ve already created or seen. <\/p>\n\n\n\n However, they need to slightly change the algorithm and also not break the existing functionality that depends on it. So they will just copy and paste that code into a new function for their specific needs, either due to a short deadline or out of pure laziness. <\/p>\n\n\n\n The term duplicate code<\/strong> doesn’t have to refer to code that’s just copy and paste (or nearly copy and paste) either. Sometimes a developer may have good intentions on writing clean code but may have not even realized they just created duplicate code. <\/p>\n\n\n\n This often happens when multiple team members are working on a similar problem and not communicating properly, each creating their own version to the solution without realizing it. <\/p>\n\n\n\n Hey wait!!! If you want to learn some refactoring tips for cleaning up duplicate code<\/strong>, then I’d highly suggest you buy the book “Clean Code<\/span>” by Robert “Uncle Bob” Martin. Click here to purchase it on Amazon!<\/a><\/p>\n\n\n\n The more, the merrier? Not when it comes to constructing software!<\/p>\n\n\n\n Every additional class adds more complexity to the project. If you identify a class that isn’t entirely too useful and not pulling its own weight in the project, then consider collapsing the class or possibly combining it with an existing class (just keep in mind the Single Responsibility Principle<\/a>). <\/p>\n\n\n\n Alas, you come across a class that doesn’t appear to be hooked up to anything. Is it being used? Are there any other classes depending on this class to remain functional? Is it dead code<\/strong>???<\/p>\n\n\n\n Jeff Atwood advocates to “ruthlessly delete code that isn’t being used. That’s why we have source control systems!” on his blog over at CodingHorror<\/a>.<\/p>\n\n\n\n I tend to agree with him. It’s frustrating coming across old code that you have no clue what it’s doing in the project in the first place. Worse if you actually take the time to try and refactor any of it when it turns out it wasn’t even being used in the first place. <\/p>\n\n\n\n How does dead code end up in a project? Most of the time code becomes dead as a result of refactoring and\/or software requirements change. The developers either forget to clean up the old code or didn’t even know about the old code in the first place. <\/p>\n\n\n\n Dead code can also take the form of old commented out code. During code refactoring, some developers may leave the old code in there but commented out in fear that they may need to revert back to the old changes. Again to Jeff’s point, THAT’S THE EXACT REASON WE USE VERSION CONTROL!<\/span><\/p>\n\n\n\n Jeff’s stance on just deleting the old code makes sense. You will (should) know if it breaks anything if all of your test cases still pass after you delete the code. Plus, in the event that your test cases didn’t catch something and the application does break after deleting the presumed dead code, you can always roll back the changes in your versioning control system anyways! Then it really does give you a reason to refactor and document the code so the next developer doesn’t try and make the same mistake as you!<\/p>\n\n\n\n Speculative Generality is similar to dead code in that it’s unused code. However instead of being code that once lived and then died, speculative generality is code that never even lived in the first place. <\/p>\n\n\n\n Often times developers will put in code as a “placeholder” for a future requirement, either as directed by management or with good intentions thinking it’s what the customer\/ end user would actually want.<\/p>\n\n\n\n However, this code just ends up cluttering the already crowded project. As time goes on, these speculative classes end up getting forgotten, buried, and lost in the documentation as they never made their way into hard requirements. <\/p>\n\n\n\n As a result, no one remembers what these classes are for and they’re just left in the code base adding to the complexity, wasting valuable time as developers analyze them to try and understand their existence. <\/p>\n\n\n\n In short, don’t create classes based on speculative generality. Create classes that solve the current requirements. Nothing less, nothing more. <\/p>\n\n\n\n What is an oddball solution? Imagine having two solutions to one problem. One of these solutions is the oddball solution<\/strong>. <\/p>\n\n\n\n Often times this occurs from poorly duplicated code<\/a> or similar classes with different interfaces. More often than not, the way to fix this code is to just remove the oddball solution (the duplicate code that’s used less throughout the project). <\/p>\n\n\n\n Sometimes, you may need to keep the duplicated classes, i.e. you need to keep the oddball solution. Perhaps an existing class may provide the functionality required by a client, but its interface may not be what the client expects.<\/p>\n\n\n\n In such cases, the existing interface needs to be converted into The Adapter pattern suggests defining a wrapper class around the object with the incompatible interface. It converts the interface of a class into another interface the clients expect. The Adapter Pattern lets classes work together that couldn\u2019t otherwise because of incompatible interfaces.<\/p>\n\n\n\n\n Bloater Code Smells occur when code is way bigger than it should be, but you don’t exactly know “when” it happened, it just kind of creeps up on you. Usually, the project started out with some rock solid code, but as the life of the program continues to age, new requirements come in, and different programmers cycle through the code base, the code smells start trickling in as more and more code gets added to the same old classes. No one honors the Programmer Boy Scout Rule – leave the code base camp cleaner than you found it!<\/p>\n\n\n\n Many of the common examples of bloater code smells include:<\/p>\n\n\n\n Are you familiar with SRP (Single Responsibility Principle)? I’ve written a blog article about SRP<\/a>, and it’s also heavily documented in Uncle Bob’s Clean Code book<\/a>, as well as StackOverflow<\/a>, and many other places on the internet. Basically what I’m saying is if you don’t know the Single Responsibility Principle then you’re wrong.<\/p>\n\n\n\n As the name suggests, the SRP tells us that a single class should only do one single thing. Or as Uncle Bob puts it, should only have one single reason to change<\/em>.<\/p>\n\n\n\n The SRP is one of the simplest of the principles, and one of the hardest to get right. Conjoining responsibilities is something that we do naturally. Finding and separating those responsibilities from one another is much of what software design is really about.<\/p>Robert “Uncle Bob” Martin<\/cite><\/blockquote>\n\n\n\n I’m sure you see where I’m going with this now. Large classes are most likely the result of developers violating the Single Responsibility Principle. More often than not, a large class is doing more than one thing. <\/p>\n\n\n\n As time goes on and the program grows, classes tend to get bigger and more bloated. As new requirements come in, software engineers need to consider if the code they’re adding to an existing class really belongs in the class, or if they would benefit from creating a separate class or possibly even restructuring the existing class and breaking it down into smaller classes. <\/p>\n\n\n\n Large classes are just naturally harder to read, harder to understand, and harder to maintain. Keep your classes small and adhere to the Single Responsibility Principle. <\/p>\n\n\n\n Much like large classes, long methods and long functions are generally harder to read<\/a> than shorter ones. Everyone has a different target number for their “maximum lines of code” a function should be, some say 20 lines long, other 10 lines, and few even only 5 lines long. <\/p>\n\n\n\n Realistically though, the only important thing to keep in mind when writing functions is to ensure that each method should only manage one responsibility<\/strong>. <\/p>\n\n\n\n A method should be as long as it needs to be. However, most can agree that anything above 25 lines of code should make you start asking questions. There may be cases where a 50 or 100 line method is necessary, but more often than not you should be able to break down a function into smaller chunks to make the code more readable. <\/p>\n\n\n\n Do you have functions that take so many parameters you need to wrap the parameter list down to the next line? <\/p>\n\n\n Functions that suffer from long parameter lists are harder to read and tend to be more complex, i.e. they’re doing more than they’re supposed to be doing. <\/p>\n\n\n\n Remember, functions should be small enough to handle one and only one thing, but large enough to do that one thing well (however most of the time you should be able to handle that one thing in under 10 lines of code). <\/p>\n\n\n\n When functions or methods exceed 4+ parameters, then there’s likely 1 of 2 problems going on:<\/p>\n\n\n\n If there’s more than one thing happening in the function, then it should be pretty obvious that you need to refactor the function into smaller, more targeted functions. <\/p>\n\n\n\n If the method truly is only doing one thing and you do need all those parameters, then chances are you’d benefit from combining those parameters together in a data object. <\/p>\n\n\n Here’s a code smell that really starts to creep up on you, primitive obsession. As the name probably indicates, this occurs when there’s a bunch of unnecessary primitive variables. <\/p>\n\n\n\n The reason this one creeps up on you is because it usually only starts with one or two primitives. It’s like sneaking a piece of chocolate while on a strict diet, “just one little piece won’t hurt.”<\/em><\/p>\n\n\n Then the next release is under development and management realizes you never added fields to capture the user’s first and last name. So you add them in real quick.<\/p>\n\n\n Then more subsequent release cycles and marketing starts breathing down your neck asking why you’re not capturing email or address information.<\/p>\n\n\n I’m sure you see where this is going. No one is naturally constructing primitive obsession from the get-go (hopefully anyways). It just sort of happens as new requirements come in and the programmers don’t know where to store the data. <\/p>\n\n\n\n Take a page from the Programmer Boy Scouts Rule, leave the code base camp cleaner than you found it. When you start to see this bloating happen, take the proactive action to introduce a parameter object<\/strong> or create a whole object to represent the data and clean up this code smell. <\/p>\n\n\n\n\n Much like primitive obsession, data clumps are data that always seems to cling onto each other. They’re always hanging out together, getting themselves into trouble. It’s often hard to pull these data away from each other. You finally have the nerve to tease them “if you like them so much then why don’t you marry them?”<\/p>\n\n\n\n “Actually, what a great idea!” they rejoice. <\/p>\n\n\n\n Too lame an analogy? Ok true, well what about that old proverb then “birds of a feather flock together” or something like that? <\/p>\n\n\n\n Basically what I’m saying is, just group like-data together in a class. For much of the same reasons for many of the bloater code smells, data clumps just make your code harder to read and understand. <\/p>\n\n\n\n Why should the next programmer have to piece together different yet related data just to find out they’re all doing. Group them together in a class with a meaningful class name to represent the data. <\/p>\n\n\n\n Abusers are more commonly referred to as Object-Orientation Abusers<\/strong>. This category represents cases where the solutions do not fully exploit the possibilities of good object-oriented design. Many times, these code smells are a result of novice programmers who fail to identify when and where to put their design patterns to good use<\/a>. <\/p>\n\n\n\n Object-Orientation Abuser Code Smells:<\/p>\n\n\n\n If you’ve been a developer for any amount of time I’m sure you’ve heard (hopefully) that switch statements are often preferred over if-else statements. If you’ve been a developer for anything longer than any amount of time then I’m sure you’ve heard (hopefully) that switch statements are bad in OOP. <\/p>\n\n\n\n So are switch statements a code smell<\/strong> in Object Oriented Programming? <\/p>\n\n\n\n Here’s the thing, switch statements aren’t bad<\/em>, but they’re also not good<\/em>. It’s like saying a fork is bad but a spoon is good. Both are useful for their intended purpose. And switch statements have their own intended purpose. The problem arises when newer developer abuse<\/strong> switch statements when they should be using a design pattern instead. <\/p>\n\n\n\n Consider this scenario for example:<\/p>\n\n\n This is a great example of when not<\/em> to use a switch statement. <\/strong><\/p>\n\n\n\n (By the way, I’m about to attempt to go into a Pokemon reference when I’m not that much of a Pokemon fan. So 2 things to note before we proceed: 1. if you’re a true Pokemon fan, please don’t get too offended when I screw something up, this is only for an example, and 2. if you’re not a Pokemon fan and have no clue what the above code really means, just know that there are like hundreds of different types of Pokemon and each can do different types of things, some have “fire” action moves, others have “water” action moves, etc.) <\/p>\n\n\n\n So why is the above example bad? When starting out we only have 4 different character types here, but what happens when we get more characters? We’ll need to add more and more cases to our switch statement. <\/p>\n\n\n\n What’s worse is that this is only for fighting moves. We may have different functions for other action types where we’d have a very similar switch statement in each one. Maybe each character type talks differently, moves differently (some only walk, some can swim, some can fly), maybe they all even sleep differently. I don’t really know. The point is, we would have a duplicated switch statement structure for different functions to accommodate all the different character types. <\/p>\n\n\n\n For reference, the above code would benefit from something like the Factory Design Pattern or Strategy Design Pattern, where we can use polymorphism create different object types or apply different algorithms to an object dynamically. But that’s beyond the discussion of this blog post. Read the Design Patterns<\/a> article to learn more!<\/p>\n\n\n\n So then, when should<\/em> we use a switch statement?<\/strong> The example above shows a program that’s switching on a different object “type”. You’re essentially hardcoding something that would be better handled by polymorphism. Switching on a value<\/strong> is usually alright, as long as switching logic is confined to one place (i.e. avoid the Duplicate Code<\/a> smell). <\/p>\n\n\nAre code smells bad?<\/h3>\n\n\n\n
Dispensables<\/h2>\n\n\n\n
1. Comments<\/h3>\n\n\n\n
![\"Comments](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-comments.png\")
<\/figure><\/div>\n\n\n\n<\/a>\n\n\n\n
2. Duplicate Code<\/h3>\n\n\n\n
![\"Duplicate](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-duplicate-code.png\")
<\/figure><\/div>\n\n\n\n3. Lazy Class<\/h3>\n\n\n\n
![\"lazy](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-lazy-class.png\")
<\/figure><\/div>\n\n\n\n4. Dead Code<\/h3>\n\n\n\n
![\"dead](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-dead-code-1.png\")
<\/figure><\/div>\n\n\n\n5. Speculative Generality<\/h3>\n\n\n\n
![\"speculative](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-speculative-generality.png\")
<\/figure><\/div>\n\n\n\n6. Oddball Solution<\/h3>\n\n\n\n
![\"Oddball](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-oddball-solution.png\")
<\/figure><\/div>\n\n\n\n
another interface, which the client expects, preserving the reusability
of the existing class. The Adapter Pattern<\/a> would help cleanup these types of scenarios. <\/p>\n\n\n\n<\/a>\n\n\n\n
Bloaters<\/h2>\n\n\n\n
7. Large Class<\/h3>\n\n\n\n
![\"Large](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-large-class.png\")
<\/figure><\/div>\n\n\n\n8. Long Method<\/h3>\n\n\n\n
![\"Long](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-long-method.png\")
<\/figure><\/div>\n\n\n\n9. Long Parameter List<\/h3>\n\n\n\n
![\"Long](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-long-param-list.png\")
<\/figure><\/div>\n\n\n\npublic void<\/span> printPerson(String<\/span> firstName, String<\/span> lastName, String<\/span> middleName, String<\/span> maidenName, String<\/span> babyName, String<\/span> nickName, String<\/span> thirdFavoriteDinosaur) {\n ...\n}<\/code><\/div>Code language:<\/span> JavaScript<\/span> (<\/span>javascript<\/span>)<\/span><\/small><\/pre>\n\n\npublic void<\/span> printPerson(Person person) {\n ...\n}<\/code><\/div>Code language:<\/span> JavaScript<\/span> (<\/span>javascript<\/span>)<\/span><\/small><\/pre>\n\n\n10. Primitive Obsession<\/h3>\n\n\n\n
![\"Primitive](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-primitive-obsession.png\")
<\/figure><\/div>\n\n\n\n\/\/ user data capture form<\/span>\nString<\/span> username;<\/code><\/div>Code language:<\/span> JavaScript<\/span> (<\/span>javascript<\/span>)<\/span><\/small><\/pre>\n\n\n\/\/ user data capture form<\/span>\nString<\/span> username;\nString<\/span> firstName;\nString<\/span> lastName;<\/code><\/div>Code language:<\/span> JavaScript<\/span> (<\/span>javascript<\/span>)<\/span><\/small><\/pre>\n\n\n\/\/ user data capture form<\/span>\nString<\/span> username;\nString<\/span> firstName;\nString<\/span> lastName;\nString<\/span> addressLine1;\nString<\/span> addressLine2;\nString<\/span> city;\n...<\/code><\/div>Code language:<\/span> JavaScript<\/span> (<\/span>javascript<\/span>)<\/span><\/small><\/pre>\n\n\n![\"\"](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/Screen-Shot-2020-02-10-at-10.40.24-PM-254x300.png\")
<\/figure>\n\n\n\n<\/a>\n\n\n\n
11. Data Clumps<\/h3>\n\n\n\n
![\"data](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-data-clump.png\")
<\/figure><\/div>\n\n\n\nAbusers<\/h2>\n\n\n\n
12. Switch Statements<\/h3>\n\n\n\n
![\"switch](\"http:\/\/pragmaticways.com\/wp-content\/uploads\/2020\/02\/code-smells-switch-statements.png\")
<\/figure><\/div>\n\n\n\nfunction<\/span> void<\/span> fight<\/span>(int characterType<\/span>) <\/span>{\n switch<\/span> (characterType):\n case<\/span> CHARMANDER:\n doFireStuff();\n break<\/span>;\n case<\/span> BULBASOUR:\n doPlantStuff();\n break<\/span>;\n case<\/span> SQUIRTLE:\n doWaterStuff();\n break<\/span>;\n case<\/span> PIKACHU:\n doElectricStuff();\n break<\/span>;\n default<\/span>:\n doOtherStuff();\n }\n}<\/code><\/div>Code language:<\/span> JavaScript<\/span> (<\/span>javascript<\/span>)<\/span><\/small><\/pre>\n\n\nfunction<\/span> Character<\/span> getCharacterType<\/span>(int userSelectedCharacter<\/span>) <\/span>{\n switch<\/span> (userSelectedCharacter) {\n case<\/span> CHARMANDER:\n return<\/span> new<\/span> Charmander();\n case<\/span> BULBASOUR:\n return<\/span> new<\/span> Bulbasour();\n case<\/span> SQUIRTLE:\n return<\/span> new<\/span> Squirtle();\n