FindAllReferencesCommand 2.0

Rubberduck 2.0 will feature a "Search Results" dockable toolwindow that will be used for displaying the results of Find all references, Find all implementations, and whatever else we need to find. It replaces the ugly 1.x "SimpleList" view:

It all starts with a command. Here, the FindAllReferencesCommand:

```
///
/// A command that locates all references to a specified identifier, or of the active code module.
///
[ComVisible(false)]
public class FindAllReferencesCommand : CommandBase
{
private readonly INavigateCommand _navigateCommand;
private readonly RubberduckParserState _state;
private readonly IActiveCodePaneEditor _editor;
private readonly ISearchResultsWindowViewModel _viewModel;
private readonly SearchResultPresenterInstanceManager _presenterService;

public FindAllReferencesCommand(INavigateCommand navigateCommand, RubberduckParserState state, IActiveCodePaneEditor editor, ISearchResultsWindowViewModel viewModel, SearchResultPresenterInstanceManager presenterService)
{
_navigateCommand = navigateCommand;
_state = state;
_editor = editor;
_viewModel = viewModel;
_presenterService = presenterService;
}

public override void Execute(object parameter)
{
if (_state.Status != ParserState.Ready)
{
return;
}

var declaration = FindTarget(parameter);
if (declaration == null)
{
return;
}

var viewModel = CreateViewModel(declaration);
_viewModel.AddTab(viewModel);
_viewModel.SelectedTab = viewModel;

try
{
var presenter = _presenterService.Presenter(_viewModel);
presenter.Show();
}
catch (Exception e)
{
Console.WriteLine(e);
}
}

private SearchResultsViewModel CreateViewModel(Declaration declaration)
{
var results = declaration.References.Select(reference =>
new Sea

If FindAllReferencesCommand is ICommand implementation, then shouldn't simple checks such as

if (_state.Status != ParserState.Ready)
{
    return;
}

go into CanExecute portion of a command?

I'm not sure I like the idea of creating viewmodel in a command and then passing it to presenter. First, I feel like it should be presenter's job to create viewmodel and manage its lifetime. Second, the current implementation has really confusing semantics. Consider this:

var p1 = _presenterService.Presenter(new SearchResultsViewModel());
var p2 = _presenterService.Presenter(new SearchResultsViewModel());

What should happen? Well, it's hard to tell. First guess that comes to mind: there are now two different presenters with different views and different viewmodels. Second guess: there is a single presenter, and each call of Presenter method simply replaces the DataContext of a view. But both of those guesses are incorrect, and I would have never guessed correctly what actually happens.

CreatePresenter() instead of Presenter()? It might be a matter of taste, but I like it when there is a verb in method name.

I'm not a fan of having ViewModel property on my views. One of the advantages of MVVM is that it allows you to have your view and viewmodel loosely coupled. By declaring a strongly typed ViewModel property on view you lose this advantage. I mean can't you just store the viewmodel reference as a field in your presenter?

You have a lot of nested if's which can be inverted to reduce nesting or even removed completely, such as this one:

if (declaration == null)
    {
        return null;
    }
}
return declaration;

Removing the first three lines does not change anything.

There are also a lot of returns, but very little explanation to the user why the command returned early. Is this handled somewhere else? I mean if, say, the selection is null (which I assume is the text user selected) and your command returns early without doing anything, how does the user know why it doesn't work? If such situation should be impossible you should throw an exception. If it is possible, you should provide some sort of feedback for the sake of good UX.

I think you forgot to unsubscribe from viewModel.Close += viewModel_Close;. Better safe than sorry.

You should also consider renaming the handler, so it does not look so out of place in otherwise conventional code. OnTabClosed is a better name.

You should have an extension method for that:

var handler = LastTabClosed;
if (handler != null)
{
    handler.Invoke(this, EventArgs.Empty);
}

I also tend to use Action instead of EventHandler whenever I need neither sender, nor arguments. Yes, someone, somewhere on MSDN says that you should always use EventHandler for events blah-blah, but unless the event is exposed as part of public API I see no reason why you should be dragging those empty arguments around.

P.S. I'll be honest, this is not the first time I review a code marked with rubberduck tag, but I still have no clue what rubberduck (or VBA) is. But oh well, perhaps one day... Hopefully MVVM is the same everywhere you go. :)

StackExchange Code Review Q#120649, answer score: 2