search

saltyrtc / saltyrtc-client-java

SaltyRTC Java implementation.
Apache License 2.0
3 stars 4 forks source link

Use enum instead of int constants for close codes #120

Closed ovalseven8 closed 5 years ago

ovalseven8 commented 5 years ago

This change provides better type safety and is considered as best practice (Effective Java, 3rd Edition, Item 34)

Should fix #119

@dbrgn Created no additional tests as enum should provide enough safety so that #119 should not happen again. But please review.

lgrahl commented 5 years ago

I'll assign you @dbrgn since there was some performance stuff related to enums. Kind of forgot what it was, so it must have been weird... or I mixed up things.

dbrgn commented 5 years ago

Yeah, enums are quite memory-inefficient on Android (and there used to be performance issues in very old Android versions). That's why the Android added things like @IntDef as an alternative which is type-checked at build time. (We can't use it in this project though since it's not Android-specific.)

From https://android.jlelse.eu/android-performance-avoid-using-enum-on-android-326be0794dc3:

Enums add at least two times more bytes to the total APK size than plain constants and can use 5 to 10 times more RAM memory than equivalent constants.

Thus, while I agree with the advantages of enums, I'd prefer the old-fashioned int constants.

ovalseven8 commented 5 years ago

Wow, what a clusterfuck Android is. Didn't know that.

After some reading it's apparently only relevant for old/pre-ART Android phones. Also, the official Android documentation removed the warning to avoid enums.

So I guess it makes sense to avoid it here since Threema should also support older phones. It feels wrong though.

dbrgn commented 5 years ago

So I guess it makes sense to avoid it here since Threema should also support older phones. It feels wrong though.

I fully agree with all you said :wink:

Replaced by #122.