HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Reordering method parameters

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
parametersmethodreordering

Problem

Next release of Rubberduck (should be 1.4) introduces a reorder parameters refactoring, which lets user reorder the parameters of a module (or class) member, and automatically adjust all usages.

Before:

After:

This is the IDialogView interface implementation:

interface IReorderParametersView : IDialogView
{
    Declaration Target { get; set; }
    List Parameters { get; set; }
    void InitializeParameterGrid();
}


And here is the Parameter class:

public class Parameter
{
    public string FullDeclaration { get; private set; }
    public int Index { get; private set; }
    public bool IsOptional { get; private set; }
    public bool IsParamArray { get; private set; }

    public Parameter(string fullDeclaration, int index)
    {
        FullDeclaration = fullDeclaration;
        Index = index;
        IsOptional = FullDeclaration.Contains("Optional");
        IsParamArray = FullDeclaration.Contains("ParamArray");
    }
}


This is the dialog code-behind:

```
public partial class ReorderParametersDialog : Form, IReorderParametersView
{
public List Parameters { get; set; }
private Parameter _selectedItem;
private Rectangle _dragBoxFromMouseDown;
Point _startPoint;
private int _newRowIndex;

public ReorderParametersDialog()
{
Parameters = new List();
InitializeComponent();
InitializeCaptions();

MethodParametersGrid.SelectionChanged += MethodParametersGrid_SelectionChanged;
MethodParametersGrid.MouseMove += MethodParametersGrid_MouseMove;
MethodParametersGrid.MouseDown += MethodParametersGrid_MouseDown;
MethodParametersGrid.DragOver += MethodParametersGrid_DragOver;
MethodParametersGrid.DragDrop += MethodParametersGrid_DragDrop;
}

private void InitializeCaptions()
{
OkButton.Text = RubberduckUI.OkButtonText;
CancelButton.Text = RubberduckUI.CancelButtonText;
Text = RubberduckUI.ReorderParamsDialog_Caption;
Tit

Solution

In the Show() method, this condition could be reversed into a positive one:

if (_view.Target != null)
{
    ...
}


...which would reduce nesting because if the _view.Target is null, then there's nothing to act upon and the method can return early:

if (_view.Target == null)
{
    return;
}


The name of the resource string RubberduckUI.ReorderPresenter_LessThanTwoVariablesError is slightly off - a better name would be ReorderPresenter_LessThanTwoParametersError, or perhaps ReorderPresenter_RequiresTwoOrMoreParametersError.

There's a similar scheme with ReorderPresenter_OptionalVariableError: a "variable" isn't a "parameter", and the resource string could have a name that better explains why this is happening - like ReorderPresenter_OptionalParametersMustBeLastError.

I don't like that you're referring to parameters as "variables" in multiple places; there's a DeclarationType.Variable enum member, and then there's a DeclarationType.Parameter enum member - they are two completely distinct things.

From a UX standpoint, there's an opportunity here: a Parameter knows when it's Optional, so you have a way to validate the data before the user hits the Ok button. One way to implement this could be to remove the column heading, and insert a column before the "Parameters" column, that would display some [x] icon (Rubberduck uses such an icon elsewhere for similar purposes) with a tooltip explaining what's wrong; the Ok button could be disabled when an icon is shown in that column.

This would greatly simplify the OnOkButtonClicked handler, which has a confusing name (I've been guilty of that too) - OnXxxx should be for procedures that raise an event, not handle it.

Why is AdjustSignatures plural? It's only ever processing one signature at a time, right?

A Declaration called reference is confusing:

private void AdjustSignatures(Declaration reference)


Either you have a Declaration, or you have one of its references!

Code Snippets

if (_view.Target != null)
{
    ...
}
if (_view.Target == null)
{
    return;
}
private void AdjustSignatures(Declaration reference)

Context

StackExchange Code Review Q#91221, answer score: 4

Revisions (0)

No revisions yet.