Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Architecture #1160

Open
Meisolsson opened this issue Feb 23, 2018 · 7 comments
Open

Architecture #1160

Meisolsson opened this issue Feb 23, 2018 · 7 comments

Comments

@Meisolsson
Copy link
Contributor

As it stands the app mixes a lot of UI and network code. I makes it hard to understand the flow and to see where to UI changes are made. Other problems include huge files, IssueFragment is a good example.

I'm open to suggestions to how we can fix this.

@Meisolsson
Copy link
Contributor Author

Right now my thought is to use the Android Architecture Components to solve some of the problems.

@veyndan
Copy link
Collaborator

veyndan commented Mar 1, 2018

IMO, the app currently is overly reliant on inheritance, making code editing extremely hard. For example, some of the classesInstead we should rely on composition as it is a more scalable solution. I feel that once we adopt the mantra of "composition over inheritance", the separation of network and UI concerns should be easier to manage.

I've looked into Android Architecture Components briefly, but have generally adopted an MVI approach in my own apps. However, it could be beneficial to have a standard library for the architecture to make it easier for people to contribute to the app. Ultimately, an adoption of any architecture would be beneficial.

@Meisolsson
Copy link
Contributor Author

Well inheritance is a pain in some cases but for the most part I'm not seeing it as a huge problem right now.

For architecture I've been having a look at:

It's mostly for inspiration but I'm leaning towards at least using the Domain, Data, Remote/Cache parts of it. The rest is kind of down to what ever we want do to in terms of presentation layer.

@Meisolsson
Copy link
Contributor Author

Alright I've been looking mostly at the data end of this and still unsure about how I'd like to have it done. So I think we should probably start of with the UI parts. I think MVI is my prefered way to go. Rendering a single state object feels like a nice way to handle everything. So now it's probably just down to the how.

@veyndan You have done some MVI earlier, do you have some nice way you generally approach it?

@veyndan
Copy link
Collaborator

veyndan commented Mar 20, 2018

MVI is definitely still an emerging structure in the Android community, but some things that I've found are useful is to make everything possible a stream. For example, the Android lifecycle, listening to any events (e.g. view events). A few libraries that should help with this is Navi and RxBinding. The use of Navi also means that we can remove the dependency on AutoDispose as Navi nicely handles unsubscription in a non "magical" way purely based on the way that Navi is used, compared to RxLifecycle and Autodispose. This is in relation to the discussion at #1149.

Anko could also be potentially helpful but the other two libraries would definitely aid MVI.

Regarding the overall design structure, this talk by Jake Wharton is a goldmine.

I've implemented this in one of my other apps. Here is an example of how you can use events and models. Also it shows that side effects only occur in the subscribe of the Observable. All the methods referenced are pure functions and the stream doesn't modify the state except in the subscribe (e.g. there is no doOnNext()). This is discussed in the Jake Wharton talk.

As converting an app of this size over to MVI will take a long time, I feel the best thing to do is make small changes before we start converting over to MVI. IMO the following should be done before such a change is made: fully convert the app over to Kotlin, remove unnecessary libraries, do some "light" refactoring, start using RxBinding and Navi (and Anko), be completely happy with the UI of the app (personally I found making architecture changes to an app while still making UI changes can be challenging).

I probably left some stuff out, but this is the core of it.

@Meisolsson
Copy link
Contributor Author

Regarding being happy with the UI that's not really gonna happen before moving I think. Also I'd prefer not pulling in to much extra stuff to be honest I'd prefer removing more libraries and only using the ones which are really needed.

I'll take a look at the talk and your app over the week, will also be going away this weekend (+ mon-wed) so might be a bit unresponsive. Other than that I'll probably try to prototype something just to see how I'd like the handle the stuff.

@veyndan
Copy link
Collaborator

veyndan commented Mar 20, 2018

Regarding being happy with the UI that's not really gonna happen before moving I think.

It's mostly with the overall structure of the app. Stuff like how list items look, colors, etc. don't really matter atm.

Also I'd prefer not pulling in to much extra stuff to be honest I'd prefer removing more libraries and only using the ones which are really needed.

It's only really a net addition of one library as RxBinding and Navi can be added and can remove AutoDispose. It'll be hard to model an event stream with view clicks without having something like RxBinding so I feel that it is a necessity. Plus both of the libraries are very lightweight and very easy to understand.

I'll take a look at the talk and your app over the week, will also be going away this weekend (+ mon-wed) so might be a bit unresponsive. Other than that I'll probably try to prototype something just to see how I'd like the handle the stuff.

Thanks for the heads up. If you want to discuss or talk about any ideas of MVI, I'm all ears. I'll probably continue to work on refactoring the code base, so hopefully MVI migration in the future will be less of a pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants