MP3 tag Project in MVVM style

Question

After reading the books "Clean Code" from Robert C. Martin and the "Head First Design Patterns" book from Elisabeth Robson I decided to make a mp3 tag application.

I tried to do it in the MVVM style and appy as much clean code principles as possible in the code. One thing I didn't figure out is how to apply the single responsibility principle for the ViewModels where I wanted to separate the representation logic of the properties and the command logic.

I'm specifically looking for feedback about code style and organization but any constructive feedback would be appreciated.

Here is the GitHub link to my project: https://github.com/multi-kulti66/MP3_Tag

Here are 3 example classes from the project:

Model example

public class Mp3Song : ObjectBase, IMp3Song
{
    #region Fields

    private readonly IFileModifier fileModifier;
    private readonly IMp3File mp3File;

    #endregion



    #region Constructors

    public Mp3Song(IMp3File paramMp3File, IFileModifier paramFileModifier)
    {
        this.mp3File = paramMp3File;
        this.fileModifier = paramFileModifier;
    }

    #endregion



    #region IMp3Song Members

    public string FilePath
    {
        get { return this.mp3File.FilePath; }
    }

    public string WishedFilePath
    {
        get { return this.mp3File.WishedFilePath; }
    }

    public string Title
    {
        get { return this.mp3File.Title; }
        set { this.SetProperty(newValue => this.mp3File.Title = newValue, value); }
    }

    public string Artist
    {
        get { return this.mp3File.Artist; }
        set { this.SetProperty(newValue => this.mp3File.Artist = newValue, value); }
    }

    public string Album
    {
        get { return this.mp3File.Album; }
        set { this.SetProperty(newValue => this.mp3File.Album = newValue, value); }
    }

    public bool FileExistsAlready
    {
        get
        {
            if ((this.WishedFilePath != this.FilePath) && this.fileModifier.FileExists(this.WishedFilePath))
            {
                return true;
            }

            return false;
        }
    }

    public void SaveAndRename()
    {
        this.Save();
        this.Rename();
        this.Reload();
        this.EndEdit();
    }

    public void Save()
    {
        this.mp3File.Save();
    }

    public void Reload()
    {
        this.mp3File.Reload();
    }

    public void Undo()
    {
        this.CancelEdit();
    }

    #endregion



    #region Methods

    private void Rename()
    {
        if (this.FileExistsAlready)
        {
            this.fileModifier.Delete(this.WishedFilePath);
        }

        this.fileModifier.Rename(this.FilePath, this.WishedFilePath);
    }

    #endregion
}

ViewModel example 1

public class Mp3SongViewModel : BindableValidator
{
    #region Fields

    private readonly IDialogService dialogService;
    private readonly Mp3Song mp3Song;

    private bool _isChecked;

    #endregion



    #region Constructors

    public Mp3SongViewModel(Mp3Song paramMp3Song, IDialogService paramDialogService)
    {
        this.dialogService = paramDialogService;
        this.mp3Song = paramMp3Song;

        this.InitCommands();

        this._isChecked = false;
    }

    #endregion



    #region Properties, Indexers

    public string FilePath
    {
        get { return this.mp3Song.FilePath; }
    }

    public bool InEditMode
    {
        get { return this.mp3Song.InEditMode; }
    }

    public bool IsChecked
    {
        get { return this._isChecked; }
        set { this.SetProperty(newValue => this._isChecked = newValue, value); }
    }

    [DisplayName]
    [Required]
    [ValidString]
    public string Title
    {
        get { return this.mp3Song.Title; }

        set
        {
            this.SetProperty(newValue => this.mp3Song.Title = newValue, value);
            this.RaisePropertyChanged(() => this.InEditMode);
        }
    }

    [DisplayName]
    [Required]
    [ValidString]
    public string Artist
    {
        get { return this.mp3Song.Artist; }

        set
        {
            this.SetProperty(newValue => this.mp3Song.Artist = newValue, value);
            this.RaisePropertyChanged(() => this.InEditMode);
        }
    }

    [DisplayName]
    [ValidString]
    public string Album
    {
        get { return this.mp3Song.Album; }

        set
        {
            this.SetProperty(newValue => this.mp3Song.Album = newValue, value);
            this.RaisePropertyChanged(() => this.InEditMode);
        }
    }

    public List<CommandViewModel> Commands { get; private set; }

    #endregion



    #region Methods

    private void InitCommands()
    {
        this.Commands = new List<CommandViewModel>();
        this.Commands.Add(new CommandViewModel(Resources.Mp3SongVM_DisplayName_Load, Resources.CommandName_Load, new RelayCommand(this.PlayMessage)));
        this.Commands.Add(new CommandViewModel(Resources.Mp3SongVM_DisplayName_Save, Resources.CommandName_Save, new RelayCommand(this.Save, this.CanSave)));
        this.Commands.Add(new CommandViewModel(Resources.Mp3SongVM_DisplayName_Undo, Resources.CommandName_Undo, new RelayCommand(this.Undo, this.CanUndo)));
        this.Commands.Add(new CommandViewModel(Resources.Mp3SongVM_DisplayName_Remove, Resources.CommandName_Remove, new RelayCommand(this.RemoveMessage)));
        this.Commands.Add(new CommandViewModel(Resources.Mp3SongVM_DisplayName_ClearAlbum, Resources.CommandName_ClearAlbum, new RelayCommand(this.ClearAlbum, this.CanClearAlbum)));
    }

    public RelayCommand GetCommand(string paramCommandName)
    {
        try
        {
            CommandViewModel command = this.Commands.Find(x => x.CommandName == paramCommandName);
            return command.RelayCommand;
        }
        catch
        {
            throw new ArgumentException(Resources.Exception_InvalidCommandName, paramCommandName);
        }
    }

    public void Rename(IMp3Tag paramMp3Tag)
    {
        if (!string.IsNullOrEmpty(paramMp3Tag.Title))
        {
            this.Title = paramMp3Tag.Title;
        }

        if (!string.IsNullOrEmpty(paramMp3Tag.Artist))
        {
            this.Artist = paramMp3Tag.Artist;
        }

        if (!string.IsNullOrEmpty(paramMp3Tag.Album))
        {
            this.Album = paramMp3Tag.Album;
        }
    }

    private void PlayMessage()
    {
        Messenger.Default.Send(new NotificationMessage<Mp3SongViewModel>(this, Resources.CommandName_Load));
    }

    private bool CanSave()
    {
        if (this.IsValid && this.InEditMode)
        {
            return true;
        }

        return false;
    }

    private async void Save()
    {
        try
        {
            if (this.mp3Song.FileExistsAlready)
            {
                bool replaceFile = await this.dialogService.ShowDialogYesNo("Warning!", "The file exists already. Do you want to replace it?");

                if (!replaceFile)
                {
                    return;
                }
            }

            this.mp3Song.SaveAndRename();
            this.UpdateProperties();
        }
        catch (IOException)
        {
            this.dialogService.ShowMessage("Warning!", string.Format("The following file is loaded in the media player and can't be saved:\n {0}", this.Artist + " - " + this.Title));
        }
    }

    private bool CanUndo()
    {
        if (this.InEditMode)
        {
            return true;
        }

        return false;
    }

    private void Undo()
    {
        this.mp3Song.Undo();
        this.UpdateProperties();
    }

    private void UpdateProperties()
    {
        PropertyInfo[] propertyInfos = this.GetAllPropertyInfos();

        foreach (PropertyInfo propertyInfo in propertyInfos)
        {
            this.RaisePropertyChanged(propertyInfo.Name);
        }
    }

    private PropertyInfo[] GetAllPropertyInfos()
    {
        return this.GetType()
                    .GetProperties(BindingFlags.Public | BindingFlags.Instance).ToArray();
    }

    private async void RemoveMessage()
    {
        if (this.mp3Song.InEditMode)
        {
            bool saveFile = await this.dialogService.ShowDialogYesNo("Warning!", "The file was not saved. Do you want to save it?");

            if (saveFile)
            {
                this.Save();
            }
        }

        Messenger.Default.Send(new NotificationMessage<Mp3SongViewModel>(this, Resources.CommandName_Remove));
    }

    private bool CanClearAlbum()
    {
        if (string.IsNullOrEmpty(this.Album))
        {
            return false;
        }

        return true;
    }

    private void ClearAlbum()
    {
        this.Album = string.Empty;
    }

    #endregion
}

ViewModel example 2

public class DataGridViewModel : BindableValidator
{
    #region Fields

    private readonly IDialogService dialogService;
    private readonly IModelFactory modelFactory;

    private List<Mp3SongViewModel> _selectedMp3SongViewModels;

    private RelayCommand<object> _dropCommand;
    private RelayCommand _checkAllMp3SongViewModelsCommand;
    private RelayCommand _uncheckAllMp3SongViewModelsCommand;
    private RelayCommand _checkOrUncheckSelectedElementsCommand;
    private RelayCommand _moveUpSelectedElementsCommand;
    private RelayCommand _moveDownSelectedElementsCommand;

    #endregion



    #region Constructors

    public DataGridViewModel(IDialogService paramDialogService, IModelFactory paramModelFactory)
    {
        this.dialogService = paramDialogService;
        this.modelFactory = paramModelFactory;

        this._selectedMp3SongViewModels = new List<Mp3SongViewModel>();
        this.Mp3SongViewModels = new ObservableCollection<Mp3SongViewModel>();
        this.Mp3SongViewModels.CollectionChanged += this.ContentCollectionChanged;

        Messenger.Default.Register<NotificationMessage<List<string>>>(this, this.AddWhenNew);
        Messenger.Default.Register<NotificationMessage<Mp3Tag>>(this, this.RenameCheckedElementsNotification);
        Messenger.Default.Register<NotificationMessage<Mp3SongViewModel>>(this, this.RemoveNotification);
        Messenger.Default.Register<NotificationMessage<string>>(this, this.HandleMp3SongCommandNotification);
    }

    #endregion



    #region Properties, Indexers

    public ObservableCollection<Mp3SongViewModel> Mp3SongViewModels { get; private set; }

    public List<Mp3SongViewModel> SelectedMp3SongViewModels
    {
        get { return this._selectedMp3SongViewModels; }
        set { this.SetProperty(newValue => this._selectedMp3SongViewModels = newValue, value); }
    }

    public RelayCommand<object> DropCommand
    {
        get
        {
            if (this._dropCommand == null)
            {
                this._dropCommand = new RelayCommand<object>(this.Drop);
            }

            return this._dropCommand;
        }
    }

    public RelayCommand CheckAllMp3SongViewModelsCommand
    {
        get
        {
            if (this._checkAllMp3SongViewModelsCommand == null)
            {
                this._checkAllMp3SongViewModelsCommand = new RelayCommand(this.CheckAllMp3Songs);
            }

            return this._checkAllMp3SongViewModelsCommand;
        }
    }

    public RelayCommand UncheckAllMp3SongViewModelsCommand
    {
        get
        {
            if (this._uncheckAllMp3SongViewModelsCommand == null)
            {
                this._uncheckAllMp3SongViewModelsCommand = new RelayCommand(this.UncheckAllMp3Songs);
            }

            return this._uncheckAllMp3SongViewModelsCommand;
        }
    }

    public RelayCommand CheckOrUncheckSelectedElementsCommand
    {
        get
        {
            if (this._checkOrUncheckSelectedElementsCommand == null)
            {
                this._checkOrUncheckSelectedElementsCommand = new RelayCommand(this.CheckOrUncheckSelectedElements);
            }

            return this._checkOrUncheckSelectedElementsCommand;
        }
    }

    public RelayCommand MoveUpSelectedElementsCommand
    {
        get
        {
            if (this._moveUpSelectedElementsCommand == null)
            {
                this._moveUpSelectedElementsCommand = new RelayCommand(this.MoveUpSelectedElements);
            }

            return this._moveUpSelectedElementsCommand;
        }
    }

    public RelayCommand MoveDownSelectedElementsCommand
    {
        get
        {
            if (this._moveDownSelectedElementsCommand == null)
            {
                this._moveDownSelectedElementsCommand = new RelayCommand(this.MoveDownSelectedElements);
            }

            return this._moveDownSelectedElementsCommand;
        }
    }

    #endregion



    #region Methods

    private void MoveUpSelectedElements()
    {
        for (int i = 1; i < this.Mp3SongViewModels.Count; i++)
        {
            int upperElement = i - 1;
            int lowerElement = i;

            if (!this.SelectedMp3SongViewModels.Contains(this.Mp3SongViewModels[upperElement]) && this.SelectedMp3SongViewModels.Contains(this.Mp3SongViewModels[lowerElement]))
            {
                Mp3SongViewModel tempMp3SongViewModel = this.Mp3SongViewModels[upperElement];
                this.Mp3SongViewModels.Remove(tempMp3SongViewModel);
                this.Mp3SongViewModels.Insert(lowerElement, tempMp3SongViewModel);
            }
        }
    }

    private void MoveDownSelectedElements()
    {
        for (int i = this.Mp3SongViewModels.Count - 2; i >= 0; i--)
        {
            int upperElement = i;
            int lowerElement = i + 1;

            if (!this.SelectedMp3SongViewModels.Contains(this.Mp3SongViewModels[lowerElement]) && this.SelectedMp3SongViewModels.Contains(this.Mp3SongViewModels[upperElement]))
            {
                Mp3SongViewModel tempMp3SongViewModel = this.Mp3SongViewModels[lowerElement];
                this.Mp3SongViewModels.Remove(tempMp3SongViewModel);
                this.Mp3SongViewModels.Insert(upperElement, tempMp3SongViewModel);
            }
        }
    }

    private void Drop(object obj)
    {
        var dataObject = obj as IDataObject;

        if (dataObject == null)
        {
            return;
        }

        string[] formats = (string[])dataObject.GetData(DataFormats.FileDrop);

        if (formats == null)
        {
            return;
        }

        foreach (string fileName in formats)
        {
            this.AddWhenNew(fileName);
        }
    }

    public void AddWhenNew(NotificationMessage<List<string>> paramNotificationMessage)
    {
        foreach (string filePath in paramNotificationMessage.Content)
        {
            this.AddWhenNew(filePath);
        }
    }

    public void AddWhenNew(string paramFilePath)
    {
        try
        {
            if (!this.IsNewMp3Song(paramFilePath))
            {
                return;
            }

            Mp3Song mp3Song = new Mp3Song(this.modelFactory.CreateMp3File(paramFilePath), this.modelFactory.CreateFileModifier());
            Mp3SongViewModel mp3SongViewModel = new Mp3SongViewModel(mp3Song, this.dialogService);

            this.Mp3SongViewModels.Add(mp3SongViewModel);
        }
        catch (FileException)
        {
            this.dialogService.ShowMessage(Resources.DataGridVM_Exception_Add, string.Format(Resources.DataGridVM_Inner_Exception_Add, paramFilePath));
        }
    }

    private bool IsNewMp3Song(string paramFilePath)
    {
        if (this.Mp3SongViewModels.Any(mp3SongViewModel => mp3SongViewModel.FilePath == paramFilePath))
        {
            return false;
        }

        return true;
    }

    private void CheckAllMp3Songs()
    {
        this.IterateAllMp3SongsAndDoAction(mp3SongViewModel => mp3SongViewModel.IsChecked = true);
    }

    private void UncheckAllMp3Songs()
    {
        this.IterateAllMp3SongsAndDoAction(mp3SongViewModel => mp3SongViewModel.IsChecked = false);
    }

    private void CheckOrUncheckSelectedElements()
    {
        if (this.SelectedMp3SongViewModels.All(selectedElement => selectedElement.IsChecked))
        {
            this.IterateSelectedElementsAndDoAction(selectedElement => selectedElement.IsChecked = false);
        }
        else
        {
            this.IterateSelectedElementsAndDoAction(selectedElement => selectedElement.IsChecked = true);
        }
    }

    public void Remove(Mp3SongViewModel paramMp3SongViewModel)
    {
        if (this.Mp3SongViewModels.Remove(paramMp3SongViewModel))
        {
            this.RaisePropertyChanged(() => this.Mp3SongViewModels);
            return;
        }

        this.dialogService.ShowMessage(Resources.DataGridVM_Exception_Remove, string.Format(Resources.DataGridVM_Inner_Exception_Remove, paramMp3SongViewModel.FilePath));
    }

    private void IterateAllMp3SongsAndDoAction(Action<Mp3SongViewModel> paramAction)
    {
        for (int i = this.Mp3SongViewModels.Count - 1; i >= 0; i--)
        {
            paramAction(this.Mp3SongViewModels[i]);
        }
    }

    private void IterateCheckedMp3SongsAndDoAction(Action<Mp3SongViewModel> paramAction)
    {
        for (int i = this.Mp3SongViewModels.Count - 1; i >= 0; i--)
        {
            if (this.Mp3SongViewModels[i].IsChecked)
            {
                paramAction(this.Mp3SongViewModels[i]);
            }
        }
    }

    private void IterateSelectedElementsAndDoAction(Action<Mp3SongViewModel> paramAction)
    {
        foreach (Mp3SongViewModel mp3SongViewModel in this.SelectedMp3SongViewModels)
        {
            paramAction(mp3SongViewModel);
        }
    }

    private void RenameCheckedElementsNotification(NotificationMessage<Mp3Tag> notificationMessage)
    {
        this.IterateCheckedMp3SongsAndDoAction(mp3SongViewModel => mp3SongViewModel.Rename(notificationMessage.Content));
    }

    private void RemoveNotification(NotificationMessage<Mp3SongViewModel> notificationMessage)
    {
        if (notificationMessage.Notification == Resources.CommandName_Remove)
        {
            this.Remove(notificationMessage.Content);
        }
    }

    private void HandleMp3SongCommandNotification(NotificationMessage<string> paramNotificationMessage)
    {
        if (paramNotificationMessage.Content == Resources.CommandBroadcast_All)
        {
            this.IterateAllMp3SongsAndDoAction(mp3SongVM => mp3SongVM.GetCommand(paramNotificationMessage.Notification).Execute(this));
        }
        else if (paramNotificationMessage.Content == Resources.CommandBroadcast_Checked)
        {
            this.IterateCheckedMp3SongsAndDoAction(mp3SongVM => mp3SongVM.GetCommand(paramNotificationMessage.Notification).Execute(this));
        }
    }

    public void ContentCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        if (e.Action == NotifyCollectionChangedAction.Remove)
        {
            foreach (Mp3SongViewModel item in e.OldItems)
            {
                // Removed items
                item.PropertyChanged -= this.ItemPropertyChanged;
            }
        }
        else if (e.Action == NotifyCollectionChangedAction.Add)
        {
            foreach (Mp3SongViewModel item in e.NewItems)
            {
                // Added items
                item.PropertyChanged += this.ItemPropertyChanged;
            }
        }
    }

    public void ItemPropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        this.RaisePropertyChanged(() => this.Mp3SongViewModels);
    }

    public override void Cleanup()
    {
        Messenger.Default.Unregister<NotificationMessage<List<string>>>(this);
        Messenger.Default.Unregister<NotificationMessage<Mp3Tag>>(this);
        Messenger.Default.Unregister<NotificationMessage<Mp3SongViewModel>>(this);
        Messenger.Default.Unregister<NotificationMessage<string>>(this);
        base.Cleanup();
    }

    #endregion
}

Answer 1

Avoid using regions or use them only sparingly. Grouping fields or constructors doesn't make any sense and doesn't help anyone.

---

Avoid the this keyword. It's rarely necessary and it makes the code more verbose.

---

Avoid prefixes for parameters. I already know that paramMp3File is a parameter. The param prefix is redundant. It's better to prefix private fields with an underscore _ like _mp3File to keep parameters clean and the assignment easier _mp3File = mp3File;.

---

public string WishedFilePath

Wished file path? What kind of path is this? Why do I need two of them? This property shouldn't be there.

---

set { this.SetProperty(newValue => this.mp3File.Title = newValue, value); }

This is strange. Why don't you set the property directly?

---

ObjectBase

This is not a correct name for the base type. It should be something more concrete but still some kind of an abstraction. We use inheritance to add a layer of abstraction. ObjectBase does not abstract anything.

---

public void SaveAndRename()

Save and rename? Why would I want to do that? If I save something then either I save it or I save something as.

---

public void Undo()

Why is there no redo? What kind of edit I can cancel? This doesn't seem to be the right operation for this kind of object.

---

public bool FileExistsAlready

Simply Exists is enough. I know it's a file and I know if it exists then it exists already. There's no need to be more verbose then absolutely necessary.

        if ((this.WishedFilePath != this.FilePath) && this.fileModifier.FileExists(this.WishedFilePath))
        {
            return true;
        }

        return false;

There's no need for the if and so many braces. A simple return is all what you need:

        return
            WishedFilePath != this.FilePath && 
            fileModifier.FileExists(this.WishedFilePath);

---

private readonly IDialogService dialogService;
private readonly Mp3Song mp3Song;
private bool _isChecked;

Be consistent. Use the underscore _ for all private fields or none of them.

---

   #region Properties, Indexers

Indexers = properties.

---

 this.RaisePropertyChanged(() => this.InEditMode);

You seem to be using expressions. Be aware of the performance hit if you use it a lot. If you use C# 6 then this would be a better alternative:

RaisePropertyChanged(nameof(InEditMode));

---

    public void Rename(IMp3Tag paramMp3Tag)

The method is called rename but it doesn't rename anything. It changes other properties. This is confusing. You should call it UpdateTag or something like this.

---

private void UpdateProperties()
{
    PropertyInfo[] propertyInfos = this.GetAllPropertyInfos();

    foreach (PropertyInfo propertyInfo in propertyInfos)
    {
        this.RaisePropertyChanged(propertyInfo.Name);
    }
}

This does not update any properties and shouldn't be called like this. It raises property changed events. Call it NotifyPropertiesChanged or alike. The name of the method should always be clear about what it does.

---

private bool CanClearAlbum()
{
    if (string.IsNullOrEmpty(this.Album))
    {
        return false;
    }

    return true;
}

I don't see much value in this method. Clearing an empty album does not make any harm so I would allow doing this anytime.