MP3 tag Project in MVVM style

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))

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.

StackExchange Code Review Q#146601, answer score: 4