Hacker News new | past | comments | ask | show | jobs | submit login

Wrong: I'm sure it is. I did it without tests and didn't go through the logic carefully since that wasn't at all the point. The design was.

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.




Re: wrong: it's not only wrong, but your admission that you didn't go through the logic carefully (that is, you didn't pay attention to the comments, you didn't pay attention to the spots where it was called, etc.) says that you didn't actually pay attention to the design of the system. You refactored for a nonsensical version of “object purity” (trying to satisfy for those metrics) instead of correctness. You claim to have tried to make a point about the design and failed, because your refactoring at best did not help readability, didn't actually improve any of the metrics that Code Climate reports about, and objectively made the code perform worse in the context of how it's called.

This is what happens when you refactor in a vacuum, and the danger of following tools like CC or even the “advice” given in the article at the top of this chain. I would never let a developer who works with me get away with the sort of refactoring attempt you did.

As far as your code being more complex and less readable, you have traded minor depth complexity for greater breadth complexity without simplifying the main comparison at all, and (as I keep pointing out) made the performance worse, to boot. Your code is less readable because now I've got some kind of comparison object that I have to instantiate—and it takes two objects called “one” and “two”. Now, when looking at your priority comparison object, I have to jump between mime/type.rb and mime/type/priority_comparison.rb to understand the data values that are being compared.

The reality is that the MIME::Type class is dead simple: it's a validated data object with the self-documenting descriptive properties. MIME::Types is even simpler: it's a container that knows how to search for its contained class. I've done some fairly aggressive refactoring (it just used to be two files) to extract parsing and file interaction from the main classes, and what's left is just what's required to interact with the data provided by a MIME::Type and to work with a MIME::Type collection (although some of it is going away because the previous on-disk data representation was too anemic, and the current on-disk data representation provides just what's needed in a fairly efficient manner).

That you look at these and say “big classes!” says much more about your own biases than about the code. mime-types provides very little deep functionality, but a lot of documented and self-documented code. It just happens that the data included has a lot of attributes, and I'm a big believer in backwards compatibility. Next year, when I release mime-types 3.0, I'll be removing the deprecated functionality—and it'll cut the code in MIME::Type by about 35-40%, but it will still leave things that will trip up naïve complexity advice and refactorers.

I misspoke, though, about improvements to priority_compare: when I feel comfortable making mime-types use refinements, then I would probably make an in-place refinement of Nil to support #<=>. That probably won't happen until 2015 or 2016, though—I have to keep supporting versions of Ruby that don't support refinements until after the interpreter version EOLs.




Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | Legal | Apply to YC | Contact

Search: