5 ms·

Neat `abuse'. Personally I don't like the what I consider an anti-pattern he used in his refactored functions. I'm talking about doing this: if (condition

Neat `abuse'. Personally I don't like the what I consider an anti-pattern he used in his refactored functions. I'm talking about doing this: if (condition) return true; return false; instead of simply return condition;

catnaroek 10y ago
That's not even an anti-pattern. It's just being ignorant of the basic properties of the Boolean type.
unwind 10y ago
Yeah, I hate that with a burning passion, too. The only justification I've seen/heard is that it makes it more "debuggable", you can single-step through and see that the expected path is taken, i.e. that the condition is properly evaluated. Still, I hate it and would much rather check that some other way (by inspecting the return value before the function exits, for instance).
- pdpi 10y ago
  If you're going for debuggable, though, you still don't want the if and multiple returns. You'd be better off with something like: bool result = (condition); return result;
  - chillingeffect 10y ago
    yes, I wish everyone did this all the time. even to the point of: bool result = (condition); if( result ) { do_some_shit(); } else { do_some_different_shit(); } p.s. how did you format the code section of your post?
    - pdpi 10y ago
      Prefix with 4 spaces. :)
      - pyre 10y ago
        4 spaces is Markdown, HN only requires 2 spaces. https://news.ycombinator.com/formatdoc https://news.ycombinator.com/formatdoc
  - to3m 10y ago
    This is good, but you do still want the multiple returns as well, because then you can breakpoint one case or the other without any performance degradation when the breakpoint is on the rarer case. (Also, at least for C/C++, you generally want condition and each case on separate lines, so that you can actually put breakpoints on each thing separately - tool support for multiple independent breakpoints on a single line is still an utter shambles, and you're best off just assuming nothing supports it.) Unconditional breakpoints that aren't being hit are a lot cheaper (if they even cost anything at all - which usually they don't) than conditional breakpoints that are always being hit...
    - richardwhiuk 10y ago
      Their will be no difference in this case - the compiler will optimize the boolean foo = {complicated expression}; if (foo){ return true; } else { return false; } into return {complicated expression}, and the debugger will be forced to add a conditional breakpoint.
      - to3m 10y ago
        I've never used a debugger that's that clever (and I'm not sure debug info gives it enough to work with to deduce this anyway, not with DWARF at least) - the ones I've used just put an unconditional breakpoint on the instruction the compiler says that corresponds to, and then show it as attached to whichever line it chooses. (DWARF decrees that each instruction is from one source line, but one line may map to many instructions - so there's no perfect way of doing this backwards.) This is why it's usual to debug the unoptimized build... and why this situation is hypothetical ;) There's nothing wrong with running an optimized build inside a debugger; in fact, I recommend it, since when it crashes you've then got at least some chance of figuring out what's going on. But for the sort of work where you're putting breakpoints on one clause rather than another, or stepping through code line by line trying to figure out what's happening, it's not very good.
  - unwind 10y ago
    True, except of course the 'result' should be 'const'. Always 'const' all the things that never change after being assigned. Another pet peeve, obviously. :)
- deleted 10y ago
  [deleted]
DSMan195276 10y ago
I agree 100%. I would add that when using the standard `_Bool` type (IE. `bool` if you include `<stdbool.h>`) all assignments to it are converted to their 'logical value' first. So even if condition is an integer or pointer, if you did what you suggested the `condition` will still be converted into `true` or `false` on the return exactly the same as if you did the `if`. It's exactly the same as doing `return !!condition`. Now if you're function is not returning a `bool`, then that conversion won't happen - but of course that doesn't matter if `condition` is already a logical value of 0 or 1 (Which it is in this case). And IMO, if your value isn't a logical value I'd much rather see `return !!condition` instead of the `if` anyway (Though I concede that those not familiar with C may not immediately recognize what that syntax does).
vram22 10y ago
Common beginner mistake. (Not saying the OP is a beginner, may have had some specific reason, since it says "in his refactored functions".) Other similar ones are things like: if ((count > 10) == true) # in any language, not just C, so I'm using generic placeholder for boolean true, instead of: if (count > 10) and inability to grok stuff like: while not done (or while not found) .... # set done or found here based on some condition
- 0xfeba 10y ago
  The other 2 devs on my team (both senior to me) routinely check in code like this: function(filename) { if (!filename) { this.setState( {x : null}); } var s = filename; if (s) { this.setState({y :''}); } else { this.setState({z: false}); } } Which to me is unfinished code (I truncated variable names). I always try to cleanup my code before checking in, probably to my detriment. They both are much faster at pushing features out. But god, some of the code is just unreadable. Also--no comments. For some reason everyone at my current company refuses to use comments. And I've seen plenty of: x == true ? true : false ternaries, though in javascript, these are not necessarily redundant and are easy to mistake. A couple other favorites I've seen: if (x == 'true') ... and var data = {option: 1} var context = this; this.someMethod.bind(this, context, data, data.option)
  - vram22 10y ago
    Wow. Or rather, !wow. Such practices are all too common ... should be the rare exception, really.

catnaroek 10y ago

That's not even an anti-pattern. It's just being ignorant of the basic properties of the Boolean type.

unwind 10y ago

Yeah, I hate that with a burning passion, too. The only justification I've seen/heard is that it makes it more "debuggable", you can single-step through and see that the expected path is taken, i.e. that the condition is properly evaluated. Still, I hate it and would much rather check that some other way (by inspecting the return value before the function exits, for instance).

pdpi 10y ago
If you're going for debuggable, though, you still don't want the if and multiple returns. You'd be better off with something like: bool result = (condition); return result;
- chillingeffect 10y ago
  yes, I wish everyone did this all the time. even to the point of: bool result = (condition); if( result ) { do_some_shit(); } else { do_some_different_shit(); } p.s. how did you format the code section of your post?
  - pdpi 10y ago
    Prefix with 4 spaces. :)
    - pyre 10y ago
      4 spaces is Markdown, HN only requires 2 spaces. https://news.ycombinator.com/formatdoc https://news.ycombinator.com/formatdoc
- to3m 10y ago
  This is good, but you do still want the multiple returns as well, because then you can breakpoint one case or the other without any performance degradation when the breakpoint is on the rarer case. (Also, at least for C/C++, you generally want condition and each case on separate lines, so that you can actually put breakpoints on each thing separately - tool support for multiple independent breakpoints on a single line is still an utter shambles, and you're best off just assuming nothing supports it.) Unconditional breakpoints that aren't being hit are a lot cheaper (if they even cost anything at all - which usually they don't) than conditional breakpoints that are always being hit...
  - richardwhiuk 10y ago
    Their will be no difference in this case - the compiler will optimize the boolean foo = {complicated expression}; if (foo){ return true; } else { return false; } into return {complicated expression}, and the debugger will be forced to add a conditional breakpoint.
    - to3m 10y ago
      I've never used a debugger that's that clever (and I'm not sure debug info gives it enough to work with to deduce this anyway, not with DWARF at least) - the ones I've used just put an unconditional breakpoint on the instruction the compiler says that corresponds to, and then show it as attached to whichever line it chooses. (DWARF decrees that each instruction is from one source line, but one line may map to many instructions - so there's no perfect way of doing this backwards.) This is why it's usual to debug the unoptimized build... and why this situation is hypothetical ;) There's nothing wrong with running an optimized build inside a debugger; in fact, I recommend it, since when it crashes you've then got at least some chance of figuring out what's going on. But for the sort of work where you're putting breakpoints on one clause rather than another, or stepping through code line by line trying to figure out what's happening, it's not very good.
- unwind 10y ago
  True, except of course the 'result' should be 'const'. Always 'const' all the things that never change after being assigned. Another pet peeve, obviously. :)
deleted 10y ago
[deleted]

DSMan195276 10y ago

I agree 100%. I would add that when using the standard `_Bool` type (IE. `bool` if you include `<stdbool.h>`) all assignments to it are converted to their 'logical value' first. So even if condition is an integer or pointer, if you did what you suggested the `condition` will still be converted into `true` or `false` on the return exactly the same as if you did the `if`. It's exactly the same as doing `return !!condition`. Now if you're function is not returning a `bool`, then that conversion won't happen - but of course that doesn't matter if `condition` is already a logical value of 0 or 1 (Which it is in this case). And IMO, if your value isn't a logical value I'd much rather see `return !!condition` instead of the `if` anyway (Though I concede that those not familiar with C may not immediately recognize what that syntax does).

vram22 10y ago

Common beginner mistake. (Not saying the OP is a beginner, may have had some specific reason, since it says "in his refactored functions".) Other similar ones are things like: if ((count > 10) == true) # in any language, not just C, so I'm using generic placeholder for boolean true, instead of: if (count > 10) and inability to grok stuff like: while not done (or while not found) .... # set done or found here based on some condition

0xfeba 10y ago
The other 2 devs on my team (both senior to me) routinely check in code like this: function(filename) { if (!filename) { this.setState( {x : null}); } var s = filename; if (s) { this.setState({y :''}); } else { this.setState({z: false}); } } Which to me is unfinished code (I truncated variable names). I always try to cleanup my code before checking in, probably to my detriment. They both are much faster at pushing features out. But god, some of the code is just unreadable. Also--no comments. For some reason everyone at my current company refuses to use comments. And I've seen plenty of: x == true ? true : false ternaries, though in javascript, these are not necessarily redundant and are easy to mistake. A couple other favorites I've seen: if (x == 'true') ... and var data = {option: 1} var context = this; this.someMethod.bind(this, context, data, data.option)
- vram22 10y ago
  Wow. Or rather, !wow. Such practices are all too common ... should be the rare exception, really.