9 ms·

The author hasn't learned from past coding practice mistakes. Having multiple exit points makes the code harder to comprehend and figure out what it's doing. A

The author hasn't learned from past coding practice mistakes. Having multiple exit points makes the code harder to comprehend and figure out what it's doing. A return in the middle of a method makes the code more obtuse, not easier to read. Even if each method is clean and does one thing, reading through multiple functions and figuring out all the return points and where things stop is a huge enabler of bugs. Personally I also find the use of negative clauses if !canDrink(age) As a bad practice. However, since the author is attempting to not use 'else' he has to resort to this. Basically all his alternative cases have to be a negative if, or the method name needs to be negative if cantDrink(age) Both of these scenarios create harder to read code. Comparing the below code, 'a)' is simpler to read and understand. // a) if canDrink(age) { okGiveBeer(to: name) } else { underAgeAlert(to: name) } // vs // b) if !canDrink(age) { underAgeAlert(to: name) return } okGiveBeer(to: name)

segmondy 6y ago
If I'm really trying to capture rules. I'll write that as. Sometimes it's easier to figure out complex rules, if there's 1 rule for every action instead of nested if else clauses. if canDrink(age) { okGiveBeer() return; } if underAge(age) { underAgeAlert() return } alert('Should never be here...')
- yongjik 6y ago
  Sorry, this is bad code. In the original version it's clear that there's only two possible outcomes: either you can drink, or you can't. Your version uses two different conditions, which makes four possibilities, which makes the code harder to reason because every reader must stop and think "Wait, can someone be underage and still allowed to drink? Or vice versa?" Eliminate edge cases, especially those that should not arise in practice.
- 100011_100001 6y ago
  Single entry, single exit is important. Your code snippet essentially have three (3) exits. The two returns and the alert, which we can assume it throws an Exception or error that halts the code. This is problematic because you have added excess code that doesn't help readability and makes bugs harder to track if an issue does come up. If anyone wants to add more rules the code will get even worse. Instinctually you recognize this because you added 'alert' it's your way of saying, if I have a bug I want to do something about it. Why is that? It's because you recognize that both if clauses might be false. If the alert triggers is it because 'canDrink' has a bug or 'underAge'? Let's say 'underAge(age)' returns true if age < 21, because you live in the United States. That also implies that 'canDrink(age), returns true if age >= 21. Hopefully you can see that both methods test the same thing. That usually makes my spider sense tingle, I am doing something programmatically wrong. However for the sake of this example let's say this got merged. New feature comes in, the age of drinking in another country is 16. We need to support their localization. If you updated only 'canDrink(age)' method (age >= 16), everything will work as expected, the if would be true and give them beer. If you only updated 'underAge(age)' (age < 16) you would have problems, 'canDrink' would return false (16 is not greater than 21) and underAge would return false (16 is not less than 16), and you would hit your alert. However if you were using a simple if/else clause if canDrink(age) { okGiveBeer() } else { underAgeAlert() } There is one method to deal with, 'canDrink'. Which means for the example of need to localize to country X with drinking age of 16 would have a single method to update. That reduces the chance of bugs. If bugs do appear they are easier to locate (is my if evaluating true when it's not? Why?). As an added benefit you have less lines of code, there is no code jumps or incomplete flows and you never need an alert in the end just in case.
- rufus_foreman 6y ago
  canDrink(age) ? okGiveBeer() : underAgeAlert()
Spivak 6y ago
In general you're right but I think for the specific pattern of checking preconditions and handling errors the author is right. if file_exists(path) { # use path ... db = get_conn() if db.is_connected() { # use db ... } else { ret = new Error("db") } } else { ret = new Error("path") } return ret Every check we need to do increases our nesting and makes it look like it's got lots of different paths when really it has exactly one happy path and many error paths. if ! file_exists(path) [ return new Error("path") } # use path ... db = get_conn() if ! db.is_connected() { return new Error("db") } # use db ... This is exactly my rule -- only if/else if it's the happy path that's branching.
- 100011_100001 6y ago
  That's a fair rebuttal. Depending on the situation though 'if not this - throw error' can be mitigated. It's one of the reasons why I try to avoid nulls coming into the code, otherwise you have to null check everywhere.

segmondy 6y ago

If I'm really trying to capture rules. I'll write that as. Sometimes it's easier to figure out complex rules, if there's 1 rule for every action instead of nested if else clauses. if canDrink(age) { okGiveBeer() return; } if underAge(age) { underAgeAlert() return } alert('Should never be here...')

yongjik 6y ago
Sorry, this is bad code. In the original version it's clear that there's only two possible outcomes: either you can drink, or you can't. Your version uses two different conditions, which makes four possibilities, which makes the code harder to reason because every reader must stop and think "Wait, can someone be underage and still allowed to drink? Or vice versa?" Eliminate edge cases, especially those that should not arise in practice.
100011_100001 6y ago
Single entry, single exit is important. Your code snippet essentially have three (3) exits. The two returns and the alert, which we can assume it throws an Exception or error that halts the code. This is problematic because you have added excess code that doesn't help readability and makes bugs harder to track if an issue does come up. If anyone wants to add more rules the code will get even worse. Instinctually you recognize this because you added 'alert' it's your way of saying, if I have a bug I want to do something about it. Why is that? It's because you recognize that both if clauses might be false. If the alert triggers is it because 'canDrink' has a bug or 'underAge'? Let's say 'underAge(age)' returns true if age < 21, because you live in the United States. That also implies that 'canDrink(age), returns true if age >= 21. Hopefully you can see that both methods test the same thing. That usually makes my spider sense tingle, I am doing something programmatically wrong. However for the sake of this example let's say this got merged. New feature comes in, the age of drinking in another country is 16. We need to support their localization. If you updated only 'canDrink(age)' method (age >= 16), everything will work as expected, the if would be true and give them beer. If you only updated 'underAge(age)' (age < 16) you would have problems, 'canDrink' would return false (16 is not greater than 21) and underAge would return false (16 is not less than 16), and you would hit your alert. However if you were using a simple if/else clause if canDrink(age) { okGiveBeer() } else { underAgeAlert() } There is one method to deal with, 'canDrink'. Which means for the example of need to localize to country X with drinking age of 16 would have a single method to update. That reduces the chance of bugs. If bugs do appear they are easier to locate (is my if evaluating true when it's not? Why?). As an added benefit you have less lines of code, there is no code jumps or incomplete flows and you never need an alert in the end just in case.
rufus_foreman 6y ago
canDrink(age) ? okGiveBeer() : underAgeAlert()

Spivak 6y ago

In general you're right but I think for the specific pattern of checking preconditions and handling errors the author is right. if file_exists(path) { # use path ... db = get_conn() if db.is_connected() { # use db ... } else { ret = new Error("db") } } else { ret = new Error("path") } return ret Every check we need to do increases our nesting and makes it look like it's got lots of different paths when really it has exactly one happy path and many error paths. if ! file_exists(path) [ return new Error("path") } # use path ... db = get_conn() if ! db.is_connected() { return new Error("db") } # use db ... This is exactly my rule -- only if/else if it's the happy path that's branching.

100011_100001 6y ago
That's a fair rebuttal. Depending on the situation though 'if not this - throw error' can be mitigated. It's one of the reasons why I try to avoid nulls coming into the code, otherwise you have to null check everywhere.