6
\$\begingroup\$

When generating a string representation of expression trees, I would like to render calls to String.Format with a constant string as the first parameter, as interpolated strings (link). In order to do so, I have to parse the compiler-generated composite format string.

I've written the following function for this purpose, based on the .NET Core parsing function. I am looking primarily for review in:

  • correctness, even in edge cases
  • clarity / readability

The function returns an array of tuples. Each tuple contains the following elements:

  1. literal string until the next placeholder
  2. index of placeholder
  3. alignment
  4. item format

If there is literal text after the last placeholder, it will be added as the last tuple of the array (other elements of the tuple will be null).


The function defines 3 local functions:

  • advanceChar -- advances the current position (pos) by one character, and stores the current character (ch)
  • skipWhitespace -- advances the current position as long as the current character is a space
  • getNumber -- gets a multi-digit number starting from the current position; ignores leading/trailing whitespace

public static (string literal, int? index, int? alignment, string itemFormat)[] ParseFormatString(string format) {
    const int indexLimit = 1000000;
    const int alignmentLimit = 100000;

    int pos = -1;
    char ch = '\x0';
    int lastPos = format.Length - 1;

    var parts = new List<(string literal, int? index, int? alignment, string itemFormat)>();

    while (true) {

        // Parse literal until argument placeholder
        string literal = "";
        while (pos < lastPos) {
            advanceChar();

            if (ch == '}') {
                advanceChar();
                if (ch == '}') {
                    literal += '}';
                } else {
                    throw new Exception("Mismatched end brace");
                }
            } else if (ch == '{') {
                advanceChar();
                if (ch == '{') {
                    literal += '{';
                } else {
                    break;
                }
            } else {
                literal += ch;
            }
        }

        if (pos == lastPos) {
            if (literal != "") {
                parts.Add((literal, (int?)null, (int?)null, (string)null));
            }
            break;
        }

        // Parse index section; required
        int index = getNumber(indexLimit);

        // Parse alignment; optional
        int? alignment = null;
        if (ch == ',') {
            advanceChar();
            alignment = getNumber(alignmentLimit, true);
        }

        // Parse item format; optional
        string itemFormat = null;
        if (ch == ':') {
            advanceChar();
            if (ch == '{') {
                advanceChar();
                if (ch == '{') {
                    itemFormat += '{';
                } else {
                    throw new Exception("Nested placeholders not allowed");
                }
            } else if (ch == '}') {
                advanceChar();
                if (ch=='}') {
                    itemFormat += '}';
                } else {
                    break;
                }
            } else {
                itemFormat += ch;
            }
        }

        parts.Add((literal, index, alignment, itemFormat));
    }

    return parts.ToArray();

    void advanceChar(bool ignoreEnd = false) {
        pos += 1;
        if (pos <= lastPos) {
            ch = format[pos];
        } else if (ignoreEnd) {
            ch = '\x0';
        } else {
            throw new Exception("Unexpected end of text");
        }
    }

    void skipWhitespace() {
        while (ch == ' ') {
            advanceChar(true);
        }
    }

    int getNumber(int limit, bool allowNegative = false) {
        skipWhitespace();

        bool isNegative = false;
        if (allowNegative && ch == '-') {
            isNegative = true;
            advanceChar();
        }
        if (ch < '0' || ch > '9') { throw new Exception("Expected digit"); }
        int ret = 0;
        do {
            ret = ret * 10 + ch - '0';
            advanceChar();
        } while (ch >= '0' && ch <= '9' && ret < limit);

        skipWhitespace();

        return ret * (isNegative ? -1 : 1);
    }
}
\$\endgroup\$
8
\$\begingroup\$

Problems:

Item format parsing is broken:

  • "{0:X2}" fails with an 'Unexpected end of text' exception, while "{0:X2}a" fails with a 'Mismatched end brace' exception. Both are valid formats.
  • "{0:}" also fails with an 'Unexpected end of text' exception, but "{0:}a" returns an empty array instead. Both are valid formats.
  • "{0:{{" and "{0:}}" are parsed successfully. Both should be rejected as invalid.

Improvements:

  • The index and alignment limits seem fairly arbitrary. If they're based on an actual limit it would be a good idea to document that. Also, exceeding those limits results in a misleading 'Mismatched end brace' error.
  • I'd recommend using a more specific exception exception type. The existing FormatException seems appropriate here.
  • For repeated string concatenation, a StringBuilder is (significantly) more efficient.
  • The exceptions don't provide much detail. It would be useful to know at what index the problem was detected, or what the parser was expecting when it hit the end of the input.
  • The main while loop body is fairly drawn out. If you're using local functions anyway, why not split things up further into a ParseLiteralPart and ParseFormatPart function?
\$\endgroup\$
  • 2
    \$\begingroup\$ mhmm... based on your findings I'm thinking of flagging the question as not-working-code... \$\endgroup\$ – t3chb0t Mar 14 at 15:08
  • \$\begingroup\$ @t3chb0t I've identified and fixed the problems (still working on the improvements); should I edit the code in the question? Should I close this and reopen another question? \$\endgroup\$ – Zev Spitz Mar 14 at 22:46
  • \$\begingroup\$ @ZevSpitz nope, editing the code is not allowed when there are already answers. What you can do is to either add a self-answer or ask a follow-up question if you'd like to have another review. \$\endgroup\$ – t3chb0t Mar 15 at 9:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.