patterncsharpMinor
Reordering method parameters
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
And here is the
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
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
...which would reduce nesting because if the
The name of the resource string
There's a similar scheme with
I don't like that you're referring to parameters as "variables" in multiple places; there's a
From a UX standpoint, there's an opportunity here: a
This would greatly simplify the
Why is
A
Either you have a
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.