| Your extraction attempt actually proves my point: your code is more complex, is less readable, is demonstrably wrong, and destroys performance in at least two ways (I’d benchmark my “ugly” code against yours every time and win). More complex and less readable: you’ve taken the comparison out of context of the class which is being used for comparison and put it in a different class and file that has to be opened for understanding a priority comparison. Worse, that separate class and file isn’t going to be reusable for anything else, because it depends heavily on the (admittedly large, but justifiably so) public interface of MIME::Type. So, readability is made worse for the sake of a dubious improvement in testability and no value in reuse. If your editor supports code folding (it should, and you should be using it), then you don’t even have to see the implementation of `priority_compare` if you don’t need it. Demonstrably wrong: you’re calling `#simplified_value` twice. You could make that not as ugly by caching, but that’s already known to be a waste of time because your extraction… Destroys performance: `priority_compare` is used for sorting (as the code comment states outright). Creating a new object for each and every comparison is going to send sort performance to hell in a handbasket with rockets on it. It’s the same problem that plagues people who use decoration heavily: performance sucks and memory use gets ugly quick. The calling of `#simplified_value` twice doesn’t help here, and caching the value only makes memory use worse. The code comment on `priority_compare` is clear: it’s a specialized implementation of `<=>` used for sorting from MIME::Types#[]. That should immediately halt any consideration of extracting the code into an object that needs creation, and continuing on with that is a failure of good software development in favour of purity suggested by stupid rules. Even extracting it into a class method is of highly questionable value, because now you have to (as you do with your extraction) know the left and right comparison objects, rather than (safely) assuming that the undecorated version is the object itself. It’s rather the difference between operators overloading in C++ where you need to provide both left and right values vs only providing the right value because the left value is the object itself. I considered separating the if/elsif conditions to separate methods, but you still need the if/elsif conditions, and all you’re doing is pushing code around in ways that, once again, will not be used outside of `priority_compare`. I had considered implementing `priority_compare` at the call-site, but it’s used at two places in MIME::Types. So, the `priority_compare` is reused, but the component parts aren’t, really. I will admit to laying a trap with `priority_compare`: I’ve worked on this code off and on for ten years. Code Climate, being a naïve program, cannot understand how the code is going to be used, but only understands how it’s defined and measures it against some fairly simplistic and stupid metrics. They can be interesting when you are first approaching a program to understand hotspots, but their value drops to zero very quickly compared to the understanding you have from working with the code regularly. Where I think Code Climate (and similar tools; they just happen to be fairly high-profile in the Ruby community) goes wrong is that a lot of inexperienced and naïve developers tend to treat it as Truth that Must Be Followed. Many of these folks are also fans of so-called “Clean Code” techniques and otherwise lack the experience necessary to know when not to follow these sorts of rules and assume that Code Climate is good becaus it simplifies the score to a GPA/grade. |
Performance: could be. If your context is one where performance is paramount and trumps readability/maintainability, that may certainly be easier to achieve with less factored code. I spent zero time trying to optimize the code.
I suspect this design doesn't make that as hard as you make it sound, though. My team writes code in the style of my example, and we don't have problems optimizing when it's called for. Though we avoid optimizing at the expense of readability unless we see a real problem.
I find it interesting that you think my example is clearly "more complex" and "less readable". I, of course, feel it is the other way around.
The fact that you've worked (mostly on your own, going by the contributor stats) on this library for ten years will of course mean that you know your way around it.
As a visitor to the codebase, I see big classes and big chunks of code that require me to load a lot into my head and digest it before I can make sense of it. Breaking that up a little means I can make sense of a smaller piece of more self-documenting logic, then dig down to lower abstractions as needed.
If you're saying that you find your way around your code better than my refactored code, I believe you.
If you're saying you think the refactored code would be harder to understand for the average developer, I think you're dead wrong.