Hacker News Reader

> 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.

>This makes the code clearer, IMO.

Depends on how much state you need to pass between A, B1, B2 and C.

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.

Could have been worse: He could have made 1000 functions two lines each.

If the amount of state you need to pass is that significant, that is another code smell that needs its own resolution.

> 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.

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.)

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.

> 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.

> 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).