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

View model usage

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

Problem

I'm about to start work on a large project using MVVM design pattern. I've worked on smaller projects in the past using MVVM, but I want to make sure my approach to MVVM is good/manageable as possible before I attempt something much larger.

The following view model is from a project that I have worked on in the past. It is a simple, functional program that I use to push releases to people within my network. When they run their programs, they fetch the latest versions from the SQL server via stored procedure. This tool is what I use to manage those files.

The program consists of 2 ListViews, one holds local program files, and the other holds server program files. Each file is represented by a DbAssembly object (even the local files). There are 2 separate context menus for the ListViews, one allows me to Upload or Delete a local file, the other allows me to push the file to beta, live or archived status, or download the file for my own use. There is also a button that runs all local files.

```
public class AssembliesViewModel : ViewModelBase
{
// Fields
private ObservableCollection _localAssemblies;
private ObservableCollection _serverAssemblies;

//Properties
public ObservableCollection LocalAssemblies
{
get { return _localAssemblies; }
private set
{
_localAssemblies = value;
RaisePropertyChanged("LocalAssemblies");
}
}
public ObservableCollection ServerAssemblies
{
get { return _serverAssemblies; }
private set
{
_serverAssemblies = value;
RaisePropertyChanged("ServerAssemblies");
}
}

//Constructors
public AssembliesViewModel()
{
InitializeCommands();
RefreshLocalAssemblies();
RefreshServerAssemblies();
}

//Methods
private void RefreshLocalAssemblies()
{
LocalAssemblies = new ObservableCollection(
VersionFinder.GetLocalVersions());
}

Solution

-
at the setters of the ObservableCollection LocalAssemblies and ObservableCollection ServerAssemblies you should check if the collection is equal to the set value. If yes, the event shouln't be raised.

-
tenary expressions can be quite helpful, but they just don't improve the readability, especially if they are used for evaluating boolean expressions. This

private bool CanReleaseBetaExecute(DbAssembly param)
    {
        return param == null ? false
            : (param.Beta == null && param.Live == null && param.Archived == null) ? true : false;
    }
    private bool CanReleaseLiveExecute(DbAssembly param)
    {
        return param == null ? false
            : (param.Live == null && param.Archived == null) ? true : false;
    }
    private bool CanArchiveExecute(DbAssembly param)
    {
        return param == null ? false
            : (param.Archived == null) ? true : false;
    }


can be refactored to be more readable

private bool CanReleaseBetaExecute(DbAssembly param)
{
    return (param != null && param.Beta == null && param.Live == null && param.Archived == null);
}
private bool CanReleaseLiveExecute(DbAssembly param)
{
    return (param != null && param.Live == null && param.Archived == null);
}
private bool CanArchiveExecute(DbAssembly param)
{
    return (param != null && param.Archived == null);
}


-
you should change the order of your code. It should be at least

  • constructors



  • properties



  • methods



I don't like naming each input parameter param and if I see this, I assume that if you have two ore more input parameteres you will have param1, param2...

Otherwise your code looks good and tidy. You are using meaningful names for methods, properties and fields.

Code Snippets

private bool CanReleaseBetaExecute(DbAssembly param)
    {
        return param == null ? false
            : (param.Beta == null && param.Live == null && param.Archived == null) ? true : false;
    }
    private bool CanReleaseLiveExecute(DbAssembly param)
    {
        return param == null ? false
            : (param.Live == null && param.Archived == null) ? true : false;
    }
    private bool CanArchiveExecute(DbAssembly param)
    {
        return param == null ? false
            : (param.Archived == null) ? true : false;
    }
private bool CanReleaseBetaExecute(DbAssembly param)
{
    return (param != null && param.Beta == null && param.Live == null && param.Archived == null);
}
private bool CanReleaseLiveExecute(DbAssembly param)
{
    return (param != null && param.Live == null && param.Archived == null);
}
private bool CanArchiveExecute(DbAssembly param)
{
    return (param != null && param.Archived == null);
}

Context

StackExchange Code Review Q#73880, answer score: 2

Revisions (0)

No revisions yet.