4 ms·
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 remai
by oefrha 7y ago
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.
- femto113 7y agoIn 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.
- diminoten 7y agoYeah the "if GET" code raised my eyebrows a bit, I know rails can handle separate GET and POST funcs.
- icebraining 7y agoWhile 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.
- bobbyi_settv 7y agoA 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.
- lysium 7y agoSo true! Another reason against dogmas, especially concerning code coverage.
- progval 7y agoSome 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 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.
- ryandrake 7y agoCool 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.
- rtpg 7y agoTypescript 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.
- Existenceblinks 7y agoAs 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?
- wolco 7y agoCould 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?
- adverbly 7y agoI 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.
- Existenceblinks 7y agoBecause 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".
- jacquesm 7y agoA 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.