Prettifier isn't pretty

Question

Some context

I'm implementing a self-closing pairs auto-completion feature in Rubberduck. One of the challenges is that the VBIDE API doesn't allow editing a line of code by "injecting" characters: we have to replace the entire line, and a side-effect of this is that it causes the VBE to parse/compile the line (since it's no longer "being edited"), which means a partial line of code like this:

msgbox(

Turns into this:

MsgBox (

And there's nothing we can do about it. Obviously this causes a serious problem with determining where the caret is supposed to be - ok maybe not in this particular case, but say you have this (where | denotes caret position):

dosomething     |, foo

If you type a " here, what we want to do is to end up with this:

dosomething     "|", foo

But we need to account for the VBE's "prettifying", and end up with this instead:

DoSomething "|", foo

So I needed a way to get the VBE to "prettify" the original code, while not losing track of where the caret needs to be - which is fundamental if I don't want Rubberduck's auto-completion to get in the way of typing code in the editor.

The Solution

So I wrote an ICodeStringPrettifier interface, and implemented it like this:

public class CodeStringPrettifier : ICodeStringPrettifier
{
    private readonly ICodeModule _module;

    public CodeStringPrettifier(ICodeModule module)
    {
        _module = module;
    }

    public CodeString Prettify(CodeString original)
    {
        var originalCode = original.Code;
        var originalPosition = original.CaretPosition.StartColumn;
        var originalNonSpacePosition = 0;
        for (var i = 0; i < originalPosition; i++)
        {
            if (originalCode[i] != ' ')
            {
                originalNonSpacePosition++;
            }
        }

        _module.DeleteLines(original.SnippetPosition.StartLine);
        _module.InsertLines(original.SnippetPosition.StartLine, originalCode);
        var prettifiedCode = _module.GetLines(original.SnippetPosition);

        var prettifiedNonSpacePosition = 0;
        var index = 0;
        for (var i = 0; i < prettifiedCode.Length; i++)
        {
            if (prettifiedCode[i] != ' ')
            {
                prettifiedNonSpacePosition++;
                if (prettifiedNonSpacePosition == originalNonSpacePosition)
                {
                    index = i;
                    break;
                }
            }
        }

        return new CodeString(prettifiedCode, new Selection(0, index + 1));
    }
}

Where CodeString is a struct that is a little more than a formalized (string,Selection) value tuple, that uses the pipe character exactly as shown at the top of this post, to denote the caret position in a code string - which is extremely handy for testing... but then, using this type with actual VBA code that includes an actual pipe character is problematic (#4373), but fixing that isn't in scope here.

I've written a few tests for the prettifier, and they all pass:

[TestFixture]
public class PrettifierTests
{
    [Test][Category("AutoComplete")]
    public void GivenSamePrettifiedCode_YieldsSameCodeString()
    {
        var original = "MsgBox (|".ToCodeString();
        var module = new Mock<ICodeModule>();
        module.Setup(m => m.GetLines(original.SnippetPosition)).Returns(original.Code);

        var sut = new CodeStringPrettifier(module.Object);
        var actual = sut.Prettify(original);

        Assert.AreEqual(original, actual);
    }

    [Test][Category("AutoComplete")]
    public void GivenTrailingWhitespace_PrettifiedCaretIsAtLastCharacter()
    {
        var original = "MsgBox |".ToCodeString();
        var prettified = "MsgBox";
        var expected = "MsgBox|".ToCodeString();
        var module = new Mock<ICodeModule>();
        module.Setup(m => m.GetLines(original.SnippetPosition)).Returns(prettified);

        var sut = new CodeStringPrettifier(module.Object);
        var actual = sut.Prettify(original);

        Assert.AreEqual(expected, actual);
    }

    [Test]
    [Category("AutoComplete")]
    public void GivenExtraWhitespace_PrettifiedCaretStillAtSameToken()
    {
        var original = "MsgBox      (\"test|\")".ToCodeString();
        var prettified = "MsgBox (\"test\")";
        var expected = "MsgBox (\"test|\")".ToCodeString();

        var module = new Mock<ICodeModule>();
        module.Setup(m => m.GetLines(original.SnippetPosition)).Returns(prettified);

        var sut = new CodeStringPrettifier(module.Object);
        var actual = sut.Prettify(original);

        Assert.AreEqual(expected, actual);
    }
}

Since this appears to work exactly as I need it, I'm going to start tweaking the self-closing pairs auto-completion feature to use it - now while it works, for a prettifier, I don't find the implementation particularly pretty. Ideas?

Answer 1

Not knowing anything about RubberDuck, I'm a little frightened by the necessity that the CodeString come from a particular ICodeModule, but that this is not (cannot be?) verified. Obviously if modules are not the only source of CodeStrings then this abstraction is probably necessary, but if the calling code is always aware of the module from which the code-string came then I'd be inclined to make this a static method taking both as parameters, so that you can't accidently use a CodeStringPrettifier for one module with a CodeString from another (especially so, since the class and interface have no means of conveying with which module they associate, which means consumers presented with an ICodeStringPrettifier have no real way to 'realise' that they are not interchangeable, not least because the class lacks inline documentation (///); does the interface have inline documentation?).

The method is readable enough if you ask me, but perhaps it would be better to split those loops out into their own methods. That would certainly reduce the number of variables floating around, and all the usual reasons why pulling self-contained logic out into independent methods is good.

I'd rename index to prettifiedPosition, which tallies with originalPosition and is much clearer as to what it represents. Alternatively, I'd be inclined to move the return within the loop, and not have the variable at all, and rather throw if the end of the loop is reached: no (immediately apparent) good can come from reaching the end of the loop, and this saves initialising index.

Replacing the return would break the (I'm guessing unexpected?) behaviour that occurs when there are no non-white-space characters before the caret: the second loop will run to completion, and index will be left as it was initialised. It might then need dedicated handling, but I think either way that it is better to clearly document such edge cases, rather than to leave them to work through non-obvious means (there is no explanation as to why index should be 0). Test-cases for this (e.g. caret at start; caret at start after " ") would also be nice, though I guess they wouldn't come under "AutoComplete".

Answer 2

Because I don't think you will add some binary prettifier, I would rename ICodeStringPrettifier and CodeStringPrettifier to ICodePrettifier and ICodePrettifier. It makes the names a little bit shorter and other developers wouldn't need to guess if there will be a CodeBinaryPrettifier as well.

---

To make the remaining code a little bit more pretty how about using some Linq ? The start of the method could look like so

public CodeString Prettify(CodeString original)
{
    var originalPosition = original.CaretPosition.StartColumn;
    var originalNonSpacePosition = original.Code
                                           .Take(originalPosition)
                                           .Count(c => c != ' ');

    _module.DeleteLines(original.SnippetPosition.StartLine);
    _module.InsertLines(original.SnippetPosition.StartLine, original.Code);
    var prettifiedCode = _module.GetLines(original.SnippetPosition);

In this way we can remove the originalCode as well.

As VisualMelon stated in his/her answer extracting the second loop to a separate method would shorten the Prettify() method as well. Another way could be to not count prettifiedNonSpacePosition but to decrease the originalNonSpacePosition and check if it equals 0 to break out of the loop like so

    var index = 0;
    for (var i = 0; i < prettifiedCode.Length; i++)
    {
        if (prettifiedCode[i] != ' ')
        {
            originalNonSpacePosition--;
            if (originalNonSpacePosition == 0)
            {
                index = i;
                break;
            }
        }
    }