-
-
Notifications
You must be signed in to change notification settings - Fork 727
remove redundant "else" test in temperature example #451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fix arduino#450. There is also a spelling problem with "precede".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You introduced a typo here (should be spelled "Throttle"). I'm actually not convinced changing this comment provides any benefit, though I do very much agree with the change to a consistent comment style.
I would recommend not removing the redundant part of the expression. The code without it may be counter-intuitive for beginners. Besides, if you add more cases (temperature ranges) to the example, you probably would have to put it back.
I instead recommend just reorganizing the code as follows:
if (temperature < 60)
{
// Safe! Continue usual tasks...
}
else if (temperature >= 60 && temperature < 70)
{
// Warning! Throttle down, run the fans, and notify the user
}
else if (temperature >= 70)
{
// Danger! Shut down the system
}
Daniel-1276
commented
Oct 5, 2018
I am a beginner and raised the related issue. My point in reading the example was to understand what the difference between "if" and "else if" is. And I still think, the current example does not show the benefit of not having to use redundant checks, if the "above" statements were already checked. In the example of @robsoncouto you could also just use "if" statements IMHO.
What about using an example something like this:
if (temperature < 60)
{
status=good; //means temperature is below 60
}
else if (temperature <90)
{
status=GettingHot; //means temperature is between 60 and 89
}
else if(temperature <100)
{
status=DangerZone; //means temperature is between 90 and 99
}
else
{
status=Alarm; //means temperature is above 99
}
Hey @per1234 , could you please fix this one as you think it is adequate?
I think that the else example should demonstrate well the concept of mutual exclusivity and not focus on saving a few instructions. This is just my opinion, though.
My concern with the current code and your suggested code even more so is not about efficiency, but ease of code maintenance. Let's imagine the programmer decides they want to reduce the danger temperature threshold to 65. With your suggested code, they need to remember to change the value in two different places. If they forget to change one of the two values (a very easy thing to do when you have a lot of code in the clause):
if (temperature < 60) { // Safe! Continue usual tasks... } else if (temperature >= 60 && temperature < 70) { // Warning! Throttle down, run the fans, and notify the user } else if (temperature >= 65) { // Danger! Shut down the system }
then the intended change in behavior was not accomplished.
Of course if you used variables instead it wouldn't be an issue, but adding variables to the example code introduces extra complexity, which is contrary to your intent of making it easier for a beginner to understand.
My suggestion is to use best practices in the code, but add comments to make it clear how the code works. Something like this:
if (temperature >= 70) { // Danger! Shut down the system } else if (temperature >= 60) // 60 <= temperature < 70 { // Warning! User attention required } else // temperature < 60 { // Safe! Continue usual tasks... }
Updated and merged.
Fix #450. There is also a spelling problem with "precede".