3
\$\begingroup\$

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<DbAssembly> _localAssemblies;
    private ObservableCollection<DbAssembly> _serverAssemblies;

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

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

    //Methods
    private void RefreshLocalAssemblies()
    {
        LocalAssemblies = new ObservableCollection<DbAssembly>(
            VersionFinder.GetLocalVersions());
    }
    private void RefreshServerAssemblies()
    {
        ServerAssemblies = new ObservableCollection<DbAssembly>(
            VersionFinder.GetServerVersions(null));
    }
    private void RunAssemblies(Window param)
    {
        AssemblyLoader.Run();
        if (param != null) param.Close();
    }
    private void Delete(DbAssembly param)
    {
        if (param == null) return;
        param.Delete();
        RefreshLocalAssemblies();
    }
    private void Upload(DbAssembly param)
    {
        if (param == null) return;
        param.Upload();
        RefreshServerAssemblies();
    }
    private void Download(DbAssembly param)
    {
        if (param == null) return;
        param.Download();
        RefreshLocalAssemblies();
    }
    private void ReleaseBeta(DbAssembly param)
    {
        if (param == null) return;
        param.ReleaseBeta();
        RefreshServerAssemblies();
    }
    private void ReleaseLive(DbAssembly param)
    {
        if (param == null) return;
        param.ReleaseLive();
        RefreshServerAssemblies();
    }
    private void Archive(DbAssembly param)
    {
        if (param == null) return;
        param.Archive();
        RefreshServerAssemblies();
    }

    //Commands
    private void InitializeCommands()
    {
        RefreshLocalAssembliesCommand = new RelayCommand(x => RefreshLocalAssemblies());
        RefreshServerAssembliesCommand = new RelayCommand(x => RefreshServerAssemblies());
        RunAssembliesCommand = new RelayCommand(x => RunAssemblies(x as Window));
        DeleteCommand = new RelayCommand(x => Delete(x as DbAssembly));
        UploadCommand = new RelayCommand(x => Upload(x as DbAssembly));
        DownloadCommand = new RelayCommand(x => Download(x as DbAssembly));
        ReleaseBetaCommand = new RelayCommand(x => ReleaseBeta(x as DbAssembly), x => CanReleaseBetaExecute(x as DbAssembly));
        ReleaseLiveCommand = new RelayCommand(x => ReleaseLive(x as DbAssembly), x => CanReleaseLiveExecute(x as DbAssembly));
        ArchiveCommand = new RelayCommand(x => Archive(x as DbAssembly), x => CanArchiveExecute(x as DbAssembly));
    }
    public ICommand RefreshLocalAssembliesCommand { get; private set; }
    public ICommand RefreshServerAssembliesCommand { get; private set; }
    public ICommand RunAssembliesCommand { get; private set; }
    public ICommand DeleteCommand { get; private set; }
    public ICommand UploadCommand { get; private set; }
    public ICommand DownloadCommand { get; private set; }
    public ICommand ReleaseBetaCommand { get; private set; }
    public ICommand ReleaseLiveCommand { get; private set; }
    public ICommand ArchiveCommand { get; private set; }
    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;
    }
}

One thing that I try to do is ensure code looks neat when folded, as I find that it's usually much easier to find what I'm looking for.

Please let me know of any ideas/improvements that could be made to improve my code technique.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ But what does it do? It helps immensely to briefly describe what the program does. It's MVVM. Okay, but that's a pattern. It's not what the program is. Make sense? \$\endgroup\$ – RubberDuck Dec 17 '14 at 1:52
  • \$\begingroup\$ @RubberDuck Apologies, I wasn't sure if the purpose of the program pertained to good application of the pattern. I've updated my question with hopefully enough information. \$\endgroup\$ – learningcs Dec 17 '14 at 2:24
  • \$\begingroup\$ The application of the program is always important. Thank you very much much! Welcome to Code Review! \$\endgroup\$ – RubberDuck Dec 17 '14 at 2:32
2
\$\begingroup\$
  • at the setters of the ObservableCollection<DbAssembly> LocalAssemblies and ObservableCollection<DbAssembly> 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

    1. constructors
    2. properties
    3. 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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your post, I found it quite helpful. I went ahead and made the changes you suggested, including changing the parameter names be more meaningful. I'm wondering if I should follow the linked code order rules for commands, as I find keeping code relating to commands in one place is very efficient... I did try grouping them in their correct categories (ICommands grouped into properties, Initialize and CanExecute methods grouped into methods), but I think it is harder to find things this way... Thank you for your help \$\endgroup\$ – learningcs Dec 17 '14 at 21:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.