7 ms·

Do you have an example of a public controller method larger than even 25 lines that does its job clearly?

by dablweb 13y ago

halostatue 13y ago
Controller method that I can share? No. This method in mime-types is reported by Code Climate as a code smell. They're wrong: it's the smallest it can possibly be while still correctly performing the necessary goal; any smaller, and you have to break it into multiple smaller methods that provide no value except keeping Code Climate happy. https://github.com/halostatue/mime-types/blob/master/lib/mime/type.rb#L155 https://github.com/halostatue/mime-types/blob/master/lib/mim... The method is ~35 lines long. There are other cases I can provide from open source work (https://github.com/halostatue/mime-types/blob/master/lib/mime/types/loader.rb#L126 https://github.com/halostatue/mime-types/blob/master/lib/mim... is a good example: deprecated code, parser for a file type where splitting into multiple methods only complicates the logic flow). It is true that the larger a method gets the less readable, understandable, and clear it gets—but that should never translate into the sort of nonsensical request you made, asking about maximum LOC. Asking that paints you as a prescriptivist who doesn't bother understanding the context the code requires. I've been writing software for a long time, and while I try to write methods as short, clear, composable “paragraphs”, I sometimes will write something much longer than is readable because I can't figure out a meaningful way to break it down. That comes over time and reading and interaction with the code. The best developers in my experience are pragmatic. They are aware of design patterns, but don't treat them like sewing patterns. They are aware of development practices, but don't treat them like holy writ. The GoF design patterns book is an excellent descriptivist treatise, but then people started treating it like a cookbook and looked for reasons to implement Patterns everywhere, rather than extracting the patterns from their code. Blog posts like this one (that tell me that I should use an Interactor) pattern are actively dangerous, because they provide dicta without properly explaining the pain that the pattern evolved to solve, or the proper evolution of the pattern. The current code base I work on shows a lot of evidence of Rails Fads just like this one, and I'm trying to get my team to step back and ask why we do things certain ways and to go back to first principles. Don't just reach for a Presenter because it's what was done before. Don't even reach for an ActiveModel::Serializer because it's what we're preferring now for API representations. Figure out your problem. State it clearly. Write the solution clearly. Find code that works similarly and figure out (a) if and (b) how they can be extracted into common code. There is a cost to adopting things like Presenters and making smaller methods: your interface becomes larger. You can complain all you like of large files and functions, but code bases that have large numbers of classes whose purpose aren't clear…are harder to navigate and understand. (I have, in the current code base, unextracted code from external classes when that external class is used in one place and it makes the behaviour more understandable. It also provides a better place to understand where similar behaviour may appear later so that we can properly extract code if and when it is necessary later.)
- henrikn 13y ago
  > They're wrong: it's the smallest it can possibly be while still correctly performing the necessary goal; any smaller, and you have to break it into multiple smaller methods that provide no value except keeping Code Climate happy. Wrong? Smallest it can possibly be? Those are strong claims. I definitely disagree with Code Climate from time to time. But I try to keep an open mind. I've found that trying to follow dumb rules can be enlightening, even when my first reaction is that I disagree. This goes for Code Climate as well as arbitrary rules, such as Sandi Metz': https://gist.github.com/henrik/4509394 https://gist.github.com/henrik/4509394 I looked at your `priority_compare`, for example. As someone who did not write that code, I find it a little hard to follow. It has conditionals four levels deep (counting the ternaries). Your `Mime::TYPE` class is quite long, as is its test. I would let the Code Climate feedback inspire me to attempt to extract a method object in this case, along these lines: https://gist.github.com/henrik/9355101 https://gist.github.com/henrik/9355101 That would result in more code overall, but smaller objects on average, and to my eyes readability is improved because the complexity is a little abstracted. Do you truly feel it provides no value?
  - halostatue 13y ago
    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.

Do you have an example of a public controller method larger than even 25 lines that does its job clearly?

by dablweb 13y ago

Do you have an example of a public controller method larger than even 25 lines that does its job clearly?

Controller method that I can share? No. This method in mime-types is reported by Code Climate as a code smell. They're wrong: it's the smallest it can possibly be while still correctly performing the necessary goal; any smaller, and you have to break it into multiple smaller methods that provide no value except keeping Code Climate happy. https://github.com/halostatue/mime-types/blob/master/lib/mime/type.rb#L155 https://github.com/halostatue/mime-types/blob/master/lib/mim... The method is ~35 lines long. There are other cases I can provide from open source work (https://github.com/halostatue/mime-types/blob/master/lib/mime/types/loader.rb#L126 https://github.com/halostatue/mime-types/blob/master/lib/mim... is a good example: deprecated code, parser for a file type where splitting into multiple methods only complicates the logic flow). It is true that the larger a method gets the less readable, understandable, and clear it gets—but that should never translate into the sort of nonsensical request you made, asking about maximum LOC. Asking that paints you as a prescriptivist who doesn't bother understanding the context the code requires. I've been writing software for a long time, and while I try to write methods as short, clear, composable “paragraphs”, I sometimes will write something much longer than is readable because I can't figure out a meaningful way to break it down. That comes over time and reading and interaction with the code. The best developers in my experience are pragmatic. They are aware of design patterns, but don't treat them like sewing patterns. They are aware of development practices, but don't treat them like holy writ. The GoF design patterns book is an excellent descriptivist treatise, but then people started treating it like a cookbook and looked for reasons to implement Patterns everywhere, rather than extracting the patterns from their code. Blog posts like this one (that tell me that I should use an Interactor) pattern are actively dangerous, because they provide dicta without properly explaining the pain that the pattern evolved to solve, or the proper evolution of the pattern. The current code base I work on shows a lot of evidence of Rails Fads just like this one, and I'm trying to get my team to step back and ask why we do things certain ways and to go back to first principles. Don't just reach for a Presenter because it's what was done before. Don't even reach for an ActiveModel::Serializer because it's what we're preferring now for API representations. Figure out your problem. State it clearly. Write the solution clearly. Find code that works similarly and figure out (a) if and (b) how they can be extracted into common code. There is a cost to adopting things like Presenters and making smaller methods: your interface becomes larger. You can complain all you like of large files and functions, but code bases that have large numbers of classes whose purpose aren't clear…are harder to navigate and understand. (I have, in the current code base, unextracted code from external classes when that external class is used in one place and it makes the behaviour more understandable. It also provides a better place to understand where similar behaviour may appear later so that we can properly extract code if and when it is necessary later.)

henrikn 13y ago
> They're wrong: it's the smallest it can possibly be while still correctly performing the necessary goal; any smaller, and you have to break it into multiple smaller methods that provide no value except keeping Code Climate happy. Wrong? Smallest it can possibly be? Those are strong claims. I definitely disagree with Code Climate from time to time. But I try to keep an open mind. I've found that trying to follow dumb rules can be enlightening, even when my first reaction is that I disagree. This goes for Code Climate as well as arbitrary rules, such as Sandi Metz': https://gist.github.com/henrik/4509394 https://gist.github.com/henrik/4509394 I looked at your `priority_compare`, for example. As someone who did not write that code, I find it a little hard to follow. It has conditionals four levels deep (counting the ternaries). Your `Mime::TYPE` class is quite long, as is its test. I would let the Code Climate feedback inspire me to attempt to extract a method object in this case, along these lines: https://gist.github.com/henrik/9355101 https://gist.github.com/henrik/9355101 That would result in more code overall, but smaller objects on average, and to my eyes readability is improved because the complexity is a little abstracted. Do you truly feel it provides no value?
- halostatue 13y ago
  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.