login button activation issue fixed

[aarashgoodarzi]

Login button activation according to inputs considered

Also LoginViewModel methods improved a little bit :)

[EbrahimTahernejad]

Hi, thank you for submitting this pull request.

upon reviewing the code a bit I found some issues with it which I'll explain.

Please use Publisher.combineLatest to combine loading, username, password into isLoginButtonDisabled. No need to update it in the imperative logic part

[EbrahimTahernejad]

it would be something like

// inside init:

$loading
    .combineLatest($username, $password)
    .map { !$0 || $1.isEmpty || $2.isEmpty }
    .assign(to: &$isLoginButtonDisabled)

OR:

// No @Published needed
var isLoginButtonDisabled: Boolean {
    return !loading || username.isEmpty || password.isEmpty
}

both will work and both will update the UI, but the second one is shorter and better.

also please change isLoginButtonDisabled to something like canSubmit or isSubmittable to separate UI logic from the ViewModel.

Thank you.

[aarashgoodarzi]

Hi, thank you for submitting this pull request.

upon reviewing the code a bit I found some issues with it which I'll explain.

Please use Publisher.combineLatest to combine loading, username, password into isLoginButtonDisabled. No need to update it in the imperative logic part

it would be something like

// inside init:

$loading
    .combineLatest($username, $password)
    .map { !$0 || $1.isEmpty || $2.isEmpty }
    .assign(to: &$isLoginButtonDisabled)

OR:

// No @Published needed
var isLoginButtonDisabled {
    return !loading || username.isEmpty || password.isEmpty
}

both will work and both will update the UI, but the second one is shorter and better.

also please change isLoginButtonDisabled to something like canSubmit or isSubmittable to separate UI logic from the ViewModel.

Thank you.

Thanks for your comments Ebrahim. @EbrahimTahernejad

For the first comment a readOnly isLoginButtonDisabled variable will not reflect on View as it changes, as its not a publisher. ( Though i tested it and it didn't work. it would be nice you give me more explanation).

For the second comment, ViewModel is responsible for presentation logic and state of an button is a part of it. Also as ViewModel itself says it belongs to a View. The point is ViewModel does not belong to a specific view and it's dummy about who calls it. so this kind of naming make more sense.

[EbrahimTahernejad]

it will re-render because loading, username, and password are all published variables. Changing any of them triggers objectWillChange and as a result a re-check from SwiftUI's engine. You can also try it yourself.

For the second one, you are both right and wrong. Because I'm not the god of software engineers and I've seen people implement ViewModels in different ways. But in my own style of coding, I try to enforce these two rules (and some more but these two are related to your PR):

1. Separate as much of the concerns as possible: Not just in MVVM, but in many other architectures we have this type of separation. In MVVM, Model is not aware of the ViewModel, and ViewModel is not aware of the View. That's why the ViewModel should not be aware of a button in the View. Let's look at it this way, in a ViewModel, if we have choices between two names for a variable: editButtonShouldBeHidden or editable. They both represent what you want to do with your data. but let's imagine tomorrow your designer changes his mind, and wants you to disable the edit button instead of hiding it. What would you do now? change the variable name? wouldn't it have been better if your view decides what to do with the model being editable or not, instead of your ViewModel deciding what the View should do with your data? canSubmit has meaning both in the View and the ViewModel but isLoginButtonDisabled only holds its meaning when the view is considered as well (which contradicts what we want from MVVM)

2. Avoid spreading out your logic for a state We're working on an opensource project here, so there will be people after us that maintain this project (hopefully :D). If the next person comes here and sees:

   var canSubmit: Boolean {
   return !loading || username.isEmpty || password.isEmpty
   }

   They will immediately understand what this part of the code does and they don't need to look at other places to understand in which situations the form is submittable and in which situations it isn't. Now let's say the ViewModel grows so much that these functions are not close to each other and we find a bug regarding this variable's behavior. Which style of coding is easier to debug?

[EbrahimTahernejad]

Although this video is about RxJS and Angular (Javascript), Rx and Combine share a lot of definitions and behavior. Watching it will be worth it:

https://www.youtube.com/watch?v=skOTEbGwncE

[aarashgoodarzi]

Thanks for your explanations

HELP :)

For the first on i tried it again and it didn't work( even on another test project). unless i'm missing something. help me please :))

NAMING

For the second one, i'm not god either:))) But let's break it down to make it more clear

Extending the example Assume in the example, tomorrow designers decide to add a new scenario "hiding" to the button. now, we have two states of 1-disabling and 2-hiding. By the approach we have "canSubmit" and "canSubmit2" ( or some better names which avoid to express the states)

Now if the day after tomorrow something changes we have two, to rename. So if we consider renaming as a costly process, still it in not solved.

Self-Documentation is Important Beside of that, as the naming are not self-documented developers trying to modify or extend the behaviors will have to sneak out to View to see what they are changing. so i'm not sure about your last sentence.

Reusability and Flaws As we all know one of the reasons behind having ViewModel is reusability( when presentation logic is the same but views are different, it could be even an extended version of ViewModel for the second and they both have a lot in common), assume we used a specific ViewModel for two different Views ( in the future Seebaro might has several login pages for gold users and regular users). As we are tightly coupling the presentation logic to the first login page, ViewModel loses its reusability. Also it might lead to some unwanted flaws . because canSubmit may be used in first view for disabling button and be used for adding a new button in the second login view. changing for the first one may break behaviors of the second one.

SoC While we are trying to separate our concerns, we have to focus on the concerns and responsibilities of each part of the code. The separation should not lead us to cripple responsibilities of a part.

Presentation logic should fully accomplish its responsibilities regardless of other concerns. And states are the core added value of presentation logic. If states should not be placed in presentation logic so where are they? Though "canSubmit" is state itself. So we should put states there and let them express themselves.

Clean Arch As uncle bob described in "Clean architecture" layers should be defined inwards and "should not mention" outer layers. As long as we are not "mentioning" outer layers the system health is fine. Expressing with explained names is not mentioning and its on the lines. Its accomplishing responsibilities.

[EbrahimTahernejad]

1. This image will explain (4.0MB):

"canSubmit" and "canSubmit2"

2. Why would we have two canSubmits? One suffices. Do you add multiple instances of a variable in your model because multiple ViewModels do different stuff with them? Same goes for a ViewModel and a View. canSubmit describes the state of the ViewModel's other props. While isLoginButtonDisabled is deciding what the view should do with the state as well. Your View logic is leaking into the ViewModel's. Although you're not "mentioning" the view itself, you're enforcing its logic. In other words, you are mentioning the View's button. But as I said there is really no clear answer. This is how I decided to name stuff in the project (if there are instances that don't conform to this logic, they will be removed upon refactoring) and my next project might be different. If you like, you could contact me via telegram @EbrahimThr and we can discuss what goes through my mind further.

[EbrahimTahernejad]

Lovely, just one small typo isNoSubmittable -> isNotSubmittable

[aarashgoodarzi]

1 - it helped ✅

2 - In the context of the example "canSubmit" and "canSubmit2" are placeholder names for the suggested naming style . one for hiding and one for disabling ( it could be XXX and YYY. name it as you desire) you can't use one "canSubmit" for both states in the example.

It is not leaking view into viewModel. it is presentation logic not mentioning any other layer. presentation logic is what it does. you might say why we have to know there s a button out there? the answer is in the above and if anything changes the logic also changes . now the difference of view and the presenter is that presenter is concept and view is coming to objectivity to fruition

anyway as it is not a critical issue i considered your suggestion

[EbrahimTahernejad]

First of all, thank you for contributing to this project.

---

I think you're confusing a ViewModel and a Presenter's flow a bit. The Presenter in the MVP pattern has knowledge of the view's existence, while a ViewModel is more unaware of the inner workings of the View.

In MVVM the first question is better while in MVP the second one is favored.

• "The view needs to know if it can submit the form"
• "The view needs to know if it can enable the login view"

This is a rule I set after experiencing a couple of projects in which more than a handful of people worked on it (with MVVM architecture).

Let's assume one person is working on the view and another is working on the ViewModel, if you are a good co-worker you'd ask the view model developer the first question "I need to know if I can submit the form at a point in time or not". Of course in a situation as simple as that it won't make much difference, but I've encountered some miscommunications with some more complicated UI logic.

If we started the project today knowing what I know I would not have pushed for the MVVM architecture, maybe MV or MVP would've been better.

[aarashgoodarzi]

There's a point we need to consider here. Differences between VM and Presenter (unidirectional data flow as key difference) does not change the essence of VM being a presenter only with improvements with different approach.

As mentioned before it is not knowing view it is knowing presentation logic. still VM does not know what is View .it knows a presentation. by hiding problems will start to appear. in your example think there's two button and also a list. the developer of View wants to show loading on list and on submitting on each button. how to handle that? https://en.wikipedia.org/wiki/File:MVVMPattern.png