nucleus-ffm / foss_warn

An unofficial open source application to get emergency alerts from https://warnung.bund.de/meldungen.
GNU General Public License v3.0
98 stars 6 forks source link

Cleanup #86

Closed nucleus-ffm closed 1 year ago

nucleus-ffm commented 1 year ago

code cleanups and refactoring

MatsG23 commented 1 year ago

I propose to still do the following tasks before merging:

nucleus-ffm commented 1 year ago

I have been working on these tasks and most of the variables are now private and only accessible with getter and setter methods. With the exception of the WarnMessage class. Should we use getter and setter there as well? I would say that it is not necessary because we do not work with these attributes inside the class. But what do you think? Otherwise, maybe we can still improve encapsulation in some methods.

MatsG23 commented 1 year ago

When it is not needed, there is no point in adding getters and setters for WarnMessages I think. But how can it be that we do not use the class attributes there? I am confused.

nucleus-ffm commented 1 year ago

But how can it be that we do not use the class attributes there? I am confused.

That was not very specific of me. Of course we use the attributes. But I thought maybe we still want direct access to those attributes from the outside. But it would be the cleaner way to always use getter methods. I'll change that as the last thing before we merge this pull request.

nucleus-ffm commented 1 year ago

Sorry, I'm thinking differently again now. I read the Dart style guide and it says

AVOID wrapping fields in getters and setters just to be "safe". Source

I then also noticed that Dart actually uses a different syntax for getters and setters. You can use a getter like String get name => _name. You can then just access the value with classXY.name as if you were using direct access. But if you didn't declare a setter, you can't just do classXY.name = "ABC". I didn't know this language feature and thought too much in Java. So I change the getter/setter to this syntax and would not declare a getter for WarnMessage since all attributes are already final and cannot be changed. So I think it would be more like the correct Dart style to not create getters for it in this class.

nucleus-ffm commented 1 year ago

I still had to change a few things.

MatsG23 commented 1 year ago

I noticed nothing during review that requires big changes. By the way, I somewhat like the global variables in a class, although this may not be perfect. Now you know better where what variable comes from - when it is e.g. a user preference and when it is not.

Since the app is now available in more languages, we should write the next changelog in English, shouldn't we?

nucleus-ffm commented 1 year ago

Since the app is now available in more languages, we should write the next changelog in English, shouldn't we?

Yes, that would be nice. We have two options here. The first would be simply to create a second file and translate the changelog manually. Or just have English changelogs? The second option would be to always create a new translation string and translate it with weblate. I think the first option is one thing we can talk about - the second option would be quite annoying. So what would you say?

nucleus-ffm commented 1 year ago

I fixed the little things you noticed. You might want to check if I got everything.

MatsG23 commented 1 year ago

Since the app is now available in more languages, we should write the next changelog in English, shouldn't we?

Yes, that would be nice. We have two options here. The first would be simply to create a second file and translate the changelog manually. Or just have English changelogs? The second option would be to always create a new translation string and translate it with weblate. I think the first option is one thing we can talk about - the second option would be quite annoying. So what would you say?

The second option is definitely the most annoying. I think in todays world we can just make English changelogs only.