Resx Translation Helper, V.2.0 Remove Files Window

Related to Resx Translation Helper, V.2.0

This is the code for the Remove Files window. I was able to keep code out of the code-behind here, but I have a feeling I botched something else.

IDisplayOpenFiles.cs:

public interface IDisplayOpenFiles
{
    object DataContext { set; }

    event EventHandler Closed;

    void Show();
    void Close();
}

DisplayOpenFiles.xaml:

DisplayOpenFilesVM.cs:

```
public class Files
{
public string Value { get; private set; }
public bool Selected { get; set; }

public Files(string value, bool selected = false)
{
Value = value;
Selected = selected;
}
}

public class DisplayOpenFilesVM
{
private readonly IDisplayOpenFiles _window;
public ObservableCollection OpenFiles { get; set; }

private ICommand _removeSelectedFiles;
public ICommand RemoveSelectedFiles
{
get
{
return _removeSelectedFiles ?? (_removeSelectedFiles = new RelayCommand
(
param =>
{
OnRemoveFiles(OpenFiles.Where(f => f.Selected).Select(f => f.Value));
_window.Close();
}
));
}
}

private ICommand _cancelRemoveFiles;
public ICommand CancelRemoveFiles
{
get
{
return _cancelRemoveFiles ?? (_cancelRemoveFiles = new RelayCommand
(
param =>
{
OnRemoveFiles(new List());
_window.Close();
}
));
}
}

public DisplayOpenFilesVM(IDisplayOpenFiles window, IEnumerable openFiles)
{
_window = window;
OpenFiles = new ObservableCollection();
foreach (var file in openFiles)
{
OpenFiles.Add(new Files

public class Files
{
    public string Value { get; private set; }
    public bool Selected { get; set; }

    public Files(string value, bool selected = false)
    {
        Value = value;
        Selected = selected;
    }
}

Huh? Files? Plural? There's no collection here. Just File is good. And this confirms it.

OpenFiles = new ObservableCollection();

What does Value represent? Is it a name? Full file path? What is it man?! =;)-

Seriously though. I've no idea. A better property name is in order.

var handler = FilesSelected;
    if (handler != null && !_eventFired)    // only fire once
    {
        _eventFired = true;
        handler(this, selectedFiles);
    }

Potentially useless comment, but I don't suppose it's causing any active harm at the moment. What I'm more concerned about is setting _eventFired to true before the event has actually fired. The chances of it not firing after setting the boolean variable are slim to none, but it's still odd enough that it caught my eye.

_window.Closed += (s, e) =>
    {
        OnRemoveFiles(new List());
    };

There are so many examples of using one letter variables for lambda expressions out there that I don't blame you. I don't. We all do it, and often we shouldn't. I think this is one of those times. Sure, I think I know what these stand for, but am I certain? No. Either way, I had to think about it longer than I should have.

public interface IDisplayOpenFiles
{
    object DataContext { set; }

    event EventHandler Closed;

    void Show();
    void Close();
}

I'm always a bit wary of "set only" properties. I would take a moment to consider why you're doing this and if there's a better way. If you've done that, and decided that this is good, leave a comment explaining why it's good.

I feel like that was just a whole bunch of nitpicking though. It seems to me that you've really improved as a developer over the last few months.

StackExchange Code Review Q#94680, answer score: 4