WPF ListView Pagination using MVVM Pattern

Question

A few days ago, I was looking for simple pagination example and I decided to write my own version. It is one of my first apps using MVVM Pattern, so I am not sure I did right. I would appreciate if you could tell if I did something wrong, or used bad programming practises or whatever.

GitHub

MainWindow:

<Window x:Class="MVVMListViewPagination.MainWindow"
 xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
 xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
 xmlns:userControls="clr-namespace:MVVMListViewPagination.Views"
 Title="MVVM - ListView Pagination" Height="350" Width="525">
<Grid>
 <Grid.RowDefinitions>
 <RowDefinition Height="*" />
 <RowDefinition Height="45" />
 </Grid.RowDefinitions>
 <ListView ItemsSource="{Binding ViewList.View}" Margin="5">
 <ListView.View>
 <GridView>
 <GridViewColumn Header="Id" Width="40" DisplayMemberBinding="{Binding Id}" />
 <GridViewColumn Header="Name" Width="150" DisplayMemberBinding="{Binding Name}" />
 <GridViewColumn Header="Surname" Width="150" DisplayMemberBinding="{Binding Surname}" />
 </GridView>
 </ListView.View>
 </ListView>
 <userControls:PaginationElements Grid.Row="1" Margin="5" />
</Grid>

User Control with buttons for pagination:

<UserControl x:Class="MVVMListViewPagination.Views.PaginationElements"
 xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
 xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
 xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
 xmlns:d="http://schemas.microsoft.com/expression/blend/2008" 
 mc:Ignorable="d" >
<UserControl.Resources>
 <Style TargetType="Button">
 <Setter Property="Margin" Value="5" />
 </Style>
 <Style TargetType="TextBlock">
 <Setter Property="Margin" Value="5" />
 <Setter Property="VerticalAlignment" Value="Center" />
 </Style>
</UserControl.Resources>
<StackPanel Orientation="Horizontal" HorizontalAlignment="Right">
 <Button Content="First" Command="{Binding FirstCommand}"/>
 <Button Content="Previous" Command="{Binding PreviousCommand}"/>
 <TextBlock Width="auto" Text="{Binding CurrentPage}"/>
 <TextBlock Text="of"/>
 <TextBlock Width="auto" Text="{Binding TotalPage}"/>
 <Button Content="Next" Command="{Binding NextCommand}"/>
 <Button Content="Last" Command="{Binding LastCommand}"/>
</StackPanel>

ViewModel:

namespace MVVMListViewPagination.ViewModels
{
 class MainViewModel : BaseViewModel
 {
 #region Commands
 public ICommand PreviousCommand { get; private set; }
 public ICommand NextCommand { get; private set; }
 public ICommand FirstCommand { get; private set; }
 public ICommand LastCommand { get; private set; }
 #endregion
 #region Fields And Properties
 int itemPerPage = 15;
 int itemcount;
 private int _currentPageIndex;
 public int CurrentPageIndex
 {
 get { return _currentPageIndex; }
 set { _currentPageIndex = value; OnPropertyChanged("CurrentPage"); }
 }
 public int CurrentPage
 {
 get { return _currentPageIndex + 1; }
 }
 private int _totalPage;
 public int TotalPage
 {
 get { return _totalPage; }
 set { _totalPage = value; OnPropertyChanged("TotalPage"); }
 }
 public CollectionViewSource ViewList { get; set; }
 public ObservableCollection<Person> peopleList = new ObservableCollection<Person>();
 #endregion
 #region Pagination Methods
 public void ShowNextPage()
 {
 CurrentPageIndex++;
 ViewList.View.Refresh();
 }
 public void ShowPreviousPage()
 {
 CurrentPageIndex--;
 ViewList.View.Refresh();
 }
 public void ShowFirstPage()
 {
 CurrentPageIndex = 0;
 ViewList.View.Refresh();
 }
 public void ShowLastPage()
 {
 CurrentPageIndex = TotalPage - 1;
 ViewList.View.Refresh();
 }
 void view_Filter(object sender, FilterEventArgs e)
 {
 int index = ((Person)e.Item).Id - 1;
 if (index >= itemPerPage * CurrentPageIndex && index < itemPerPage * (CurrentPageIndex + 1))
 {
 e.Accepted = true;
 }
 else
 {
 e.Accepted = false;
 }
 }
 private void CalculateTotalPages()
 {
 TotalPage = (itemcount / itemPerPage);
 if (itemcount % itemPerPage != 0)
 {
 TotalPage += 1;
 }
 }
 #endregion
 public MainViewModel()
 {
 populateList();
 ViewList = new CollectionViewSource();
 ViewList.Source = peopleList;
 ViewList.Filter += new FilterEventHandler(view_Filter);
 CurrentPageIndex = 0;
 itemcount = peopleList.Count;
 CalculateTotalPages();
 NextCommand = new NextPageCommand(this);
 PreviousCommand = new PreviousPageCommand(this);
 FirstCommand = new FirstPageCommand(this);
 LastCommand = new LastPageCommand(this);
 }
 private void populateList()
 {
 for (int i = 0; i < 100; i++)
 {
 peopleList.Add(new Person(i, "Jack", "Black"));
 }
 }
 }
}

One of the Command classes:

namespace MVVMListViewPagination.Commands
{
 class FirstPageCommand : ICommand
 {
 private MainViewModel viewModel;
 public FirstPageCommand(MainViewModel viewModel)
 {
 this.viewModel = viewModel;
 }
 public bool CanExecute(object parameter)
 {
 if (viewModel.CurrentPageIndex == 0)
 return false;
 else
 return true;
 }
 public event EventHandler CanExecuteChanged
 {
 add { CommandManager.RequerySuggested += value; }
 remove { CommandManager.RequerySuggested -= value; }
 }
 public void Execute(object parameter)
 {
 viewModel.ShowFirstPage();
 }
 }
}

I'm not a professional programmer, so all my knowledge comes from tutorials. I was trying to mantain a MVVM Pattern and I would like to know if I succeeded, as well as any constructive criticism about my skills as a programmer overall.

Answer 1

First

as well as any constructive criticism about my skills as a programmer overall

We don't criticise skills. We only review code.

Good

• Sometimes using braces {} for if..else statement
• Naming of methods, parameter and fields based on naming convention

Bad

• Sometimes using no braces {} for if..else statement
• Inconsistent naming of variables ( somtimes with underscore , sometimes without )
• Using no access modifier where private would be better for readability and consistence
• Some namings should be changed

Why is using {} braces, also for a single line of an ..if..else statement, important ? See Apple Bug

Refactoring

• FirstPageCommand

  We should use braces for the if..else statement in the CanExecute() method.

  public bool CanExecute(object parameter)
  {
   if (viewModel.CurrentPageIndex == 0)
   {
   return false;
   }
   else
   {
   return true;
   }
  }
  

  but we can do this better, as we don't need this if..else statement at all, at least as long there isn't passed / used / needed the parameter

  public bool CanExecute(object parameter)
  {
   return viewModel.CurrentPageIndex != 0
  }
  

• MainViewModel

  int itemPerPage = 15;
  int itemcount; 
  

  should be

   private int itemPerPage = 15;
   private int itemcount;
  

  public int CurrentPageIndex
  {
   get { return _currentPageIndex; }
   set { _currentPageIndex = value; OnPropertyChanged("CurrentPage"); }
  } 
  

  This will confuse Mr.Maintainer as this is not as readable as it could. So let us change it in the way that each get a separate line and also the setter should be private.

  public int CurrentPageIndex
  {
   get { return _currentPageIndex; }
   private set 
   { 
   _currentPageIndex = value; 
   OnPropertyChanged("CurrentPage"); 
   }
  }
  

  Another naming issue

  private int _totalPage;
  public int TotalPage
  {
   get { return _totalPage; }
   set { _totalPage = value; OnPropertyChanged("TotalPage"); }
  } 
  

  Here we should use the plural form for the backing field and the property

  private int _totalPages;
  public int TotalPages
  {
   get { return _totalPages; }
   set 
   { 
   _totalPages = value; 
   OnPropertyChanged("TotalPages"); 
   }
  } 
  

  but wait.. does the setter need to be public ? Not really so let us instead use a private set

  public int TotalPages
  {
   get { return _totalPages; }
   private set 
   { 
   _totalPages = value; 
   OnPropertyChanged("TotalPages"); 
   }
  }
  

  private void CalculateTotalPages()
  {
   TotalPage = (itemcount / itemPerPage);
   if (itemcount % itemPerPage != 0)
   {
   TotalPage += 1;
   }
  }
  

  Let us see what this code is doing. First is it assigning the value of (itemcount / itemPerPage) to the former TotalPage property. Then it is checking if the modulo is !=0 and if this is true it is adding 1. The adding 1 will involve the setter and the getter of the property. So the OnPropertyChanged event will fire twice.

  So let us refactor the method

  private void CalculateTotalPages()
  {
   if (itemcount % itemPerPage == 0)
   {
   TotalPages = (itemcount / itemPerPage);
   }
   else
   {
   TotalPages = (itemcount / itemPerPage) + 1;
   }
  }
  

  public ObservableCollection<Person> peopleList = new ObservableCollection<Person>();
  

  What about the scope of peopleList ? Does it need to be public ? Let us change the scope of peopleList to private and add a public ReadOnlyObservableCollection property

  private ObservableCollection<Person> peopleList = new ObservableCollection<Person>(); 
  public ReadOnlyObservableCollection<Person> PeopleList 
  { 
   get { return new ReadOnlyObservableCollection<Person>(peopleList); } 
  }
  

• svick: "Using no access modifier (which defaults to internal) where private would be better" That's not true, default access for members is private (it's internal only for top-level types).
• Dess: Thanks for review. I didn't know about the rule about braces. I fixed my code on GitHub, don't know if I should do it in here as well? And you didn't say anything about MVVM pattern, did I used it right? And about naming convention - I only use underscore to private properties, should I use it also with regular private fields?
• Magus: @Dess: The thing about braces is absolutely not a rule. It can be dangerous if someone used to python tries to add something at the same indentation level to put it within the conditional (which will not work). I generally avoid unneeded braces, but it can be a safety measure. As for underscore prefixes, they are also completely arbitrary. Your IDE is almost certainly smart enough to color privates differently from publics. If it helps you though, use it. 'Always use braces' is probably agreed upon by 70% of people, while underscores are more like 40%.
• JanDotNet: Another optimization: check if the the value actually changed before rising property changed in property setters.