WPF ListView Pagination using MVVM Pattern

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:





















User Control with buttons for pagination:





















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 peopleList = new ObservableCollection();
#endregion

#region Pagination Methods
public void ShowNextPage()
{
CurrentPageIndex++;
ViewList.View.Refresh();
}

public void ShowPreviousPage()
{
CurrentPageIndex--;
ViewL

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 peopleList = new ObservableCollection();

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 peopleList = new ObservableCollection(); 
public ReadOnlyObservableCollection PeopleList 
{ 
    get { return new ReadOnlyObservableCollection(peopleList); } 
}

StackExchange Code Review Q#64186, answer score: 3