Make the battery status always valid on windows #2288
Conversation
| @@ -44,6 +40,10 @@ void BatteryWindows::read() { | |||
| if (seconds_left >= 0) { | |||
| m_iMinutesLeft = seconds_left / 60; | |||
There was a problem hiding this comment.
I don't remember that I ever suggested such a change ;)
m_iMinutesLeft is not always set in this branch. Initializing both m_chargingState and m_iMinutesLeft with UNKNOWN helps to guarantee a consistent state upon exit.
There was a problem hiding this comment.
Setting m_dPercentage to 0.0 should not even be required if m_chargingState = Battery::UNKNOWN? Initializing it with NaN instead of 0.0 would emphasize this.
Considering the complexity of the various branches I would recommend to keep the initialization to invalid/unknown values at the beginning of the function.
There was a problem hiding this comment.
There was a problem hiding this comment.
m_iMinutesLeftis not always set in this branch. Initializing bothm_chargingStateandm_iMinutesLeftwith UNKNOWN helps to guarantee a consistent state upon exit.
I have fixed the not always set m_iMinutesLeft and m_chargingState.
Not initializing them with UNKNOWN is the point of this pull request.
(Setting m_chargingState to UNKNOWN for a short period of time, if it is actually known may be suboptimal)
There was a problem hiding this comment.
No interception possible, the unknown value will never become visible. Control flows could be simplified if only known and defined values need to be written. And it is safer if you could exit the function at any time while still leaving the object in a valid state.
There was a problem hiding this comment.
No interception possible
Yes, there is no interception possible. The unknown value could only become visible in a crash dump or debug session. Not sure if it really matters..
rrrapha
force-pushed
the
batterywindows
branch
from
472a201
to
715dddf
Compare
|
I'm not convinced that the code gets easier to follow by using deeply nested if blocks. Safely initializing the members with invalid/unknown default values and returning as soon as possible when you are not able to read the required values is simpler and more robust. |
rrrapha
force-pushed
the
batterywindows
branch
from
715dddf
to
ac049c2
Compare
If we agree that the class members do not have to be valid at any time, this pull request can be closed. (@daschuer please reopen if you think this is wrong) |
This change was once suggested by @uklotzde.