20

if (moduleName == null) { moduleName = componentType == vbext_ComponentType.vbext_ct_StdModule ? "Module1" : componentType == vbext_ComponentType.vbext_ct_ClassModule ? "Class1" : componentType == vbext_ComponentType.vbext_ct_MSForm ? "Form1" : "Document1"; } Sorry, that I have ...


17

Starting from this var procedures = node.FindAllProcedures().Where(procedure => procedure.Parameters.Any(parameter => !string.IsNullOrEmpty(parameter.Instruction.Value))); var targets = procedures.Where(procedure => procedure.Parameters.Any(parameter => parameter.IsImplicitByRef) && !procedure....


17

/// <summary> /// Registers specified delegate for specified key combination. /// </summary> /// <param name="key">The key combination string, including modifiers ('+': Shift, '%': Alt, '^': Control).</param> /// <param name="action">Any <c>void</c>, parameterless method that handles the hotkey.</param> void ...


17

public void Refactor() { if (_targetDeclaration == null) { _messageBox.Show(RubberduckUI.PromoteVariable_InvalidSelection); return; } RemoveVariable(); AddField(); } Not sure if anyone other than Rubberduck's architect could point this one out, but it doesn't quite feel right. The parameterless overload of void Refactor()...


16

Almost all of this code can (should?) be moved directly into the MockFactory. [TestInitialize] public void Initialize() { _ide = MockFactory.CreateVbeMock(); _ide.SetupProperty(m => m.ActiveCodePane); _ide.SetupProperty(m => m.ActiveVBProject); _ide.SetupGet(m => m....


14

I don't know how I feel about this ternary. public override IDictionary<string, Action<VBE>> GetQuickFixes() { return !Handled ? new Dictionary<string, Action<VBE>> { {"Pass parameter by value", PassParameterByVal}, {"Pass parameter by reference explicitly", PassParameterByRef} ...


12

Having worked on a similar system, I came up with a very similar design. So either we're both doing something right, or we're both doing it wrong :) Here are some things I would change, but obviously different requirements call for different decisions and may not all be applicable here. Remove CodeInspection class I don't think you gain much from having a ...


12

public override IDictionary<string, Action<VBE>> GetQuickFixes() { return !Handled ? new Dictionary<string, Action<VBE>> { {"Pass parameter by value", PassParameterByVal}, {"Pass parameter by reference explicitly", PassParameterByRef} } : new Dictionary<...


12

I can't see it because of some missing classes but are the types being returned also immutable? You've made types like Declaration nicely immutable but if the complex objects themselves are mutable then that is still worrisome. Unless you want this behaviour so you can easily change semantics, but I doubt it considering that would cause a lot of ...


11

var newContent = string.Concat(ReservedKeywords.ByVal, " ", Instruction.Value); Here's a nifty thing I learned recently: var newContent = ReservedKeywords.ByVal + " " + Instruction.Value; will be optimized by the compiler to a call to string.Concat, exactly the same as above. It's also more readable, so you have nothing to lose. You can read more about it ...


11

I wasn't originally going to post this answer as it seems like petty things, but alas, here it is. I mean no offense by any of it, sometimes I tend to type things out to seem that way, but it's all in good spirit. :) Readability issues I was reading through your code and couldn't figure out why it was so hard to read, until I realized you seem to be trying ...


11

public IntroduceField(RubberduckParserState parseResult, IActiveCodePaneEditor editor, IMessageBox messageBox) { _declarations = parseResult.AllDeclarations.ToList(); _editor = editor; _messageBox = messageBox; } why don't you pass an IEnumerable<Declaration> instead of a RubberduckParserState object here ? Right now the IntroduceField(...


10

First off, I noticed this in ExtractMethodRefactoring: if (identifiers.TryGetValue(reference.Identifier, out key)) { if (!result.ContainsKey(key)) { result.Add(key, ExtractedDeclarationUsage.UsedAfterSelection); } } That should be one if: if (identifiers.TryGetValue(reference.Identifier, out key) && !result.ContainsKey(key)) { ...


10

Your GetParentMethodDeclarations() method has a lot of repeated code when it loops over the positionSelection collections. Considering all of them are the same, you can extract them to a helper method and do it once there. At the same time I would rename result as well, not in the least because that would've made it easier for a poor reviewer like me to ...


10

First of all - I like it. It's definitely readable and the naming and style seem good. I'm going to focus on a single point really (at least for now): Is ModuleScope a bird class or a plane string? It looks like it's both: private string ModuleScope { get { return _projectName + "." + _componentName; } } vs case DeclarationType.Module: return ...


10

public static final String RUBBERDUCK_PATH = "RetailCoder.VBE/UI"; This value shouldn't be compiled into the binary. It should be a configurable setting that's modifiable without needing to recompile. "RetailCoder.VBE" is a relic of the original project, before it was even called "Rubberduck", when the Mug formerly known as @retailcoder was mucking around ...


10

Implement IEquatable<T> Generally speaking, whenever I find myself writing overrides for object.Equals and object.GetHashCode, I also make the type implement IEquatable<T>. It's generally pretty easy to do so by calling the IEquatable<T>.Equals(T) method from the object.Equals(object) override: public override bool Equals(object obj) { ...


10

I think you should probably group public members at the top of your class (constructor being first) and private members at the bottom. It is quite hard to navigate classes where public and private methods are mixed together. Your class clearly has more then one responsibility. You should at least extract parsing logic (GetKey, GetModifierKey, etc.) to ...


10

Exceptions throw new ArgumentException("Invalid declaration type"); Even though there is only one parameter in this method, I should provide the argument name so the person debugging doesn't have to look. This is done in .NET 4.5, which I am using, as: throw new ArgumentException("Invalid declaration type", "target"); For reference, C# 6.0 has a nameof()...


10

You don't check your entry parameters for null. In FindSelection, this query could be reduced : var target = _declarations .Where(item => !item.IsBuiltIn) .FirstOrDefault(item => item.IsSelected(selection) && item.DeclarationType == DeclarationType.Variable || item.References.Any(r => r.IsSelected(...


8

This doesn't address your actually question (simplifying the logic/implementation), but I feel like it's important. For the sake of readability, I would recommend replacing some of the nested lambdas with a function reference to a named methods. The assignment to globals in the first section of code has 5 levels of indentation. That is a bit much for an ...


8

Reduce variable span and live time Let me quote an interesting paragraph from Code Complete: The code between references to a variable is a “window of vulnerability.” In the window, new code might be added, inadvertently altering the variable, or someone reading the code might forget the value the variable is supposed to contain. It’s always a good ...


8

(Not a full review, just focusing on one method.) I think private string GetImageKeyForDeclaration(Declaration declaration) contains a lot of repeated code that can be significantly reduced. First of all I'm not a fan of the "use a switch to set a value that you then return"-approach: just do the return directly, especially if there are a lot of cases. I'...


7

Let's tackle this piece of code, shall we? public IEnumerable<CommentNode> ParseComments(VBComponent component) { var code = component.CodeModule.Code(); var qualifiedName = new QualifiedModuleName(component.Collection.Parent.Name, component.Name); var commentBuilder = new StringBuilder(); var continuing = false; var startLine = ...


7

Regex-nightmare By looking at the returned value of IdentifierDeclarationSyntax (?<declarations>(?:(?<identifier>(?:[a-zA-Z][a-zA-Z0-9_]*)|(?:\[[a-zA-Z0-9_]*\]))(?<specifier>[%&@!#$])?(?<array>\((?<size>(([0-9]+)\,?\s?)*|([0-9]+\sTo\s[0-9]+\,?\s?)+)\))?(?<as>\sAs(\s(?<initializer>New))?\s(?:(?<reference&...


7

This looks good! I've got a couple of comments to make in a vague, hand wavy way so I'll start off with a comment about the code as written: public SomeMethod(int beforeIndex = -1) { if (beforeIndex == -1) { No! Bad Mug. No presents for you this Christmas. Use a Nullable<int> and be more idiomatic: public SomeMethod(int? beforeIndex = null) ...


7

TestMethod The Result property (and IsEditing property) public TestResult Result { get { return _result; } set { _result = value; OnPropertyChanged();} } seems only to be set by the TestMethod object, so there is no need to have a public setter there. Making the setter either private or protected will increase its encapsulation. The ...


7

Nested ternary conditionals are ugly. Ternary conditionals nested three deep are ugly to the third power: Status = _moduleStates.Values.Any(value => value == ParserState.Error) ? ParserState.Error : _moduleStates.Values.Any(value => value == ParserState.Parsing) ? ParserState.Parsing : _moduleStates.Values.Any(value => ...


7

In HookEventArgs, do you have multiple keys or a single one? Because this : public Keys Key { get { return _key; } } is confusing! Keys is an enum. According to the naming guideline, your enum should be pluralized if it's a Flag enum. I don't know if that's your case, but if so the property name should be Keys, otherwise the enum should be Key. And ...


6

Extract methods, extract methods everywhere, PLEASE! It's embarassing, after extracting the body of this code: foreach (var instruction in instructions) { var parsed = false; foreach (var syntax in grammar) { SyntaxTreeNode node; if (!syntax.IsMatch(publicScope, localScope, instruction, out node)) { continue; ...


Only top voted, non community-wiki answers of a minimum length are eligible