michelesalvador / FamilyGem

Android app for genealogical trees
https://www.familygem.app
GNU General Public License v3.0
128 stars 34 forks source link

Want PR to use recommended app architecture? #67

Open Sternbach-Software opened 2 years ago

Sternbach-Software commented 2 years ago

There are some patterns the app uses that are contrary to what is recommended for modern Android apps in the official Android app architecture guidelines. Namely:

As well as general code smells:

@michelesalvador Would you be interested in pull requests to update these? Of course, I would be ensuring that functionality remains the same.

hannesa2 commented 2 years ago

I agree to this

hannesa2 commented 2 years ago

And don't forget to include no black-box libraries. One possible way could be git submodules ?

Sternbach-Software commented 2 years ago

@hannesa2 what do you mean black-box? That doesn't have sources and javadocs?

hannesa2 commented 2 years ago

With a copied jar, you never know exactly from where it comes from, and what it contains. I changed this on my side

Sternbach-Software commented 2 years ago

Doesn't your IDE decompile them? Android Studio does.

hannesa2 commented 2 years ago

Doesn't your IDE decompile them? Android Studio does.

That's not the question. When I compile it, there's a step less obfuscation and a little more transparency

Sternbach-Software commented 2 years ago

What black-box libraries are currently being used?

michelesalvador commented 1 year ago

@Sternbach-Software Thank you for your suggestions!

Using JSON database instead of Room

The JSON database it's not my choice: the Gedcom 5 Java library offers only JSON to store data.

I agree with most of your architecture recommendations (repository, fragments, LiveData, Kotlin coroutines, named constants). Now I'm using them indeed, e.g. for the new Merge tree function.

Restarting now I'd choose Glide instead of Picasso, but I don't agree that is Picasso the cause of #66, that occurs with large trees without media also.

I understand that reflection is a bad practice, and I'm now avoiding it whenever possible. But with one exception: there are 13 classes containing tens of lines based on one reflection.

No thanks, I'm not interested on a PR to update all these. At the moment I prefer to make it myself little by little.

Black-box libraries currently used: