HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Wiring CollectionView and updating itself without the need for Refresh()

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
itselfwithouttheneedupdatingrefreshforwiringandcollectionview

Problem

I have been using your expertise developing my application using parts of WPF that I have never touched before.

In this stage my View does all I need it to do, however, I am not sure if I've done my side of the job right. My guess is that it still going to need numerous changes to be called as code following "Best Practice".

One of those major changes should be the binding of my CollectionView, which at the moment depends on its Refresh() to update its content.

The second would be using code behind for the ListViewItems double click.

Please feel free to comment any other issues other than the couple I mentioned.

ViewModel

```
///
/// Represents a container of EditPartViewModel objects
/// that havs support for staying synchronized with the
/// PartRespository. This class also provides information
/// related to multiple selected parts.
///
public class AllPartsViewModel : WorkspaceViewModel
{
#region Fields

readonly RelayCommand _clearSearch;
readonly PartRepository _partRepository;
readonly VendorRepository _vendorRepository;
bool _errorParts;
List allpvms;
RelayCommand _exportToCsv;
INotifyCollectionChanged notifyCollectionChanged;
string _searchFilterString;

#endregion // Fields

#region Constructor

public AllPartsViewModel(PartRepository partRepository, VendorRepository vendorRepostiory)
{
if (partRepository == null)
throw new ArgumentNullException("partRepository");
if (_clearSearch == null)
_clearSearch = new RelayCommand(
param => this.OnRequestCleanSearch(),
param => this.CanClear
);

base.DisplayName = Strings.AllPartsViewModel_DisplayName;

_partRepository = partRepository;
_vendorRepository = vendorRepostiory;
_searchFilterString = string.Empty;

// Populates the AllParts collection with PartViewModels.
this.CreateAllParts();
}

void Crea

Solution

One thing that jumped out at me is this:

private bool PartsFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return PartsSearchFilter(item) && PartsWithErrorFilter(item);
}

private bool PartsSearchFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return editPartViewModel.PartName.ToUpper().Contains(_searchFilterString.ToUpper());
}

private bool PartsWithErrorFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return _errorParts ? editPartViewModel.HasErrors : true;
}


You are sacrificing C#'s strongly typed model by allowing everything to be passed in. Afterwards you cast it to the expected type but you use as which means the value will be null if it happened to be the wrong type. This will cause NullReferenceExceptions on the next lines which call methods on this object.

The solution is easy: make each method accept an object of type EditPartViewModel and embrace the strongly typed nature of C#.

Notice you don't use editPartViewModel in your first method (although this is caught by doing that cast in every method). Still, nasty.

Compare case-sensitive strings using string.Equals(a, b, StringComparison.InvariantCulture). Imagine if that method is called 500.000 times with strings over 5000 characters long: each .ToUpper() call would create a new string object.

Always put brackets ({}), even if it's for oneliners. Save yourself the inevitable headaches of having to find an error only to find out it's because it wasn't in the scope of an if or for block.

Use [CallerMemberName] instead of using weakly-typed strings inside OnPropertyChanged. Example here: http://grenangen.se/node/75

Since AllPartsCollectionView is accessible from public, it isn't needed to specifically define

public int CollectionCount { get { return AllPartsCollectionView.Count; } }


If you need this property in your xaml, you can always used the nested notation.

I don't know what _errorParts means. Keep boolean naming convention in mind and make it descriptive.

Inherit from INotifyxxx instead of using composition. They define a trait of your type and should be used as such.

Code Snippets

private bool PartsFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return PartsSearchFilter(item) && PartsWithErrorFilter(item);
}

private bool PartsSearchFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return editPartViewModel.PartName.ToUpper().Contains(_searchFilterString.ToUpper());
}

private bool PartsWithErrorFilter(object item)
{
    var editPartViewModel = item as EditPartViewModel;
    return _errorParts ? editPartViewModel.HasErrors : true;
}
public int CollectionCount { get { return AllPartsCollectionView.Count; } }

Context

StackExchange Code Review Q#74010, answer score: 7

Revisions (0)

No revisions yet.