We changed our privacy policy. Read more.

Hot answers tagged

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 ...


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

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 ...


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

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

First, I have to mention that I'm very happy that you chose to explore antlr! ...looking at what you've done here, you now know more about Visitor and ErrorNode than I do! But ANTLR aside... Classes are internal unless specified otherwise; still it would be nice to be explicit about it, and stick that internal modifier in front of your internal classes. I ...


7

I'm not sure this counts as a Code Review answer, but I wanted to at least share what I found out after nearly a day of fighting with this... As I think you know, your grammar allows strings like "IIII" and "XXXX". This is definitively a bug, and a damn hard one to squish. Part of the problem is that Roman Numerals are not a context free language. (i.e. We ...


5

GetReplacementLine() Instead of using String.Replace() you should check out the TokenRewriteStream mentioned in this answer. If you need to use String.Replace() then you should omit the not needed call to String.Contains(). If the string which should be replaced isn't found in the 'content', the unchanged 'content' will be returned. In addition it will ...


5

ProcedureNode constructor The private constructor can be simplified / made be more readable by returning early if asType == null. You also shouldn't check for returnTypeClause == null twice. You could consider to change the private readonly string _returnType; to a "normal" string and add a private setter to the property. By doing this you could get rid ...


4

Avoid early and perhaps useless enums: public enum VBProcedureKind { Sub, Function, PropertyGet, PropertyLet, PropertySet } Enums are perhaps one of the most annoying, exploited, anti-pattern, anti oop, that no language should ever support kind of thing and ruins every programmer carrer. This was in fact so bad idea that you had a ...


4

You could take a page out of functional programming and shorten the visitors for the multiply, divide, subtract, and add: public override int VisitAddition(BasicMathParser.AdditionContext context) { return VisitHelper(context, (left, right) => left + right); } public override int VisitSubtraction(BasicMathParser.SubtractionContext context) { ...


3

Extract a whole class out of this code, call it IdentifierUsageInspector, and then have your identifier usage inspections use that class. Keep the methods simple, straightforward, and to the point, and the result will be (much) cleaner code. I boiled the GetInspectionResults implementation down to this: public IEnumerable<CodeInspectionResultBase> ...


3

I wanted to post an idea I had to address my concern over the brittleness of my unit test(s). I realised that since I am performing a string comparison, that I could use Regular Expressions. Then I can ignore parts of the parse tree string that are not interesting. So I have this as a potential solution: [Fact] public void ...


3

This is a big improvement from that last post! It so great that makes my eyes sparkle and brings joy. I think you are making the right use of the keyword partial here. This may be my personal taste but I don't really like to see #region keywords in the code. The only thing I see you could improve in your code at this moment is to extract those two lines: ...


3

//result.AddRange(declarations.Select(declaration => declaration.Context) // .OfType<VBParser.TypeStmtContext>() // .Select(context => // context.AmbiguousIdentifier().ToQualifiedContext(module.QualifiedName))); Commented code is dead code and dead code should be deleted. In ...


3

public struct IdentifierReference You should not be using structs unless you have a very good reason to. MSDN has the following guidelines for choosing between structs and classes: √ CONSIDER defining a struct instead of a class if instances of the type are small and commonly short-lived or are commonly embedded in other objects. X AVOID defining a ...


2

Going back to your original code. I think a little bit of reflection will get you out of the try catch block situation. public bool HasExplicitCallStatement() { var theObject = ((dynamic) Context.Parent); if(theObject.GetType().GetMethod("CALL") != null) { var call = theObject.CALL(); return call != null && call.Symbol....


2

Some basic refactoring: Make members static you can make IsIgnoredProcedure(), IsPublicModuleMember(), IsClassLifeCycleHandler(), IsInterfaceMember() and GetImplementedInterfaceMembers() static since they do not use any instance info. Alternatively, if each instance of ProcedureNotUsedInspection were to be immutable (i.e. determine the results at ...


2

Just an idea but maybe create a custom collection type to hold your variable classes and load the types into lookup dictionaries to make your property queries faster. This will obviously add some overhead at load, along with memory additions, but my guess is it will be worth it. Please let me know. class Program { static void Main(string[] args) { ...


2

One would expect that a method of a parser which returns an IParseTree would be named Parse() instead of startRule() which is in addition violating the naming guidelines. You should declare variables as near as possible to their usage. You could also omit the assignment of the call to Parse() public INode Parse(string projectName, string componentName, ...


2

You are basically building a compiler. You have your tokenizer and parser available, but your target language generator is missing. Sure, hardcoding in html can be done for a small use case, but I would prefer to use an existing API. Suggested API: HtmlTextWriter from System.Web. Alternative API: HtmlElement from System.Windows.Forms. Both API's allow ...


2

The way Antlr visitors and listeners work, there's little we can do to avoid redundant code - you've done quite a good job at keeping the redundancies minimal. Essentially the logic is the same in all overrides: return a new ExtractMethodValidationResult when context is contained in the user's selection, or the no-op base return value otherwise. You could ...


2

Let's start with the minor things: Personally, I like splitting my lexer grammar and parser grammar into two separate files. I understand that a lot of people don't like that though, so either way is fine. I split mine because the lexing and parsing are two separate logical steps that have nothing to do with each other. It also allows for some more powerful ...


2

Even though it is already quite good that you abstract most of the functionality using magic constants into their own function. However, it would help a lot if you gave all those magic constants a name and defined them up top. This way the code becomes easier to read and should the spec ever change, there is only one place where you have to change them. ...


2

print('fragment N : Nd | Nl | No ;') It probably doesn't matter, but there's an extraneous whitespace there, before the colon. And the whitespace before the semicolon isn't needed either, so you could output like this: print('fragment N : Nd | Nl | No;') Now, your script is generating fragment rules for a lexer grammar: I would expect to see a function ...


1

From a now deleted answer by Bart Kiers. You've given multiplication a higher precedence than division. Usually, these have the same precedence. I.e., the expression 1 / 2 * 3 is usually evaluated as (1 / 2) * 3 (from left to right), which is 1.5. But in your case, it will be evaluated as 1 / (2 * 3), which is 0.16666666666. Also, your NUMBER ...


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