Separate business logic from UI

[utkarshshendge]

• Parent issue : Refactoring of codebase and separation on data/UI #715

• Issue Description:
  Making a universal class that stores all the data fetching functions which are currently written individually for each and every file for their own use. Doing so would help in separating the Data fetching functions to a separate place and UI in a separate place. Also, there is an advantage of this as well some functions need not be written again like theuserinfo.

• Issue Severity:
  Medium

• Environment Details:
  Project Level

• Observed Behaviour:
  Business logic is not separated from UI.

• Expected Behaviour:
  Separate business logic from UI

• Does this issue need immediate attention?
  Yes

• Please ask mentors/maintainers to assign if you are willing to work on this issue.

[sumitra19jha]

Can I work on this @palisadoes @Sagar2366 ?

[palisadoes]

Issue #700 has now been integrated into the pull request process. It will flag all Dart files that are longer than 300 lines to help with readability. Please make sure your contribution reduces file sizes to within this limit.

[CyberWake]

@palisadoes and @Sagar2366 can I also work in with @sumitra19jha on this issue.

[palisadoes]

@CyberWake I added you as you were the author of the original issue #715. Please work with @sumitra19jha to determine a way to figure out a layout that meets these goals.

[CyberWake]

Sure sir

[sumitra19jha]

Great. @CyberWake We will come with the best effective solution

[sumitra19jha]

@rutvik11062000 I checked your code. @CyberWake and I have a conversation on this and we were currently implementing the provider for this solution as assigned by @Sagar2366

This particular file was being worked by @CyberWake

[rutvik11062000]

@sumitra19jha and @CyberWake #721 issue suggests the separation of service from the UI. What #758 suggests is making a function layer between the UI and the service class file. Please see the graphic mentioned in 758 to have a better idea.

[sumitra19jha]

@rutvik11062000 I have gone through the diagram. I am confused about its intended purpose. Why would we need this?

When this issue will be resolved business logic and UI will be separated. The reason you stated for the need of issue #758 will be taken care of in this issue itself.

1. The 300 lines will be taken care of by separating all logic from UI.
2. Duplicate codes will be avoided by creating a separate widget file in the widgets folder for success toast, error toast, etc. This will further reduce line count.
3. The second reason which you stated regarding the widget test will be served here too. After separating Ui from logic, we will be having efficient widget testing. If you go through the current widget test files, you can see we are concerned only with individual widget count.

Example: 
    testWidgets('Verifying presence of icons in HomePage', (tester) async {
      await tester.pumpWidget(createHomePageScreen());
      //detecting icons by find.byIcon(Icons.home)
      expect(find.byIcon(Icons.home), findsOneWidget);
      expect(find.byIcon(Icons.chat), findsOneWidget);
      expect(find.byIcon(Icons.calendar_today), findsOneWidget);
      expect(find.byIcon(Icons.group), findsOneWidget);
      expect(find.byIcon(Icons.folder), findsOneWidget);
    });

    testWidgets('Verifying if the first page is NEWSFEED', (tester) async {
      await tester.pumpWidget(createHomePageScreen());
      expect(find.byKey(const Key('NEWSFEED_APP_BAR')), findsOneWidget);
      //any other page should not be there
      expect(find.byKey(const Key('GROUPS_APP_BAR')), findsNothing);
    });

This will be taken care of by having a separate widget file itself.
If it's ok, Can you please elaborate more on why we need that separate service layer here?

[rutvik11062000]

@sumitra19jha @CyberWake

To be clear the mentioned perks (i.e <300 lines) are the aftermath (after effects) of using MVVM architecture, we are not implementing it just because we need shorter page lengths.

First, we need to understand the actual meaning of separating business logic.
Principles:

1. UI file must not be in the contact / must not responsible for initiating the data (fetching) related services and managing states.
2. The only job of the UI file is to show the data based on the state it is in. (not to call any service functions or maintain states).
3. The part of calling the data related functions, manipulating the data and managing the state must be done separately.
4. One can see the ViewModel as the controller which talks with the service file, take data from it, update it (if required) and notify the UI file to show the updated data.
5. The part of taking/sending data from/to the actual API must be done separately (this is what 721 is about) for reuse of the functions.

And to be on the same page all the tests that are currently written are only the UI testing we haven't added any tests related to the services and testing the functions.

This is where 758 comes into the picture:
Testing can be broken down into 3 major sections
a. When we want to test the widget or the UI, we will only test the xyz_view.dart file.
b. When we want to test the data fetching related services we will test the service files.
c. When to test the functions (like data changes when pressing a button), we will take inly xyz_viewModel.dart into consideration.

If we are separating only service layer(721),

1. managing states and calling the functions again goes in the hand of the UI file, as business logic is still attached to the UI file we can't say that we had separated the business logic entirely.
2. And also we have to test widgets by passing the parameters and changing the UI file
   

I hope everything is clear by now😊.

[sumitra19jha]

@rutvik11062000 I appreciate you for taking your time to explain to me this. I have few more doubts:
The principles which you mention for business logic the points 1,2 and 3 will be taken care of here in this issue, now let's review points 4 & 5. On the newsfeed page, state management is implemented, and on the joined organizations page MVVM is followed and implemented.

NewsFeed Page onpressed function for icon button: [------Provider-----]

if (post['likeCount'] != 0) {
              if (isPostLiked == false) {
                //If user has not liked the post addLike().
                Provider.of<NewsFeedProvider>(context, listen: false)
                    .addLike(post['_id'].toString());
              } else {
                Provider.of<NewsFeedProvider>(context, listen: false)
                    .removeLike(post['_id'].toString());
              }
            } else {
              //if the likeCount is 0 addLike().
              Provider.of<NewsFeedProvider>(context, listen: false)
                  .addLike(post['_id'].toString());
}

Joined Organisation Page onpressed of elevated button: [-----MVVM----]

onPressed: () {
                  // model.itemIndex = organization['_id'].toString();
                  model.setItemIndex(organization['_id'].toString());
                  print("itemIndex : " + model.itemIndex);
                  if (organization['isPublic'].toString() == 'false') {
                    model.setIsPublic('false');
                  } else {
                    model.setIsPublic('true');
                  }
                  model.confirmOrgDialog(
                      organization['name'].toString(), index, context);
                },

So as you can see to the best of my knowledge, point 5 will also be taken care of here in the issue. If point 5 is resolved, is there any need for having a separate distinct file for point 4?
If yes then what code it will contain?

I am asking this because I want to make sure that we follow one single method for every UI-related part and logic-related part.

Now coming to your point of test-codes. Yes you are right we currently don't have services and functions testing implemented properly.

But That will be done after resolving this issue. We can add test code for functions and services separately with help of the provider. The code snippet you share is based on the previous implementation, but it will be taken care of after the provider addition. The test code will be very specific to UI itself.

[rutvik11062000]

@sumitra19jha Actually we are talking about the same things but with a different approach.

As this issue quotes "Making a universal class that stores all the data fetching functions" it gave everyone an ambiguity.
There is no mention of using providers to inject these services into the UI file.

This issue gave an idea that you both are doing the work of separating services by putting them into an actual class.
Like class AuthenticationService {} which then needs to be injected using either provider or locator.

There is no right and wrong in using the provider or MVVM as a service class both have their own pros and cons.
As you mentioned about why to create extra file, so in basic terms what MVVM do is it kind of break down the XYZProvider file into two files, service and view-model file. Rest the working of both are same (code-length will be almost the same).

talking about the MVVM pros against the provider:

1. ViewModel have a state property which takes care of the state of the UI file.
   (suppose if we want to show a loading indicator we need to implement it separately in every provider.)
2. We can keep track of instances easily by registering services as a factory or as lazy singleton.
3. In provider we need to make sure that it is always above one widget where we want to use it, hence we used multiproviders from the root of the app.
4. Dependency injection
   Suppose A and B are 2 widget which needs the service:
   a. In case of provider we need to make the least common node as a provider dependency injection.
   b. In case of MVVM we can inject it directly to the children widget which needs them.
   

[rutvik11062000]

@sumitra19jha I think we should take opinions from mentors and contributors about which direction we should follow😊.

[sumitra19jha]

@rutvik11062000 The purpose here was to separate business logic from UI and to do that we will obviously use provider and it is a big task. So this issue was assigned to two contributors and if you want to work, we would love to work together with you on this.

Issue #766 #765 #764 #763 #761 all will be covered here and there is no need to duplicate the issue just because the solution is different.

Now coming to the use of MVVM or provider(State Management), I will leave it up to the mentor's recommendation of what should we use. @palisadoes @Sagar2366 @DeltaHarbinger

---

All I am saying is either we should use MVVM or Provider but not both. If some files will be using the provider and some by using MVVM then this will destroy the code Uniformity.

---

Now, coming to my opinion of the solution, I would recommend the provider as a lot of contributors have already worked on creating files(codes) related to it. The entire controller folder is built for that purpose and contains codes using the provider. If we are using MVVM then we have to start all this work again from scratch and which means removing all this work. Since Provider is very efficient to handle what MVVM will be doing then why waste our already existing work.

[utkarshshendge]

@rutvik11062000 as seen in the parent issue #715 there is a link to discussion thread. I guess @sumitra19jha and @CyberWake came to the conclusion of continuing with the provider.
The use of providers is not mentioned in any of the comments on this issue and maybe you didn't see the comment by Peter sir on issue #478 and that might have confused you.
Let's get the mentors opinion on state management.