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); } 
  }