openhab-android

openhab-android copied to clipboard

Hi!

I am working on a tool for detection of violations on design principles in my master thesis (read more about it here: https://github.com/Mkohm/detekt-hint), and is looking for an open source project where I can integrate my tool for some feedback and testing. Would you welcome such an integration? It would mean a lot, and I hope it can continue to improve the maintainability of this project.

It only requires creating a configuration file and a new GitHub action. Please let me know if it is of any interest, and I will create a PR with the required changes.

Hey @Mkohm, is it possible that you run detekt-hint on the current master branch, so I can see what issues are found? Based on that I can have a look if we want to integrate it in the app.

Sure, will do! But be aware that the list of issues will be long, and contain many false-positives. Using it without the github integration will defeat much of its purpose, only commenting on added/modified files.

Will add a report with the issues later, stay tuned 👍

detekt-hint-report-openhab.html.zip Inside this .zip you will find a .html file that you can open in your browser. There you will find all the reported issues.

Also, take into consideration that by integrating the tool you will be able to configure the rules, turn them on/off and/or exclude files included for inspection. Please let me know if you need anything from me. Any feedback on the rules/tool in general is highly appreciated !

IMO there are too many false positives, e.g. InterfaceSegregationPrinciple in:

/Users/mariuskohmann/Code/School/master/code/openhab-android/mobile/src/foss/java/org/openhab/habdroid/ui/MapViewHelper.kt:116:9 onMarkerDragStart is not implemented an may not be necessary. This is a possible violation of the interface segregation principle.

https://github.com/openhab/openhab-android/blob/master/mobile/src/foss/java/org/openhab/habdroid/ui/MapViewHelper.kt#L116

Yes, I don't need onMarkerDragStart() and onMarkerDrag(), but onMarkerDragEnd(). And because all three are in the same interface, I need to implement all three.

/Users/mariuskohmann/Code/School/master/code/openhab-android/mobile/src/main/java/org/openhab/habdroid/ui/WidgetAdapter.kt:372:44 Switching on enum values is a common sign of violation the Open-Closed Principle. Consider introducing an abstraction (interface) for Type, with new implementations of the interface for every value

Well, Widget.Type is an enum, so this would require to change them to a class and adding an interface with a function like getHumanReadableType(), but only for some Widget.Types, right?

I haven't looked at the report in detail, but I spotted some issues where I agree that they should be fixed.

Thanks for leaving some feedback, i appreciate it!

I agree about the InterfaceSegregationPrinciple issue you point out. In this case you are forced by the map library to break the principle. I have seen this multiple places on Android, e.g Textwatcher as well. An solution to this problem would having an option to ignore certain interfaces.

For the Open-closed principle you could create an interface (or multiple) that each widget type needs to implement. Then, the logic for each Widget.Type could be contained in its own implementations. And yes, then a function like getHumanReadableType would be suitable. Since not all Widget.Type's need the getHumanReadableType it is a sign that you need multiple interfaces, and that different Widget.Type needs to implement multiple interfaces based on their behaviour 😃

Well, Widget.Type is an enum, so this would require to change them to a class and adding an interface with a function like getHumanReadableType(), but only for some Widget.Types, right?

If we did that, we might no longer violate open-closed principle, but blatantly violate separation of model and presentation. Widget.Type is an abstract data representation, it's up to the view or presentation layers what to do with it.

I'd rather keep the code as is here...

@maniac103, interesting, i guess there is a trade-off here. But looking at the Widget.Type enum it is clear that new entries might be added over time. Is this a sign of a bigger problem that the Widget class instead should be broken into separate implementations for each of the Widget types?

@Mkohm I'd need to think about that, but that approach sounds more useful to me. At the moment, the Widget class is more or less a 1:1 representation of the data coming from the server API; it just abstracts away the OH1 vs. OH2 differences. Having a class for each widget type, where each class only contains the data items relevant for that specific widget type, certainly is an interesting idea.

Detekt could be added as scanning tool directly in GitHub, e.g. on my fork: https://github.com/mueller-ma/openhab.android/security/code-scanning?query=is%3Aopen+branch%3Amain+tool%3Adetekt