3 ms·

Another risk is if you add print_table() then someone else is going to find it and use it in their code, but also add a little flag to adjust the output for the

Another risk is if you add print_table() then someone else is going to find it and use it in their code, but also add a little flag to adjust the output for their use case. 12 months later you have: print_table( rows, headers = None, is_unicode = False, left_align = False, align = [], remove_emoji = None, max_width = 80, potato_mode = 7, _debug_frontend = not FLAGS.dont_debug, ellipsis_for = 0, no_print = False, )

ncann 3y ago
I think we all know at least some functions like this in a code base. All it takes is for a newcomer to come across a complex function that they need to update some logics for but also don't understand it enough to refactor, so they just added some parameters with default values and call it a day. > no_print = False love this
reedf1 3y ago
To play devil's advocate - what's the issue with this? Is print_table() + print_table_without_emoji() better than print_table(remove_emoji= False)?
- myrmidon 3y ago
  The issue is that this approach is almost guaranteed to produce basically untestable code with a myriad of invalid/nonsensical/completely broken input combinations, and its impossible to refactor, too, because you don't even know which parts of the parameter space are actually ever needed, or even how they are supposed to interact. Whenever function semantics need to change, everything degrades further because of refactoring uncertainties (=> you end up with even more parameters). This will also be extremely resistant to optimization because even finding the "happy path" is non-trivial.
- leonseled 3y ago
  As a fresh dev, I’d like to know the answer to this as well. Abstract to function w multiple params, abstract to multiple functions, no abstraction and keep as switch statement. `print_table() + print_table_without_emoji()` vs `print_table(remove_emoji= False)` vs `switch table_name: case emoji: print(table) case no_emoji: print(table no emoji)`
  - layer8 3y ago
    If callers typically know statically whether they want emoji or not (in other words, if the parameter would typically be a literal true or false at the call site), then the parameterless version is better. (Note that you can still factor out common code from the two functions into a separate private function if you like. So the parameterless version doesn’t necessarily imply code duplication.) If, on the other hand, callers tend to be parameterized themselves on whether to use emojis or not, then the parameterized version is better, because it avoids having to make a case distinction at the call site.
  - PH95VuimJjqBqy 3y ago
    It depends on the implementation, but in general you prefer the parameterless version because in theory a bug that shows up in print_table has less code it could be in (less surface area to debug). Without understanding the implementation no one can truly say which is the better approach, but this idea of "surface area for bugs" is something that should be considered when approaching these types of decisions.
- dahauns 3y ago
  Honestly? Both reek of SRP violation. Why should print_table specifically care about emoji? if needed remove the emoji, then print. if performance/table size is an issue, working via streams/generators/etc. should be on the (heh) table anyway. But if you have conceded to being in quick&dir^H^Hpragmatic-land anyway, IMO both can be ok depending on the context.
krembo 3y ago
This example looks totally legit to me as long as it preserves backward compatibility and doesnt add unnecessary junk flags
tonyedgecombe 3y ago
I just had to implement potato_mode in a report, what a rabbit hole that turned out to be.
BenFrantzDale 3y ago
That’s what tests are for. And if `print_table` is factored properly then they won’t want to add flags, they’ll make a new function out of the pieces of `print_table` that has distinct behavior of its own.

ncann 3y ago

I think we all know at least some functions like this in a code base. All it takes is for a newcomer to come across a complex function that they need to update some logics for but also don't understand it enough to refactor, so they just added some parameters with default values and call it a day. > no_print = False love this

reedf1 3y ago

To play devil's advocate - what's the issue with this? Is print_table() + print_table_without_emoji() better than print_table(remove_emoji= False)?

myrmidon 3y ago
The issue is that this approach is almost guaranteed to produce basically untestable code with a myriad of invalid/nonsensical/completely broken input combinations, and its impossible to refactor, too, because you don't even know which parts of the parameter space are actually ever needed, or even how they are supposed to interact. Whenever function semantics need to change, everything degrades further because of refactoring uncertainties (=> you end up with even more parameters). This will also be extremely resistant to optimization because even finding the "happy path" is non-trivial.
leonseled 3y ago
As a fresh dev, I’d like to know the answer to this as well. Abstract to function w multiple params, abstract to multiple functions, no abstraction and keep as switch statement. `print_table() + print_table_without_emoji()` vs `print_table(remove_emoji= False)` vs `switch table_name: case emoji: print(table) case no_emoji: print(table no emoji)`
- layer8 3y ago
  If callers typically know statically whether they want emoji or not (in other words, if the parameter would typically be a literal true or false at the call site), then the parameterless version is better. (Note that you can still factor out common code from the two functions into a separate private function if you like. So the parameterless version doesn’t necessarily imply code duplication.) If, on the other hand, callers tend to be parameterized themselves on whether to use emojis or not, then the parameterized version is better, because it avoids having to make a case distinction at the call site.
- PH95VuimJjqBqy 3y ago
  It depends on the implementation, but in general you prefer the parameterless version because in theory a bug that shows up in print_table has less code it could be in (less surface area to debug). Without understanding the implementation no one can truly say which is the better approach, but this idea of "surface area for bugs" is something that should be considered when approaching these types of decisions.
dahauns 3y ago
Honestly? Both reek of SRP violation. Why should print_table specifically care about emoji? if needed remove the emoji, then print. if performance/table size is an issue, working via streams/generators/etc. should be on the (heh) table anyway. But if you have conceded to being in quick&dir^H^Hpragmatic-land anyway, IMO both can be ok depending on the context.

krembo 3y ago

This example looks totally legit to me as long as it preserves backward compatibility and doesnt add unnecessary junk flags

tonyedgecombe 3y ago

I just had to implement potato_mode in a report, what a rabbit hole that turned out to be.

BenFrantzDale 3y ago

That’s what tests are for. And if `print_table` is factored properly then they won’t want to add flags, they’ll make a new function out of the pieces of `print_table` that has distinct behavior of its own.