exer_log icon indicating copy to clipboard operation
exer_log copied to clipboard

#10 MVVM & Feature First Layer Architecture using Riverpod

Open momshaddinury opened this issue 2 years ago • 5 comments

New Architecture is implemented under lib/src/

The approach I've taken here after discussion in #10 is MVVM & Feature first layer using Riverpod state management.

The reasoning behind using Riverpod: Easy to use, Less boilerplate, Easy to learn. One of the other reason is, BLOC introduces lots of layer and given our app is using Firebase, it's rather simple to implement. With easier learning curve, think this is easier to to grasp.

Some helpful resources:

  1. Riverpod Doc
  2. Riverpod Flutter Package
  3. Riverpod Tutorial - Reso Coder

Now about our Feature First, MVVM Architecture:

Under lib/src/feature directory we will have

  • view (presentation layer, holds UI, NO BUSINESS LOGIC)
  • controller (aka View Controller, holds BUSINESS LOGIC and can talk with data layer)
  • repository (data layer, handles data)
  • widgets (these are widgets local to feature and is not used by any other screens, outside of that feature)

Under lib/src/utils directory:

  • we will have things such as text constants, assets, validators, text style, etc

Under lib/src/widgets/ directory:

  • we will have widgets that have global scope. These can be used by other features as well. For example buttons, text fields widgets, etc.

Under lib/src/core/ directory:

  • we want to have things do not change frequently. For example app wide base state won't change. The can be extended if needed, in the feature that requires it but the actual implementation of it should not change. Same goes for base view which I haven't implemented yet. Something I want to discuss in another issue.

This is a basic implementation. Tried to keep it as simple as possible. There are still room for improvements. LMK, if you folks have any suggestions.

momshaddinury avatar Aug 12 '22 18:08 momshaddinury

@KalleHallden Please review this. I can't mention participators in this issue (#10) for some reason.

momshaddinury avatar Aug 12 '22 18:08 momshaddinury

This looks like a massive improvement, good job.

jorre127 avatar Aug 12 '22 20:08 jorre127

@KalleHallden I think this is good to go. We went through some reviews and everything looks clean. Should get this merged if you don't have anything else to add.

momshaddinury avatar Aug 13 '22 13:08 momshaddinury

@jorre127 would request you to review this since I have addressed issue #9 and #13. 😟

momshaddinury avatar Aug 13 '22 16:08 momshaddinury

Looks great

jorre127 avatar Aug 13 '22 16:08 jorre127

Wow guys this looks like a fair amount of work! I really appreciate that!

KalleHallden avatar Aug 16 '22 07:08 KalleHallden

Just a suggestion for anyone coming back to this commit as maybe a point of reference.

This commit was huge. And impossible to review because of its size. I understand that it was probably blindly merged, and that's fine, it's Kalle's project, but in the spirit of open source, this was a giant change that should have been split up into multiple PRs.

Thanks for all the work either way, @dinurymomshad, but I would seriously recommend not trying to merge so many changes in one PR. It's easier for the community to review the changes.

HacDan avatar Aug 16 '22 13:08 HacDan

I get what you mean, but the PR wasn't insanely big before. The reason its 146 now is mainly because of moving the project to the root folder. But that's easily checked

jorre127 avatar Aug 16 '22 15:08 jorre127

@jorre127 I was more referring to the number of commits instead of smashing them into one. Not the amount of changes. In the grand scheme of things, this was relatively small, but 43 commits on a single PR is a bit much, especially with no discussion made between commits. Just adds to git log clutter.

HacDan avatar Aug 16 '22 20:08 HacDan

@HacDan I am really curious. When a project like this doesn't have any structure, and you need to introduce one, how do you handle that in smaller issues or PR? This only tackles one screen. The landing screen (with login and sign-up form). And smaller commits were made with the intention that, everyone can follow how the changes happened.

momshaddinury avatar Aug 17 '22 04:08 momshaddinury