Ruby code smells

Editor's Notes

• #3 Code smells are a really cool way to describe and remember things that in some situations might cause issues in our code, flags to parts of the code that need refactoring. And there are many different names for common refactoring techniques, some language specific, but after researching most of what I saw was already described in the refactoring holly book. This presentation will not be code intensive; I’ll hardly show any code but I’ll try to help everyone identify some red flags and will comment how to improve them, most of the time using techniques name from this book.
• #4 Adventure is good when we arrive in a marvelous country like Thailand, but for our code it isn’t always good… I know that sometimes the case without else covers all the possibilities of an enumeration, but in the future we might add another item to that enumeration and break the code And for code without tests that is always a big risk of everything breaking down with any change We have all been there, I use an enum to identify tag types in a system, and that enum has a case to display a user friendly name in one or two screens in the system, after a few years the system was running fine, the editors asked for a new tag type, that we promptly added to the enumeration, and a “Undefined method for NilClass” started appearing in the system logsand this is a sample of both scenarios because we also didn’t add the new enum value to the test case before that bug… And of course, those two situations aren’t even in the refactoring list, maybe in Clean Code, but it is better to avoid both scenarios when possible And add new tests for any code you touch
• #6 There are other scenarios that can explode your code and are easy to find and fix, the easiest way to find these simpler cases is to use a tool like rubocop
• #7 Rubocop will also help your ruby code look and feel like Ruby code, and not like the juice stand in a comedy show from Latin Americain the show a homeless boy opened a juice stand, and whenever you asked for one of the juices he would say: this one is lemon juice, it tastes like gooseberry and looks like tamarind..Rubocop standard configuration will warn you before your code looks like one of those juices
• #8 I think it is a good practice to add rubocop for all your projects Gemfile, that will prevent versioning issues and make sure you always have it installed. And with a little help of husky and lint-staged JS modules, we can make sure it is executed before every commit reducing the risk of human error/forgetting to run it I think of rubocop as your everyday deodorant, it’ll keep out most mildly smells but won’t prevent you from needing a shower
• #9 This is one of the best smells on an Italian restaurant and one of the worst smiles on your code, if you think of spaghetti while looking at the code it means there are many other smaller smells, like a wale class, dolphin method, you might smell something missing, or you might smell like you’ve been there already. One of the most common consequences of this smell is that when you change one part of the code, another or many other parts will blow due to circular dependencies or simply poor organization. There isn’t one single path to fix this big mess, so I’ll break it into a few different smells to help with the refactoring.
• #10 When you find a whale class, it is probably not alone, because whales travel in pod, and they are usually composed of big methods
• #11 Dolphin methods are usually a big part of whale classes, most of the time these big methods are doing more than they should, but even if they aren’t doing more then what they should, they will make your life debugging or understanding the code a less fun and a lor harder.
• #12 In case the pod of whales has duplicated code what is really common, we can start the refactoring with an extract super class, sometimes the pod has nothing in common, in this case we collapse hierarchy, and when the whale is trying to fly, walk or do something that it shouldn’t we can extract module or extract class to move the weird behavior outside and use composition. Since in our code whales and dolphins usually are part of the same pod, we’ll also use a lot of extract method, that is one of the hardest refactoring in my opinion, because it involves passing parameters to the new method, using a good name and returning everything the old method needs to continue and if this is correctly done the new method might remove duplicated code from other parts of our big class hierarchy. Many times we combine extract method with move function to move methods to the new parent class or the newly created module. Extract variable also usually helps in extracting methods, and sometimes inline variable will make the extracted method include code duplicated code from more whales. This also means that we usually use extract/inline variable to create code duplication in many places to solve that duplication with extract method. An last but not less important, an spaghetti rarely has a good naming convention, so use rename with abundance.
• #13 Some classes remember a fruit salad, because they contain a lot of instance variables, a huge number of methods and methods with too many attributes. Sometimes this is hidden, like an activerecord class that points to a table with a huge number of fields. To improve this scenarios, to initialize a class with too many instance variables and to fix methods with too many arguments we usually accept a parameter object, what in ruby is usually replaces by a more flexible hash. For an API method it is usually better to receive an object with the specific arguments instead of having one method with many arguments in-line, it is easier to document. The refactoring for that would be a Combine Variables into Value Object, and combine arguments into Value Object
• #14 There is also code that just doesn’t smell like anything, actually it smells like it shouldn’t be there, for these we use Remove Dead Code And with a lot of dead code removed, and sometimes not dead but simply useless like return statements, our code will look a lot more like ruby and will look cleaner. Also using returns when the code isn’t really useless, we should probably rearrange the logic of that method because too many exist points is a really bad code smell that will start turning your code into an Italian pasta
• #15 There are other scenarios where your code is making your life a lot harder, for example when a change in one method reflects in many different places, or worse yet to change one field or method you need to apply the same change in various other places. This is usually a lack of encapsulation To solve this, we should use Encapsulate Variable or work on the class hierarchies with Extract Superclass or Collapse Hierarchy, usually the former is better. There is also no perfect path for something that is depended on a lot of places, but it is a good idea to have clear paths of dependencies through the application and libraries, and avoid circular dependencies with all your forces.
• #16 Those are the two most common rails anti patterns, of course there is no magic number but if your controller is doing much more than preparing data to send to the model and preparing the result to send to the view probably most of that logic should be extracted to a method and then the method moved to the model. And if the models are too fat it is very possible they are doing too many things that aren’t really related, or they become a domain dumpster Like a music model, that starts with music name, id, file name, and suddenly starts to convert the music file to wav, mp3, ogg, created a clip for that music, counts downloads, …the convert methods should be their own commands, the download counter should probably be another model, that would also help to save more information about who and when the music was downloaded, … In summary a fat model is usually doing things that should be elsewhere and a fat controller is probably doing things the model should do.
• #17 Migrations are those things that when you have multiple environments or take too long to move a feature to production will break during every deploy, not all migrations of course, but one of them will… And it is usually because a model that was used in a migration changed after the migration was created The migration doesn’t have a down method so you can’t go back to where you were before to work on the other feature and your database becomes a mess of migrated code from different branches And migrations that change models and update data at the same time are also a good source for errors and regrets. For these situations I could not apply the refactoring names from martin fowler book, but just avoid these cases that in my experience are the most common error sources in migrations.
• #18 When you are using erb views the most common anti patterns are adding model query logic in the views, that should be in the controllers, in the best case scenario the data needed for that view should get to the view in instance variables. Also views with repeated code or some really complex parts, those two scenarios have the same solution, use extract method for the complex or repeated part, and move the method to a ViewHelper This also has the side effect of allowing that bit of code to be easily tested