search

TheCacophonyProject / bird-monitor

Android application to regularly record birdsong for analysis
GNU General Public License v3.0
11 stars 9 forks source link

Large refactor to improve the message broadcasting system. #54

Closed jackodsteel closed 5 years ago

jackodsteel commented 5 years ago

There are two main things targeted here, reducing the code duplication in sending messages, and adding Enums to reduce copy paste errors.

Firstly, I've refactored a lot of the broadcastMessage sending code to use more concise methods, reducing a lot of code duplication by creating the MessageHelper class and its relevant broadcastMessage methods.

Second, reworked the way messages are handled in general to make bugs/errors harder to happen. This is done by enforcing the Action messages to use an Action class, and their possible messages to be defined in an Enum. Because of this, classes must use the provided action/message types and you can't just typo something and miss it. This also allows switch statements to be used which is easier to follow than the if statements of before.

Most of the smart stuff here is in MessageHelper, which provides ways to create/register/unregister the message handlers, as well as the functionality to broadcast. This class basically adds a wrapper to enforce the Action class/Enums to be used.

I've also refactored the View/Fragment components out to their own subpackage to break the classes up slightly.

jackodsteel commented 5 years ago

@timhot Let me know if this is all clear how it's working now, especially taking a look at MessageHelper.

If anything's unclear or seems off let me know and we can take a look further/discuss.

timhot commented 5 years ago

Great work - looking far more professional. With so much change, I'm concerned some minor errors may have crept it. What is your thoughts on testing before release?

jackodsteel commented 5 years ago

Great work - looking far more professional. With so much change, I'm concerned some minor errors may have crept it. What is your thoughts on testing before release?

@timhot Sorry I meant to reply to this when you sent it then just forgot! As you were discussing in the #57 PR, it would be good to do some kind of pre release in general every time before release, and also trying to make the tests more reliable and extensive will go a long way to helping be more confident that the code works. So as a more long term thing that's the ideal approach.

For this specific case like you say it's a big change and a lot of the things it changed could result in minor bugs like messages not being received which would be hard to notice. One thing is that hopefully it wouldn't cause the biggest issues due to them really only doing stuff like user notifications. Would still be good to do a standard test release, and also just make sure to go over the code (I have reviewed it again).

timhot commented 5 years ago

Excellent - thanks