trjhtr

I would like some feedback on whether this follows MVVM principles well.

The MainPage has a Login button that opens the login modal page. Upon successful login, the LoginPageViewModel will raise an event which is handled in the parent viewmodel.

I use Action objects to do the show/hide modals. The Actions are defined in a page's codebehind and invoked in its viewmodel.

MainPage.xaml

<Button Command="Binding ShowLoginModalCommand" Text="Login"/>

MainPage.xaml.cs

public MainPage()

 var vm = new MainPageViewModel();
 vm.ShowLoginModal += (lvm) => Navigation.PushModalAsync(new LoginPage(lvm));
 vm.HideLoginModal += () => Navigation.PopModalAsync();
 BindingContext = vm;
 InitializeComponent();

MainViewModel.cs

public Action<LoginViewModel> ShowLoginModal;
public Action HideLoginModal;

public ICommand ShowLoginModalCommand => new Command(() =>

 var lvm = new LoginViewModel();

 lvm.LoginSucceeded += (se, ev) =>
 
 this.Customer = ev.Customer;
 HideLoginModal();
 ;

 ShowLoginModal(lvm);


public Customer Customer

 get => _customer;
 set
 
 _customer = value;
 OnPropertyChanged(nameof(Customer));
 

LoginPage.xaml

<Button Command="Binding LoginCommand" Text="Login"/>

LoginViewModel.cs

public ICommand LoginCommand => new Command(async () => 

 var customer = await VerifyLogin(); // Assume valid login.
 LoginSucceeded(this, new LoginSuccessEventArgs(customer));


public class LoginSuccessEventArgs : EventArgs

 public Customer Customer get; set; 
 public LoginSuccessEventArgs(Customer customer) => Customer = customer;

public event EventHandler<LoginSuccessEventArgs> LoginSucceeded;

Generally speaking I find a good implementation and like the simplicity of the approach. I would point out some problems I found though.

Inconsistent MVVM style

You are using two different ways to instantiate views and viewmodels and linking them together in both views, an inconsistence that could get bigger the and messier the more complex the program becomes.

In the MainPage you're creating the viewmodel in the view's constructor, and assigning it to the BindingContext right away. But in the LoginPage you create the viewmodel in the handler of the main page and pass it to the constructor of the view (not show here, but I guess it also assigns the BindingContext with it).

Specially for bigger applications, it's important to clearly define one style, either view first or viewmodel first, and live with it (I don't even know if it's called "style", but it sounds like that to me). A more concerning problem is that when VMs begin to take dependencies on services and other stuff, you most likely will want to use an IoC container for injecting them, and then it becomes more important to clearly define who'll create viewmodels and how. Personally, I like viewmodel-first approach, in which viewmodels create other viewmodels and are passed to the views.

MainPage viewmodel

• Why are ShowLoginModal and HideLoginModal plain public fields? In situations like this, I would expect them to be normal events, raised from the viewmodel and handled in the view. This way feels a bit non-standard.


• On the ShowLoginModalCommand method, the viewmodel creates the login viewmodel directly. Maybe its unnecesary now, but consider using an IoC container or other factory to create this, as generally when other viewmodels take extra dependencies it becomes harder to create them all inline.


• The Customer setter doesn't needs to be public, a private one will do. You only want to set the current user from the login and nowhere else, so make that crystal clear.


• The invocations of the ShowLoginModal and HideLoginModal delegates don't contain a null validation, and will result in a NullReferenceException if no one suscribed to them. A nice shortcut to call them safely is to use the ShowLoginModal?(lvm); syntax.

LoginPage viewmodel

• Beware of the async reentrancy in the LoginCommand method. The VerifyLogin call inside it could potentially be a slow one, but since it's an await call, the UI will remain responsive. That means the user could do other things in the meanwhile, including changing the user/password or even click the login button again! That could have many unanticipated effects. I would counter it by disabling the UI while the verification takes place, using something like this in the VM:
  
  

  public bool EnableForm get; set; 
  
  public ICommand LoginCommand => new Command(async () => 
  
   //Disable UI before doing anything
   EnableForm = false;
   OnPropertyChanged(nameof(EnableForm));
   var customer = await VerifyLogin(); // Assume valid login.
   LoginSucceeded(this, new LoginSuccessEventArgs(customer));
  
  

  
  And in the view bind the controls to that property:
  
  

  <Button Command="Binding LoginCommand" Text="Login" IsEnabled="Binding EnableForm"/>
  

  
  


• In the LoginSuccessEventArgs the Customer property doesn't needs to be read-write. Once again, since you're just informing a value, there is no benefit in changing it after creation. Also, the constructor should check for null on the customer passed.


• Possibly it's not shown here or just a work-in-progress thing, but there is no handling of the login failure case, that should just reenable the controls (following my above suggestion), show some error message to the user and not raise the LoginSuccess event.


• Like in the main page, the raising of the event is not null checked and will blow up if nobody happens to be suscribed.


• The call to the VerifyLogin method, although maybe simplified here, could imply a lot of work under the cover. That's the kind of thing that's better delegated to a service to do all the heavy work, and just have the presentation layer handle the visual thing. I would inject it in the constructor and call it passing the user/password entered, and it would return success or failure. Something along the lines of this:
  
  

  private readonly ILoginService _loginService;
  
  public LoginViewModel(ILoginService loginService)
  
   if(loginService == null) throw new ArgumentNullException(nameof(loginService));
  
   _loginService = loginService;
  
  
  public ICommand LoginCommand => new Command(async () => 
  
   var customer = await _loginService.VerifyLogin(Username, Password);
   if(customer == null)
   //show some error message here
   else
   LoginSucceeded(this, new LoginSuccessEventArgs(customer));