5
\$\begingroup\$

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:

<Grid>
    <Grid.RowDefinitions>
        <RowDefinition />
        <RowDefinition Height="40" />
    </Grid.RowDefinitions>
    <DataGrid ItemsSource="{Binding OpenFiles}" AutoGenerateColumns="False"
              CanUserAddRows="False" CanUserDeleteRows="False" ScrollViewer.HorizontalScrollBarVisibility="Disabled">
        <DataGrid.Columns>
            <DataGridCheckBoxColumn CanUserSort="True" IsReadOnly="False" Binding="{Binding Selected, Mode=TwoWay}" />
            <DataGridTextColumn CanUserSort="True" Binding="{Binding Value}" IsReadOnly="True" Width="600" />
        </DataGrid.Columns>
    </DataGrid>

    <Grid Grid.Row="1">
        <Grid.ColumnDefinitions>
            <ColumnDefinition />
            <ColumnDefinition />
        </Grid.ColumnDefinitions>
        <Button Grid.Column="0" Content="{x:Static properties:Resources.RemoveFilesCancel}" Command="{Binding CancelRemoveFiles}" />
        <Button Grid.Column="1" Content="{x:Static properties:Resources.RemoveFilesRemove}" Command="{Binding RemoveSelectedFiles}" />
    </Grid>
</Grid>

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<Files> 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<string>());
                    _window.Close();
                }
            ));
        }
    }

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

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

        window.DataContext = this;
    }

    public void Show()
    {
        _window.Show();
    }

    private bool _eventFired;
    public event EventHandler<IEnumerable<string>> FilesSelected;
    protected virtual void OnRemoveFiles(IEnumerable<string> selectedFiles)
    {
        var handler = FilesSelected;
        if (handler != null && !_eventFired)    // only fire once
        {
            _eventFired = true;
            handler(this, selectedFiles);
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$
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<Files>();

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<string>());
    };

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.

\$\endgroup\$
4
  • \$\begingroup\$ If there's using System.IO;, then File clashes with System.IO.File. \$\endgroup\$ – Mathieu Guindon Jun 26 '15 at 2:03
  • \$\begingroup\$ That's what using directives are for @Mat'sMug. =;)- \$\endgroup\$ – RubberDuck Jun 26 '15 at 2:57
  • \$\begingroup\$ I'd argue that File would be a confusing name nonetheless, and that FileViewModel would be much, much better ;-) \$\endgroup\$ – Mathieu Guindon Jun 26 '15 at 3:16
  • \$\begingroup\$ Eh. I've never liked that naming scheme. Smells of Hungarian to me. It doesn't say anything about the object except that it lives in a program that uses MVVM architecture @Mat'sMug. But sure. Resolves the conflict. \$\endgroup\$ – RubberDuck Jun 26 '15 at 8:35
1
\$\begingroup\$

My event is doing too much. When you look at the call, it is signaling both that the user has clicked the Remove button to remove the selected files, and that the user has clicked the Cancel button or the Close 'X' button, and wants to close the window. This is better off as two events.

Previous call:

var thread = new Thread(() =>
{
    var displayOpenFilesVM = new DisplayOpenFilesVM(
        new DisplayOpenFiles(),
        _components.Select(c => c.FilePath));

    displayOpenFilesVM.Show();

    displayOpenFilesVM.FilesSelected += (s, e) =>
    {
        filesSelected = e.ToList();

        Dispatcher.CurrentDispatcher.BeginInvokeShutdown(DispatcherPriority.Normal);
    };

    Dispatcher.Run();
});

Updated to use two events:

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

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

public DisplayOpenFilesVM(IDisplayOpenFiles window, IEnumerable<string> openFiles)
{
    _window.Closed += (sender, selectedFiles) =>
    {
        OnWindowClosed();
    };
}

public event EventHandler<IEnumerable<string>> FilesSelected;
protected virtual void OnRemoveFiles(IEnumerable<string> selectedFiles)
{
    var handler = FilesSelected;
    if (handler != null)
    {
        handler(this, selectedFiles);
    }
}

public event EventHandler<EventArgs> WindowClosed;
protected virtual void OnWindowClosed()
{
    var handler = WindowClosed;
    if (handler != null)
    {
        handler(this, EventArgs.Empty);
    }
}

Now it only signals that values are selected and should be removed when the Remove button is clicked, and the WindowClosed event fires to handle the window being closed:

var thread = new Thread(() =>
{
    var displayOpenFilesVM = new DisplayOpenFilesVM(
        new DisplayOpenFiles(),
        _components.Select(c => c.FilePath));

    displayOpenFilesVM.Show();

    displayOpenFilesVM.FilesSelected += (sender, e) =>
    {
        filesSelected = e.ToList();
    };

    displayOpenFilesVM.WindowClosed += (sender, e) =>
    {
        Dispatcher.CurrentDispatcher.BeginInvokeShutdown(DispatcherPriority.Normal);
    };

    Dispatcher.Run();
});
\$\endgroup\$

Your Answer

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