Hi guys, I’ve been working on a new code base lately I have to do a new change to the application so I started checking out some metrics for the class which I have to make the change I found out a flog total score of 450 and in the particular method there is an score of 192 which is obviously high for complexity there are bunch of reek reported smells also but the churn report is what strikes me and is the reason for this question; it’s none existent so I was thinking in just extract some methods to clarify the intention of the method in question; but then again also I considered to extract an entire class which handles the method’s logic; that class is an active record model if I extract it I will be able to test it in isolation. What you guys suggest in this type of situations? Should I just go ahead and add more code to satisfy the solution; which I really don’t like because of the current mess or which could be a better refactor at this stage? extract method or extract class.
Under such circumstances I’ve successfully applied a combination of both: start with extract method and group the new methods according to their scope and purpose, and then extract those groups as classes. I won’t say that this is the best approach, but I like that it forces me to: 1) understand the purpose of every line of the complicated method (because I’ll need that purpose to name the new methods that I’ll be extracting) and 2) see more clearly the dependencies and relations between the methods before having to decide how to extract them as classes.
You should try to get tests that confirm the existing functionality is working before you start making any changes. This test is going to function as a sort of integration test, because as you take the class apart into smaller classes later, it’s going to test the original functionality end-to-end. However, I wouldn’t start extracting classes yet.
Once you have those specs in place, I would start with extracting methods into private methods. Do them one at a time, and check the specs are still passing after each extraction. Along the way you will find opportunities to extract classes, but I think pulling methods out is easier first. You’ll go from one huge class with one huge method to one huge class with lots of smaller methods, which is already an improvement. Private methods that are tightly related to each other will make good candidates for refactoring into smaller classes.
Sure that was my started thinking extracting some more methods; then again I found this Class Methods Resists Refactor and I was thinking in promoting this class methods to be instance methods instead; but I really don’t know. In the end we need to have a balance between keep delivering bug fixes and features and doing refactoring I think that’s the harder part of working with legacy code.
Ah. If your huge method is a class method, then I would start off by trying to make it into an instance method or move the whole method into a new class and then start the refactor. Bryan’s blog post that you linked to there was a big impetus for my thinking in this area.
@geoffharcourt yes; well in the end I decided to extract it to a new class transform it into a new instance method and start refactoring there. This is a class method inside an Active Record model so I will just try to transform it into a new Service Object and use the already written test for that particular model as integration test to not break anything.
Also there’s something that I have to think on how to handle and is the fact that this code is using transaction call from AR I just copy the entire thing from one class to another but when I start to isolate myself from Rails probably I will have work around this.