5 ms·

I can't believe this "refactoring" is actually presented as good—creating unnecessary abstractions for a simple function. If anything, I would go in the opposi

I can't believe this "refactoring" is actually presented as good—creating unnecessary abstractions for a simple function. If anything, I would go in the opposite direction. There's no need to use parameterized variables at all. Just write out the message 3 times: private String generateGuessSentence(char candidate, int count) { if (count === 0) { return "There are no " + candidate + "s"; } else if (count === 1) { return "There is 1 " + candidate; } else { return "There are " + count + " " + candidate + "s"; } } Maybe this isn't "clean" (I'd claim it is) but it's certainly the easiest to understand.

jgalentine007 2y ago
I agree!
graypegg 2y ago
Yes! I really like "99 Bottles of OOP" by Sandi Metz. I feel like it's a version of "Clean Code" that my brain actually aligns with. In a chapter, talking about different ways you could write a class that generates verses for the song 99 bottles of beer, we end up at: https://sandimetz.com/99bottles-sample-ruby#_shameless_green https://sandimetz.com/99bottles-sample-ruby#_shameless_green The Shameless Green solution is disturbing because, although the code is easy to understand, it makes no provision for change. In this particular case, the song is so unlikely to change that betting that the code is "good enough" should pay off. However, if you pretend that this problem is a proxy for a real, production application, the proper course of action is not so clear. When you DRY out duplication or create a method to name a bit of code, you add levels of indirection that make it more abstract. In theory these abstractions make code easier to understand and change, but in practice they often achieve the opposite. One of the biggest challenges of design is knowing when to stop, and deciding well requires making judgments about code.
- vsuperpower2020 2y ago
  This book is beyond parody. I send it to people I hate and want to sabotage, along with clean code.
  - celdon25 2y ago
    So you’re actually sabotaging the people who have to maintain their code, not them.
  - graypegg 2y ago
    ...ok? Noted, you do not like the book I like.
    - Joker_vD 2y ago
      Which means that in the future not only can you two ignore each other's book recommendations, you can also note each other's book disrecommendations to find stuff you'd actually enjoy! It's a win-win!
      - graypegg 2y ago
        HAH! I kind of like the idea of a site where you can post recommendations for books, and subscribe to other people's recommendations, OR the inverse of other people's recommendations
        vsuperpower2020 2y ago
        Good idea. Other books I hate for you: Game programming patterns, Crafting Interpreters, the C# player's guide.
        em-bee 2y ago
        you mean like this place? https://www.goodreads.com/book/show/31183020-99-bottles-of-oop https://www.goodreads.com/book/show/31183020-99-bottles-of-o...
kccqzy 2y ago
I'd think your code is clean. The only change I might make is to change the if statements into a single switch with `case 0`, `case 1`, and `default`. But even without this, it's already much better than what Uncle Bob comes up with.
asattarmd 2y ago
I'd remove the else if blocks since we're returning. private String generateGuessSentence(char candidate, int count) { if (count === 0) return "There are no " + candidate + "s"; if (count === 1) return "There is 1 " + candidate; return "There are " + count + " " + candidate + "s"; }
- nj5rq 2y ago
  I feel like single-line conditionals are harder to read in bigger projects. I remove the brackets in single-statement conditionals and loops, but I still use another line.
  - theshrike79 2y ago
    Braces _always_. It just needs one tired / inexperienced / somethingelse coder to "quickly add" an extra call to the topmost if and then you'll have interesting issues. I'll take the "ugly" braces every day compared to having risky structures like that in the code at all.
    - tredigi 2y ago
      Thorough testing should catch all those "interestig" issues. But easier maintainability I also favor braces everywhere.
joevandyk 2y ago
or pattern match it: defmodule Guess do def guess_message(candidate, _count=0) do "There are no #{candidate}s" end def guess_message(candidate, _count=1) do "There is 1 #{candidate}" end def guess_message(candidate, count) do "There are #{count} #{candidate}s" end end
WalterBright 2y ago
return "There are no " + candidate + "s"; I always hated overloading + for both addition and concatenation. Hence D uses ~ for concatenation.
- happytoexplain 2y ago
  Why? It seems natural and I can't think of a case where it could be confusing off the top of my head (for strongly typed languages).
  - WalterBright 2y ago
    Algol was infamous for: "10" + 1 does that mean 11 or "101" ?
    - fragmede 2y ago
      wat https://www.destroyallsoftware.com/talks/wat https://www.destroyallsoftware.com/talks/wat
    - happytoexplain 2y ago
      That shouldn't compile (or it should be an IDE error in interpreted languages). That doesn't seem to be the fault of operator overloading.
  - mhh__ 2y ago
    + should be commutative.
    - happytoexplain 2y ago
      I disagree - I think the intuition that mathematical addition being commutative means that the + operator must be commutative is not pragmatic. Whether + must be commutative is less relevant than whether + must be mathematical. I.e. I see no problem - theoretically, practically, or semantically - with using + to combine things in programming, mathematically or otherwise. As long as it's in a strong language that doesn't support nonsense. E.g. JavaScript might not deserve operator overloading (I imagine - I rarely use JS).
jwoq9118 2y ago
I love this. The cognitive of understanding what this code does is super low, it solves the problem, presents no glaring performance issues, and isn’t trying to be cute.
wcfrobert 2y ago
This is much better. Love it when code follows the KISS principle without unnecessary abstractions or optimizations when its not needed.
cagey 2y ago
A few decades ago I refactored a different way: // this may be a mistake! English language is unorthogonal in the extreme! const char * Add_es( int count ) { return 1 == count ? "" : "es"; } const char * Add_s( int count ) { return 1 == count ? "" : "s" ; } (some English words pluralize with "es" suffix, most with "s"). It has proven surprisingly useful: Msg( "%d file%s updated when you switched back", updates, Add_s( updates ) ); Only difference vs example is mine prints "0" vs "no".
sshine 2y ago
I wanted to write a sane version of this function, and it looks exactly like this.
dmurray 2y ago
My preference would be not to refactor it at all. It works, you can figure out what it does, and the tests pass. Forget about it. Whoever wrote it, wrote it to the best of their ability to implement the requirements and style that existed at the time. The requirements changed and it now has to handle a new case? Now you can refactor it. What refactoring to choose depends on the new case you have to support - it's going to look a lot different if you need to accept a new "language" parameter than if you need to special-case another number.
- morgante 2y ago
  Agreed that I wouldn’t refactor this for no reason. That being said, the first time I had to edit this (even just do change the base string) I’d make the change.
sham1 2y ago
Sadly this only works for English. Of course, given the functional requirements, it's probably fine, but this kind of code exists IRL and it can be annoying to refactor to support more languages. For example, many languages have grammatical gender and so may also change how the word looks like. Bonus points, when the word body changes. In this example it wouldn't change radically, but the word for a file in German is "die Datei" and the plural is "die Dateien" (although apparently "das File" can be used.) Okay, this isn't that big of a deal, one might say. You just pack the word's singular and plural with the possible gender, and you're set. But no, because this isn't even the worst part! The simplest complicated case might be that you have a singular, dual, and plural. In some sense this makes sense, since "two" is pretty special in terms of amount of stuff. But then you get stufi like the following, as documented in [0]: > Three forms, special cases for numbers ending in 1 and 2, 3, 4, except those ending in 1[1-4] > > [...] > > Languages with this property include: > > Slavic family > > Russian, Ukrainian, Belarusian, Serbian, Croatian Or the six form pluralisation found in Arabic. So yeah, this sort of "complicated" pluralisation isn't just an issue if you're trying to support some comparatively very niche languages, but this is a thing one needs to think about when supporting many major world languages. And of course, how you inflect the noun you're pluralising depends on the context of the sentence and the relevant grammar. For example, the form might change if you're at a place that expects a noun in its nominative versus accusative form. And of course, there's the order of words, and so muth more, that could be mentioned. So yeah, natural language is difficult. Not quite Turing complete, but good localisation takes a lot of effort. [0]: https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html https://www.gnu.org/software/gettext/manual/html_node/Plural...
dsego 2y ago
This is so much easier to read for me personally. It immediately tells me what the test cases are. With the author's version it's not that obvious what all the possible combinations of verbIs, countStr and sIfPlural are. By eliminating redundancies the author introduces a new one, the count == 1 case is handled twice, once for verbIs and than again for sIfPlural.

jgalentine007 2y ago

I agree!

graypegg 2y ago

Yes! I really like "99 Bottles of OOP" by Sandi Metz. I feel like it's a version of "Clean Code" that my brain actually aligns with. In a chapter, talking about different ways you could write a class that generates verses for the song 99 bottles of beer, we end up at: https://sandimetz.com/99bottles-sample-ruby#_shameless_green https://sandimetz.com/99bottles-sample-ruby#_shameless_green The Shameless Green solution is disturbing because, although the code is easy to understand, it makes no provision for change. In this particular case, the song is so unlikely to change that betting that the code is "good enough" should pay off. However, if you pretend that this problem is a proxy for a real, production application, the proper course of action is not so clear. When you DRY out duplication or create a method to name a bit of code, you add levels of indirection that make it more abstract. In theory these abstractions make code easier to understand and change, but in practice they often achieve the opposite. One of the biggest challenges of design is knowing when to stop, and deciding well requires making judgments about code.

vsuperpower2020 2y ago
This book is beyond parody. I send it to people I hate and want to sabotage, along with clean code.
- celdon25 2y ago
  So you’re actually sabotaging the people who have to maintain their code, not them.
- graypegg 2y ago
  ...ok? Noted, you do not like the book I like.
  - Joker_vD 2y ago
    Which means that in the future not only can you two ignore each other's book recommendations, you can also note each other's book disrecommendations to find stuff you'd actually enjoy! It's a win-win!
    - graypegg 2y ago
      HAH! I kind of like the idea of a site where you can post recommendations for books, and subscribe to other people's recommendations, OR the inverse of other people's recommendations
      - vsuperpower2020 2y ago
        Good idea. Other books I hate for you: Game programming patterns, Crafting Interpreters, the C# player's guide.
      - em-bee 2y ago
        you mean like this place? https://www.goodreads.com/book/show/31183020-99-bottles-of-oop https://www.goodreads.com/book/show/31183020-99-bottles-of-o...

kccqzy 2y ago

I'd think your code is clean. The only change I might make is to change the if statements into a single switch with `case 0`, `case 1`, and `default`. But even without this, it's already much better than what Uncle Bob comes up with.

asattarmd 2y ago

I'd remove the else if blocks since we're returning. private String generateGuessSentence(char candidate, int count) { if (count === 0) return "There are no " + candidate + "s"; if (count === 1) return "There is 1 " + candidate; return "There are " + count + " " + candidate + "s"; }

nj5rq 2y ago
I feel like single-line conditionals are harder to read in bigger projects. I remove the brackets in single-statement conditionals and loops, but I still use another line.
- theshrike79 2y ago
  Braces _always_. It just needs one tired / inexperienced / somethingelse coder to "quickly add" an extra call to the topmost if and then you'll have interesting issues. I'll take the "ugly" braces every day compared to having risky structures like that in the code at all.
  - tredigi 2y ago
    Thorough testing should catch all those "interestig" issues. But easier maintainability I also favor braces everywhere.

joevandyk 2y ago

or pattern match it: defmodule Guess do def guess_message(candidate, _count=0) do "There are no #{candidate}s" end def guess_message(candidate, _count=1) do "There is 1 #{candidate}" end def guess_message(candidate, count) do "There are #{count} #{candidate}s" end end

WalterBright 2y ago

return "There are no " + candidate + "s"; I always hated overloading + for both addition and concatenation. Hence D uses ~ for concatenation.

happytoexplain 2y ago
Why? It seems natural and I can't think of a case where it could be confusing off the top of my head (for strongly typed languages).
- WalterBright 2y ago
  Algol was infamous for: "10" + 1 does that mean 11 or "101" ?
  - fragmede 2y ago
    wat https://www.destroyallsoftware.com/talks/wat https://www.destroyallsoftware.com/talks/wat
  - happytoexplain 2y ago
    That shouldn't compile (or it should be an IDE error in interpreted languages). That doesn't seem to be the fault of operator overloading.
- mhh__ 2y ago
  + should be commutative.
  - happytoexplain 2y ago
    I disagree - I think the intuition that mathematical addition being commutative means that the + operator must be commutative is not pragmatic. Whether + must be commutative is less relevant than whether + must be mathematical. I.e. I see no problem - theoretically, practically, or semantically - with using + to combine things in programming, mathematically or otherwise. As long as it's in a strong language that doesn't support nonsense. E.g. JavaScript might not deserve operator overloading (I imagine - I rarely use JS).

jwoq9118 2y ago

I love this. The cognitive of understanding what this code does is super low, it solves the problem, presents no glaring performance issues, and isn’t trying to be cute.

wcfrobert 2y ago

This is much better. Love it when code follows the KISS principle without unnecessary abstractions or optimizations when its not needed.

cagey 2y ago

A few decades ago I refactored a different way: // this may be a mistake! English language is unorthogonal in the extreme! const char * Add_es( int count ) { return 1 == count ? "" : "es"; } const char * Add_s( int count ) { return 1 == count ? "" : "s" ; } (some English words pluralize with "es" suffix, most with "s"). It has proven surprisingly useful: Msg( "%d file%s updated when you switched back", updates, Add_s( updates ) ); Only difference vs example is mine prints "0" vs "no".

sshine 2y ago

I wanted to write a sane version of this function, and it looks exactly like this.

dmurray 2y ago

My preference would be not to refactor it at all. It works, you can figure out what it does, and the tests pass. Forget about it. Whoever wrote it, wrote it to the best of their ability to implement the requirements and style that existed at the time. The requirements changed and it now has to handle a new case? Now you can refactor it. What refactoring to choose depends on the new case you have to support - it's going to look a lot different if you need to accept a new "language" parameter than if you need to special-case another number.

morgante 2y ago
Agreed that I wouldn’t refactor this for no reason. That being said, the first time I had to edit this (even just do change the base string) I’d make the change.

sham1 2y ago

Sadly this only works for English. Of course, given the functional requirements, it's probably fine, but this kind of code exists IRL and it can be annoying to refactor to support more languages. For example, many languages have grammatical gender and so may also change how the word looks like. Bonus points, when the word body changes. In this example it wouldn't change radically, but the word for a file in German is "die Datei" and the plural is "die Dateien" (although apparently "das File" can be used.) Okay, this isn't that big of a deal, one might say. You just pack the word's singular and plural with the possible gender, and you're set. But no, because this isn't even the worst part! The simplest complicated case might be that you have a singular, dual, and plural. In some sense this makes sense, since "two" is pretty special in terms of amount of stuff. But then you get stufi like the following, as documented in [0]: > Three forms, special cases for numbers ending in 1 and 2, 3, 4, except those ending in 1[1-4] > > [...] > > Languages with this property include: > > Slavic family > > Russian, Ukrainian, Belarusian, Serbian, Croatian Or the six form pluralisation found in Arabic. So yeah, this sort of "complicated" pluralisation isn't just an issue if you're trying to support some comparatively very niche languages, but this is a thing one needs to think about when supporting many major world languages. And of course, how you inflect the noun you're pluralising depends on the context of the sentence and the relevant grammar. For example, the form might change if you're at a place that expects a noun in its nominative versus accusative form. And of course, there's the order of words, and so muth more, that could be mentioned. So yeah, natural language is difficult. Not quite Turing complete, but good localisation takes a lot of effort. [0]: https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html https://www.gnu.org/software/gettext/manual/html_node/Plural...

dsego 2y ago

This is so much easier to read for me personally. It immediately tells me what the test cases are. With the author's version it's not that obvious what all the possible combinations of verbIs, countStr and sIfPlural are. By eliminating redundancies the author introduces a new one, the count == 1 case is handled twice, once for verbIs and than again for sIfPlural.