patterncsharpMinor
Move Closer To Usage
Viewed 0 times
moveusagecloser
Problem
One of my latest refactorings for Rubberduck is Move Closer To Usage. The refactoring will take a field and move it just above the reference to it only if it is used in a single method, or take a variable declaration and move it just above its first call.
As promised, I moved many of the duplicated support methods into extension methods, although I have a few left that are duplicated between a few refactorings. Also, thanks to Thomas Eyde, I found some bugs in
Additionally, this refactoring does not have a user interface, so there is just one file. Be sure to let me know what you think!
```
public class MoveCloserToUsageRefactoring : IRefactoring
{
private readonly List _declarations;
private readonly IActiveCodePaneEditor _editor;
private readonly IMessageBox _messageBox;
public MoveCloserToUsageRefactoring(RubberduckParserState parseResult, IActiveCodePaneEditor editor, IMessageBox messageBox)
{
_declarations = parseResult.AllDeclarations.ToList();
_editor = editor;
_messageBox = messageBox;
}
public void Refactor()
{
var qualifiedSelection = _editor.GetSelection();
if (qualifiedSelection != null)
{
Refactor(_declarations.FindVariable(qualifiedSelection.Value));
}
else
{
_messageBox.Show("Invalid Selection.", "Rubberduck - Move Closer To Usage", System.Windows.Forms.MessageBoxButtons.OK,
System.Windows.Forms.MessageBoxIcon.Exclamation);
}
}
public void Refactor(QualifiedSelection selection)
{
Refactor(_declarations.FindVariable(selection));
}
public void Refactor(Declaration target)
{
if (target.DeclarationType != DeclarationType.Variable)
{
throw new ArgumentException(@"Invalid Argument", "target");
}
if (!target.References.Any())
{
var message = string.Format(RubberduckU
As promised, I moved many of the duplicated support methods into extension methods, although I have a few left that are duplicated between a few refactorings. Also, thanks to Thomas Eyde, I found some bugs in
RemoveComma(), which are now fixed.Additionally, this refactoring does not have a user interface, so there is just one file. Be sure to let me know what you think!
```
public class MoveCloserToUsageRefactoring : IRefactoring
{
private readonly List _declarations;
private readonly IActiveCodePaneEditor _editor;
private readonly IMessageBox _messageBox;
public MoveCloserToUsageRefactoring(RubberduckParserState parseResult, IActiveCodePaneEditor editor, IMessageBox messageBox)
{
_declarations = parseResult.AllDeclarations.ToList();
_editor = editor;
_messageBox = messageBox;
}
public void Refactor()
{
var qualifiedSelection = _editor.GetSelection();
if (qualifiedSelection != null)
{
Refactor(_declarations.FindVariable(qualifiedSelection.Value));
}
else
{
_messageBox.Show("Invalid Selection.", "Rubberduck - Move Closer To Usage", System.Windows.Forms.MessageBoxButtons.OK,
System.Windows.Forms.MessageBoxIcon.Exclamation);
}
}
public void Refactor(QualifiedSelection selection)
{
Refactor(_declarations.FindVariable(selection));
}
public void Refactor(Declaration target)
{
if (target.DeclarationType != DeclarationType.Variable)
{
throw new ArgumentException(@"Invalid Argument", "target");
}
if (!target.References.Any())
{
var message = string.Format(RubberduckU
Solution
There's not a lot to be said in my opinion. You've done a good job of using variables where they help clarify the intent and everything seems to be well named. The code is easy to read, even if some lines get kind of long and could be split across several lines. (Linq has a way of going sideways instead of down if you let it.)
There is this here.
I copied it in isolation to simulate what would happen if this gets thrown. Can you tell what exactly went wrong? No? Maybe some context would help.
Crystal clear, but we had to look at the code to know it. I'd recommend a better message.
Also note that there wasn't a need for the string literal identifier (
There's also this.
-
I'm not entirely sure why you fully qualified the MessageBox enums. If you don't want to import the
There is this here.
throw new ArgumentException(@"Invalid Argument", "target")I copied it in isolation to simulate what would happen if this gets thrown. Can you tell what exactly went wrong? No? Maybe some context would help.
if (target.DeclarationType != DeclarationType.Variable)
{
throw new ArgumentException(@"Invalid Argument", "target");Crystal clear, but we had to look at the code to know it. I'd recommend a better message.
if (target.DeclarationType != DeclarationType.Variable)
{
throw new ArgumentException("Invalid Argument. DeclarationType must be a Variable", "target");Also note that there wasn't a need for the string literal identifier (
@). There's also this.
{
_messageBox.Show("Invalid Selection.", "Rubberduck - Move Closer To Usage", System.Windows.Forms.MessageBoxButtons.OK,
System.Windows.Forms.MessageBoxIcon.Exclamation);
}- Those strings should be in a resource file so they can be localized. Rubberduck has been translated into several languages at this point.
-
I'm not entirely sure why you fully qualified the MessageBox enums. If you don't want to import the
Forms namespace, I understand. However, you could alias the enums that you need to interact with the IMessageBox abstraction. using MessageBoxButtons = System.Windows.Forms.MessageBoxButtons;
using MessageBoxIcons = System.Windows.Forms.MessageBoxIcons;Code Snippets
throw new ArgumentException(@"Invalid Argument", "target")if (target.DeclarationType != DeclarationType.Variable)
{
throw new ArgumentException(@"Invalid Argument", "target");if (target.DeclarationType != DeclarationType.Variable)
{
throw new ArgumentException("Invalid Argument. DeclarationType must be a Variable", "target");{
_messageBox.Show("Invalid Selection.", "Rubberduck - Move Closer To Usage", System.Windows.Forms.MessageBoxButtons.OK,
System.Windows.Forms.MessageBoxIcon.Exclamation);
}using MessageBoxButtons = System.Windows.Forms.MessageBoxButtons;
using MessageBoxIcons = System.Windows.Forms.MessageBoxIcons;Context
StackExchange Code Review Q#114997, answer score: 4
Revisions (0)
No revisions yet.