4 ms·

I'm afraid the article has left me unconvinced. I'm open to having my mind changed on the matter, though. The point of passing boolean params instead of named

by tgb 10y ago

I'm afraid the article has left me unconvinced. I'm open to having my mind changed on the matter, though. The point of passing boolean params instead of named functions is that most of the time there is shared code between the two paths and not literally all the code is enclosed in either the if or the else block. If the author was intending just to restrict to that one specific case where there was no overlap whatsoever, then I guess I would agree but I'm not sure how often that happens. Polymorphism has some advantages but it's certainly got it's own troubles in terms of understand-ability. The author links to an article which even states "I think that we hate conditionals more than we should. Introducing polymorphism too soon will complicate your code, making it hard to understand and test." It then goes on to give an example where it says polymorphism is actually the right choice, to avoid two switches. I'd say that at least this trivial example would be best handled by some data-driven approach. Look up salaries in a database instead. The inline statement one can certainly be better in some cases, but the example given is quite poor. What does "foo && bar || baz" mean? Well, now I have to remember the order of operations ... 'or' after 'and' I guess. Surely this is easier to misread than the if statement example, which in my mind closely matches how I think. At least it should be "(foo && bar) || baz". But even then, that only works when the actual return type is boolean and there are no state changes made. I don't really see how using an Optional type would remove the if(null) checks. It just makes the meaning explicit, which is good but not remedying the original problem. 5 seems nice.

losvedir 10y ago
> I don't really see how using an Optional type would remove the if(null) checks. It just makes the meaning explicit, which is good but not remedying the original problem. Yeah, the benefit is not really the Option (which you still have to check), the benefit is when there's no Option. I'm not really sold on there being a benefit in Java (looks like those are the code samples in the article?), but in languages where `null` is not a part of the type, and your parameter is just a plain, `int`, say, then you know you don't need to check if it's `null`. In other languages, you either have to always check, or rely on a convention or code flow that has the null checks happening earlier.
wereHamster 10y ago
> I don't really see how using an Optional type would remove the if(null) checks. It just makes the meaning explicit, which is good but not remedying the original problem. If you always use `Option` (or whatever equivalent your language/base library provides) and banish `null` from your source code you've effectively avoided all null pointer exceptions and can therefore remove all null checks.
- thaumasiotes 10y ago
  Well, no, you still need the null checks. You've eliminated null pointer exceptions by guaranteeing that null is always checked for before the value is used -- you're still checking, but you're now immune to unanticipated crashes. (That would have resulted from null dereferences, that is. You can still have other bugs.)
  - lmm 10y ago
    You need checks but only in the cases where you need checks. More importantly, nullable values are very obvious in the code (and should very much be the exception rather than the rule), so the cases where you need to think about "what if that's null" are cases you actually think about.
mdpopescu 10y ago
> The point of passing boolean params instead of named functions is that most of the time there is shared code between the two paths and not literally all the code is enclosed in either the if or the else block If you have this method(bool arg) { // block A if (arg) { // block B1 } else { // block B2 } // block C } you can replace it with method1() { A(); B1(); C(); } method2() { A(); B2(); C(); } where A(), B1(), B2() and C() are new methods that contain the previous blocks. This makes the code clearer, IMO.
- Kristine1975 10y ago
  >This makes the code clearer, IMO. Depends on how much state you need to pass between A, B1, B2 and C.
  - majewsky 10y ago
    It also depends on the semantic value of A, B1, B2 and C. If you cannot express them as senseful steps of your overall algorithm, then you are bound to end up with very confusing method names like calculateFooAndCreateBarAndAlsoInitializeBaz, or (even worse) prepareForFoo. EDIT: By the way, an anecdote. Years ago, I was working with an offshore developer who was producing huge scripts without any modularization whatsoever. In particular, we told him that a certain 2000-line script needed to be modularized, so he should break out steps into functions etc. He produced a new version containing two functions of 1000 lines each. The first function would prepare everything, then tail-call the second function with 27 arguments, most of them large intermediate data structures.
    - Kristine1975 10y ago
      Could have been worse: He could have made 1000 functions two lines each.
  - jasonlotito 10y ago
    If the amount of state you need to pass is that significant, that is another code smell that needs its own resolution.
lmm 10y ago
> The point of passing boolean params instead of named functions is that most of the time there is shared code between the two paths and not literally all the code is enclosed in either the if or the else block. If the author was intending just to restrict to that one specific case where there was no overlap whatsoever, then I guess I would agree but I'm not sure how often that happens. The cases where there's a lot of commonality are cases for polymorphism instead. If you have something like: def doSomething(useCache: Boolean) = { //long and complicated function if(useCache) cache.lookup(foo) else calculate(foo) //more long and complicated stuff } then it's probably worth breaking that out as trait FooProvider { def get(foo: String): Foo } object CachedFooProvider extends FooProvider { def get(foo: String) = cache.lookup(foo) } object CalculatingFooProvider extends FooProvider { def get(foo: String) = calculate(foo) } and passing the FooProvider instead - that way you pass something with a clear semantic meaning rather than a bare Boolean. > I'd say that at least this trivial example would be best handled by some data-driven approach. Look up salaries in a database instead. Very much disagree. Every time I've seen a program do logic based on what was in the database it's been very hard to debug or reason about. Anything you can possibly do to make it unit-testable instead of needing to test against a prod database dump is worth it. > Surely this is easier to misread than the if statement example, which in my mind closely matches how I think. Disagree. The if looks like control flow when it's not actually control flow. If you're just doing Boolean logic, make it look like Boolean logic. > But even then, that only works when the actual return type is boolean and there are no state changes made. Yes, that's exactly the point. It's well worth forcing those things into different functions, so that you can inspect what's going on. Particularly when debugging, that means you can step over the conditional and see what the result was, rather than having to step through each if because you never know when one of the branches might actually do something. > I don't really see how using an Optional type would remove the if(null) checks. It just makes the meaning explicit, which is good but not remedying the original problem. Optionals have polymorphic methods so you can express most common use cases directly rather than having to branch, i.e. rather than: val user = if(null == userId) null else userRepository.lookup(userId) you can just do val user = userId flatMap userRepository.lookup map/flatMap/foreach each have their own specific semantics so it's easy to see what's going on, whereas an "if(null == userId)" could mean anything. (If your logic really doesn't fit into any of the standard use cases then you may need to use an if even with an optional, but such cases will stand out when reading the code - as they should).

by tgb 10y ago

losvedir 10y ago

> I don't really see how using an Optional type would remove the if(null) checks. It just makes the meaning explicit, which is good but not remedying the original problem. Yeah, the benefit is not really the Option (which you still have to check), the benefit is when there's no Option. I'm not really sold on there being a benefit in Java (looks like those are the code samples in the article?), but in languages where `null` is not a part of the type, and your parameter is just a plain, `int`, say, then you know you don't need to check if it's `null`. In other languages, you either have to always check, or rely on a convention or code flow that has the null checks happening earlier.

wereHamster 10y ago

> I don't really see how using an Optional type would remove the if(null) checks. It just makes the meaning explicit, which is good but not remedying the original problem. If you always use `Option` (or whatever equivalent your language/base library provides) and banish `null` from your source code you've effectively avoided all null pointer exceptions and can therefore remove all null checks.

thaumasiotes 10y ago
Well, no, you still need the null checks. You've eliminated null pointer exceptions by guaranteeing that null is always checked for before the value is used -- you're still checking, but you're now immune to unanticipated crashes. (That would have resulted from null dereferences, that is. You can still have other bugs.)
- lmm 10y ago
  You need checks but only in the cases where you need checks. More importantly, nullable values are very obvious in the code (and should very much be the exception rather than the rule), so the cases where you need to think about "what if that's null" are cases you actually think about.

mdpopescu 10y ago

> The point of passing boolean params instead of named functions is that most of the time there is shared code between the two paths and not literally all the code is enclosed in either the if or the else block If you have this method(bool arg) { // block A if (arg) { // block B1 } else { // block B2 } // block C } you can replace it with method1() { A(); B1(); C(); } method2() { A(); B2(); C(); } where A(), B1(), B2() and C() are new methods that contain the previous blocks. This makes the code clearer, IMO.

Kristine1975 10y ago
>This makes the code clearer, IMO. Depends on how much state you need to pass between A, B1, B2 and C.
- majewsky 10y ago
  It also depends on the semantic value of A, B1, B2 and C. If you cannot express them as senseful steps of your overall algorithm, then you are bound to end up with very confusing method names like calculateFooAndCreateBarAndAlsoInitializeBaz, or (even worse) prepareForFoo. EDIT: By the way, an anecdote. Years ago, I was working with an offshore developer who was producing huge scripts without any modularization whatsoever. In particular, we told him that a certain 2000-line script needed to be modularized, so he should break out steps into functions etc. He produced a new version containing two functions of 1000 lines each. The first function would prepare everything, then tail-call the second function with 27 arguments, most of them large intermediate data structures.
  - Kristine1975 10y ago
    Could have been worse: He could have made 1000 functions two lines each.
- jasonlotito 10y ago
  If the amount of state you need to pass is that significant, that is another code smell that needs its own resolution.

lmm 10y ago

> The point of passing boolean params instead of named functions is that most of the time there is shared code between the two paths and not literally all the code is enclosed in either the if or the else block. If the author was intending just to restrict to that one specific case where there was no overlap whatsoever, then I guess I would agree but I'm not sure how often that happens. The cases where there's a lot of commonality are cases for polymorphism instead. If you have something like: def doSomething(useCache: Boolean) = { //long and complicated function if(useCache) cache.lookup(foo) else calculate(foo) //more long and complicated stuff } then it's probably worth breaking that out as trait FooProvider { def get(foo: String): Foo } object CachedFooProvider extends FooProvider { def get(foo: String) = cache.lookup(foo) } object CalculatingFooProvider extends FooProvider { def get(foo: String) = calculate(foo) } and passing the FooProvider instead - that way you pass something with a clear semantic meaning rather than a bare Boolean. > I'd say that at least this trivial example would be best handled by some data-driven approach. Look up salaries in a database instead. Very much disagree. Every time I've seen a program do logic based on what was in the database it's been very hard to debug or reason about. Anything you can possibly do to make it unit-testable instead of needing to test against a prod database dump is worth it. > Surely this is easier to misread than the if statement example, which in my mind closely matches how I think. Disagree. The if looks like control flow when it's not actually control flow. If you're just doing Boolean logic, make it look like Boolean logic. > But even then, that only works when the actual return type is boolean and there are no state changes made. Yes, that's exactly the point. It's well worth forcing those things into different functions, so that you can inspect what's going on. Particularly when debugging, that means you can step over the conditional and see what the result was, rather than having to step through each if because you never know when one of the branches might actually do something. > I don't really see how using an Optional type would remove the if(null) checks. It just makes the meaning explicit, which is good but not remedying the original problem. Optionals have polymorphic methods so you can express most common use cases directly rather than having to branch, i.e. rather than: val user = if(null == userId) null else userRepository.lookup(userId) you can just do val user = userId flatMap userRepository.lookup map/flatMap/foreach each have their own specific semantics so it's easy to see what's going on, whereas an "if(null == userId)" could mean anything. (If your logic really doesn't fit into any of the standard use cases then you may need to use an if even with an optional, but such cases will stand out when reading the code - as they should).