You Need CodeRush

Oops

The other day, I was chatting with some developers and one of them pulled me over to show me the following code:

private void SetLayout(Page page)
{
    if (null == page)
    {
        return;
    }
}

I did a double take and then smirked a bit, pointing out that this was probably dead code. I asked the person who was showing me this to do a “find all references” and there actually was one:

private void mainFrameNavigated(object sender, System.Windows.Navigation.NavigationEventArgs e)
{
    Page page = e.Content as Page;
    if (page != null)
    {
        SetLayout(page);
    }
}

So, we then backed it up a step and discovered that this was an event handler for an event declared in XAML (this whole thing was in a code-behind file). So, every time some event occurred (some sort of navigation thing), the code would cast the sender as a page, call a method with that page, and then return if the page was null, and also return if the page wasn’t null.

How Did It Come To This?

Looking at the SetLayout method, I quickly developed a hypothesis as to how the code wound up this way. The “SetLayout” method probably checked the page for null as a precondition prior to performing some sort of action on Page. At some point, that action became undesirable and was removed from the code, but whoever removed it didn’t realize that the precondition check was now meaningless, as was the whole chain of code above it for the event handler. This was confirmed correct by a quick glance through source control.

Here’s what this looks like in my IDE:

Do you see the dashes on the right, next to the scroll bar? Those are the issues in the CodeRush “issues list”. Do you see the squiggly underline of “SetLayout” (hard to see at this resolution, but you can zoom in or open the image in a new tab if you want)? That means there’s an issue here. In the interests of full disclosure, this is actually to tell me that the method could be static (it operates on no instance member of the class) rather than to tell me that the method has no effect on anything, but the important thing is that it’s underlined. It draws my attention to the method instead of encouraging me to skip over it while looking for something else here. And, as soon as this method catches anyone’s attention, it will obviously be deleted.

The Value of Productivity Tools

In general, the Code Rush issues list is a great companion. It alerts me to dead code, undisposed disposables, redundant statements, and plenty of other things. If you pause any time it tells you about an issue, you will become better at programming and you will start to recognize more problems. It isn’t necessarily superior to using, say, Style Cop or any other particular tool, but it is very convenient in that it paints the problems right inline with the IDE. They’re quite noticeable.

Code Rush’s competitor, Resharper does this as well. I plug for Code Rush because I use it and I love it, but if you get Resharper, that’s fine — just get something. These productivity tools are game changers. When looking at this code in someone else’s IDE, there was no squiggly and no immediately obvious problem if you’re casually scrolling through the class. But in my IDE, my eyes are drawn right to it, allowing and in fact demanding that I fix it. This sort of boy-scouting makes you better at writing your own code and better at fixing other people’s.

I forget what the license costs, but your employer can afford it. Heck, if they’re too cheap, you can afford it. Because what you can’t afford is to have code checked in under your name that looks like the code in this post. That’s no good, and it’s avoidable.

DXCore Plugin Part 3

In a previous post, I mentioned my ongoing progress with a DX Core plugin. I’d been using it successfully for some time now but had to dust it off again last week. Another convention that I’m required to abide for consistency sake but don’t much care for is explicit typing of my variables.

That is, I prefer declarations like:

var myTypeWithLongName = new TypeWithALongName();

rather than:

TypeWithALongName myTypeWithLongName = new TypeWithALongName();

I personally find the second to be needlessly syntactically noisy, and I don’t really see any benefit since the implicit (var) typing preserves strong typing and even causes the resulting type to show up in Intellisense. But, when in Rome…

Compared to my previous setup, this was relatively straightforward. Initially, I thought it would be even simpler than it turned out to be, since Code Rush itself supports the explicit/implicit conversion as an individual refactoring. I figured I’d just have to iterate through the file and call something like “Some.CodeRush.Namespace.MakeExplicit(myVariable)”.

Well, it didn’t turn out to be that easy (at least not that I found), but it did turn out to be relatively simple. Eliding the part about actually finding the variables, I was able to accomplish what I wanted with this method:

/// 
Convert the local variable declaration to FSG's standard of explicit typing
/// Variable object to convert
/// A string with the new type declaration
public string ConvertDeclaration(Variable variable)
{
    var myImplicit = variable as ImplicitVariable;
    string myString = variable.MemberType;
    if (myImplicit != null)
    {
        var myObjectCreation = myImplicit.Expression as ObjectCreationExpression;
        if (myObjectCreation != null && myObjectCreation.ObjectType != null)
        {
            myString = myObjectCreation.ObjectType.ToString();
        }

        var myArrayCreate = myImplicit.Expression as ArrayCreateExpression;
        if (myArrayCreate != null)
        {
            myString = myArrayCreate.BaseType.ToString();
        }
    }
    return myString;
}

This isn’t actully my finished ‘production’ code, but it’s easier to display in this format. I didn’t like the size of that method, so I created an abstract ExplicitExpressionConverter and gave it inheritors ObjectCreationExpressionConverter and ArrayCreateExpressionConverter, using a factory method creation pattern to figure out which to create, based on the variable. But while that abstraction makes the code cleaner, it makes the logic harder to document in a blog post.

So, anyway, the idea here is that you need to convert the variable you find into the appropriate type of expression. An object creation expression is standard allocation, and the array create expression is a special case. There are other special cases as well, such as null coalescing (var myFoo = someClass.GetAFoo() ?? new Foo()) and method call expression (var myFoo = someClass.GetAFoo()), but I have not yet figured out how to obtain the return type from those expressions. I’ll probably add a part 4 if/when I do.

DXCore Plugin Part 2

In the previous post on this subject, I created a basic DXCore plugin that, admittedly, had some warts. I started using it and discovered that there were subtle issues. If you’ll recall, the purpose of this was to create a plugin that would convert some simple stylistic elements between my preferences and those of a group that I work with. I noticed two things that caused me to revisit and correct my implementation: (1) if I did more than one operation in succession, things seemed to get confused and garbled in terms of variable naming; and (2) I was not getting renames in all scopes.

The main problem that I was having was the result of calling LanguageElement.Document.SetText(). Now, I don’t really understand the pertinent details of this because the API is a black box to me, but I believe the gist of it, based on experience and poking around blog posts, is that calling this explicitly causes the document to get out of sync if you chain renames together.

For instance, take the code:

void Foo()
{
    string myString = "asdf";
    int myInt = myString.Length();
}

The way that DXCore’s Document API appears to process this is with a concept called “NameRange.” That is, there are various ways you can use LanguageElements and other things in that inheritance tree to get a particular token in the source file: the string type, the Foo method signature, the “myString” variable, etc. When you actually want to change something name-wise, you need to find all references to your token and do an operation that essentially takes the parameters (SourceRange currentRange, string new text). In this fashion, you might call YourClass.Document.SetText(myStringDeclaration.NameRange, “newMyString”);

Assuming that you’ve rounded up the proper LanguageElement that corresponds to the declaration of the variable “myString”, this tells DXCore that you want to change the text from “myString” to “myNewString”. Conceptually, what you’re changing is represented by some int parameters that I believe correspond to row and column in the file, ala a 2D array. So, if you make a series of sequential changes to “myString” (first lengthen it, then shorten it, then lengthen it again) strange stuff starts to happen. I think this is the result of the actual allocated space for this token getting out of whack with what you’re setting it to. It sometimes starts to gobble up characters after the declaration like Windows when you hit the “insert” key without realizing it. I was winding up with stuff like “string myStringingingingd”;”

So, what I found as a workable fix to this problem was to use a FileChangeCollection object to aggregate the changes I wanted to make as I went, rather than making them all at once. FileChangeCollection takes a series of FileChange objects which want to know the path of the class, the range of the proposed change target, and the new value. I aggregated all of my changes in this collection and then flushed them at the end with CodeRush.File.ChangeFile(_targetClass.GetSourceFile(), _collection); After doing that, I cleared the collection so that my class can reuse it.

This cleared up the issue of inconsistent weirdness in naming. Now I can convert back and forth as many times as I please or run the same conversion over and over again and then run the other one, and atomicity of the standards application is perceived. If I run “convert to theirs, convert to theirs, convert to theirs, convert to mine,” the code ends up retaining my standards perfectly, regardless of whose it started with. This is due both to me getting DXCore right (at least as far as my testing so far proves) and also to my implementation being consistent and unit-tested. However, confidence that I have the DXCore implementation right now allows me in the future to know that, if things are wacky, it’s because I’m not implementing string manipulation and business rules correctly.

The second issue was that some variables weren’t getting renamed properly, depending on the scope. Things that were in methods were fine, but anything in nested Linq queries or loops or what-have-you were failing. I had previously made a change that I thought took care of this, but it turns out that I just pushed the problem down a level or two.

This I ultimately solved with some recursion. My conversion functions take an (element, targetScope) tuple, and they add FileChanges for all elements in the target scope. They then call the same function for all scoped children of targetScope with (element, targetScopeChild). If you think of the source as a tree with the root being your scope, intermediate nodes being scopes with children, and leaves being things containing no nested scoping language elements, this walks your source code as if it were a tree, finding and renaming everything.

Here is an example of my recursive function for adding changes to a class field “_collection”, which corresponds to a FileChangeCollection (no worries, I’m renaming that field to something more descriptive right after I finish this post 😀 )

/// Abstraction for converting all elements in a target scope
/// Element whose name we want to convert
/// Target scope in which to do the conversion
private void RecursiveAggregateLocalChanges(LanguageElement local, LanguageElement target)
{
    VerifyOrThrow();

    foreach (IElement myElement in local.FindAllReferences(target))
    {
        var myFieldInstance = CodeRush.Source.GetLanguageElement(myElement);
        if (myFieldInstance != null)
        {
            AddFileChange(_targetClass.GetFullPath(),
            myFieldInstance.NameRange, _converter.ConvertLocal(local.Name));
        }
    }

    foreach (LanguageElement myElement in target.Nodes)
    {
        RecursiveAggregateLocalChanges(local, myElement);
    }
}

You want to preserve “local” as a recursion invariant, since this method is called by the method that handles cycling through all local variables in the file and invoking the recursion to change their names. That is, the root of the recursion is given a single local variable to change as well as the target scope of the method it resides in. From here, you change the local everywhere it appears within the method, then you get all of the methods child scopes and do the same on those. You keep recursing until you run out of nested scopes, falling out of the recursion at this point having done your adds.

It does not matter whether you recurse first or last because CodeRush keeps track of the original element regardless and even if it didn’t, you’re only aggregating eventual changes here – not making them as you go – so you don’t wind up losing the root.

Hopefully this continues to be somewhat helpful. I know the DXCore API documentation is in the works, so this is truly the diary of someone who is just tinkering and reverse engineering (with a bit of help from piecing together things on other blogs) to get something done. I’m hardly an expert, but I’m more of one than I was when I started this project, and I find that the most helpful documentation is often that made by someone undertaking a process for the first time because, unlike an expert, all the weird little caveats and gotchas are on display since they don’t know the work-arounds by heart.

I will also update the source code here in a moment, so it’ll be fresh with my latest changes.