smarthomej / addons

SmartHome/J addons for openHAB
Eclipse Public License 2.0
59 stars 24 forks source link

[tuya] Improve decode color format error handling #541

Closed jsetton closed 9 months ago

jsetton commented 9 months ago

Fixes #540

J-N-K commented 9 months ago

I would suggest a different solution, that also covers illegal formats for other value: Surround l. 166 to l. 193 with

try {

...

} catch (IllegalArgumentException ignored) {
}
updateState(channelId, UndefType.UNDEF);

And keep the log we have now. WDYT?

jsetton commented 9 months ago

What about handling Integer.parseInt exception? I also personally rather have a try statement in a utility function than clogging the handler function or at the very least only cover the statements that would generate the catch exception.

As far as setting the channel to UNDEF, I don't mind doing it at that the bottom for any channel that wasn't updated but I have some reservation about the existing log line. In the use case I covered, I don't think it should log a warning since I always get this issue when my device is restarted and there isn't much that can be done to fix this. However it should certainly log a warning if it's an unknown data point.

J-N-K commented 9 months ago

I agree, but it sure is garbage. We can't know if it is a single "initial value" or we received an invalid value later, so I guess a warning is fine.

We usually try to return definite values and not null, it makes life much easier if it becomes a little bit more complex than here. That's why the method throws an exception instead of returning null. If it was Kotlin and we could use a safe-call or elvis operator I would agree that returning null is preferred.

I agree that we should use (IllegalArgumentException | NumberFormatException ignored) to also catch that one.

jsetton commented 9 months ago

I made the changes per your comments. I forgot that NumberFormatException extends from IllegalArgumentException. So it is not needed.