Comment by oefrha | Hacker News Reader

Comment by oefrha | Hacker News Reader

Parent Full thread

This is why I hate it when people think there are N possible cases, and in a cascading if/else construct they only check for N-1 conditions and assume the remaining all fall within the Nth case. Thing is application logic in the real world is often fuzzy, it is entirely possible to overlook certain possibilities (which may or may not arise from bugs elsewhere); and even if you have considered all cases at present, future changes to other parts of the code may inadvertently introduce new possibilities.

Personally, I always check for all N conditions, and raise an exception when "the impossible" happens; this way, the application would fail noisily instead of silently doing something unintended. However, sometimes other people would "optimize" away my last condition, which is very frustrating.

Edit: Checking for all N cases helps with readability too. Otherwise one has to figure out what exactly is the remaining case by exclusion, which is sometimes not at all obvious.

A common reason people don't do it the way you're suggesting is dogma about code coverage.

If the code has branches for each possible case, and then an "else" to cover anything else (which raises an exception indicating something impossible has happened), there is often no good way to write a test that covers the "else" logic, since the coder doesn't believe there's a way to end up going there. Or if there is a way to force it to happen, it involves jumping through hoops that are too much effort.

Some languages have good alternatives to this. Examples:

* Rust has an unreachable!() macro that tells readers and static analyzers that it should be dead code; but raises an error if executed at runtime. https://doc.rust-lang.org/std/macro.unreachable.html

* Languages with pattern-matching will either crash at compile-time if the set of clauses is non-exhaustive, or at run time if a case is missing. That is, if you don't have a catch-all clause.

Cool language feature! I’ve done this in other languages using a code style policy where all “naked” else statements and ‘default’ inside a switch statement must throw an uncaught exception or otherwise deliberately crash.

    if(option==1)
        do_somethimg();
    else
        do_something_else();

...must then be rewritten as:

    if(option==1)
        do_somethimg();
    else if(option==2)
        do_something_else();
    else
        abort();

This kind of defensive coding would have helped to prevent the famous ‘goto fail’ bug from a few years ago.

Typescript has the `never` type, which is meant for values that are never used. You can combine that with other stuff to get something _like_ `unreachable!` but it's a bit hard to get working right to be honest.

So true! Another reason against dogmas, especially concerning code coverage.

In general I’d say testing the method in a controller is a smell，and in this particular case I’d suggest GET and POST should never have been handled by the same function. If GET did nothing but render the view sending a HEAD would be harmless.

Yeah the "if GET" code raised my eyebrows a bit, I know rails can handle separate GET and POST funcs.

While you're not wrong, if I explicitly tell the framework to only accept GET and POST, I consider it a bug if any other method "gets through", particularly since the docs don't say that can happen.

As an average Ruby developer. The first 100ms I saw:

  if request.get?
    # serve authorization page HTML
  else
    # grant permissions to app
  end

I know that's really smell. Why would I put a serious statement inside `else` block?

Could you explain that further.

Why would you avoid a serious statement in an else?

Would this be more acceptable? The negative on the if

  if !request.get?

I assume the correct pattern for finite possible inputs would be something like

   if request.post? || request.put? || more...
     # do dangerous thing
   else
     # do safe thing

This of course ignores the smell which others have pointed out where with rails you would ideally have a totally different method get called in the first place for a GET vs a POST as you can have different actions for them in the controller.

Because unknown gives anxiety.

Robot:

  if !request.get?
    do_nothing
  else
    kill_one_person
  end

What are possible states for NOT `!get?`, I have to think twice or all day thinking.

The problem is that real world program may have exception state you can't think of. If you write down a set of finite state; {A,B,C,D}.

  if A
    # For A, do nothing
  else
    # for B or C or D, kill a man
  end

It's sure that if not A, it's going to be B|C|D. But what if it isn't? You could get an exceptional Z which will kill a man too. You know A better anything "else".

A cascading if/else construct can have up to 2^(N+1) possible combinations where N is the number of if-else constructs. That can become very hard to test. If you have more than two levels of nested if-else's you should probably refactor especially if it is security sensitive code. The mental model you have of your code is as complex as the code itself and there is no way that you will be able to accurately model some more complex than what you can see all at once. Scope reduction is super important if you want reliable code and function extraction is one way to achieve this. It also allows for much cleaner test setups.

Reply on news.ycombinator.com