views:

326

answers:

2

Is this code even complex enough to deserve a higher level of abstraction?

public static JsonStructure Parse(string jsonText)
{
    var result = default(JsonStructure);
    var structureStack = new Stack<JsonStructure>();
    var keyStack = new Stack<string>();
    var current = default(JsonStructure);
    var currentState = ParserState.Begin;
    var key = default(string);
    var value = default(object);

    foreach (var token in Lexer.Tokenize(jsonText))
    {
        switch (currentState)
        {
            case ParserState.Begin:
                switch (token.Type)
                {
                    case TokenType.BeginObject:
                        currentState = ParserState.Name;
                        current = result = new JsonObject();
                        break;
                    case TokenType.BeginArray:
                        currentState = ParserState.Value;
                        current = result = new JsonArray();
                        break;
                    default:
                        throw new JsonException(token, currentState);
                }
                break;
            case ParserState.Name:
                switch (token.Type)
                {
                    case TokenType.String:
                        currentState = ParserState.NameSeparator;
                        key = (string)token.Value;
                        break;
                    default:
                        throw new JsonException(token, currentState);
                }
                break;
            case ParserState.NameSeparator:
                switch (token.Type)
                {
                    case TokenType.NameSeparator:
                        currentState = ParserState.Value;
                        break;
                    default:
                        throw new JsonException(token, currentState);
                }
                break;
            case ParserState.Value:
                switch (token.Type)
                {
                    case TokenType.Number:
                    case TokenType.String:
                    case TokenType.True:
                    case TokenType.False:
                    case TokenType.Null:
                        currentState = ParserState.ValueSeparator;
                        value = token.Value;
                        break;
                    case TokenType.BeginObject:
                        structureStack.Push(current);
                        keyStack.Push(key);
                        currentState = ParserState.Name;
                        current = new JsonObject();
                        break;
                    case TokenType.BeginArray:
                        structureStack.Push(current);
                        currentState = ParserState.Value;
                        current = new JsonArray();
                        break;
                    default:
                        throw new JsonException(token, currentState);
                }
                break;
            case ParserState.ValueSeparator:
                var jsonObject = (current as JsonObject);
                var jsonArray = (current as JsonArray);
                if (jsonObject != null)
                {
                    jsonObject.Add(key, value);
                    currentState = ParserState.Name;
                }
                if (jsonArray != null)
                {
                    jsonArray.Add(value);
                    currentState = ParserState.Value;
                }
                switch (token.Type)
                {
                    case TokenType.EndObject:
                    case TokenType.EndArray:
                        currentState = ParserState.End;
                        break;
                    case TokenType.ValueSeparator:
                        break;
                    default:
                        throw new JsonException(token, currentState);
                }
                break;
            case ParserState.End:
                switch (token.Type)
                {
                    case TokenType.EndObject:
                    case TokenType.EndArray:
                    case TokenType.ValueSeparator:
                        var previous = structureStack.Pop();
                        var previousJsonObject = (previous as JsonObject);
                        var previousJsonArray = (previous as JsonArray);
                        if (previousJsonObject != null)
                        {
                            previousJsonObject.Add(keyStack.Pop(), current);
                            currentState = ParserState.Name;
                        }
                        if (previousJsonArray != null)
                        {
                            previousJsonArray.Add(current);
                            currentState = ParserState.Value;
                        }
                        if (token.Type != TokenType.ValueSeparator)
                        {
                            currentState = ParserState.End;
                        }
                        current = previous;
                        break;
                    default:
                        throw new JsonException(token, currentState);
                }
                break;
            default:
                break;
        }
    }
    return result;
}
+10  A: 

Without looking at it in detail, as you are parsing based on state, could you use the state pattern to break it up and parse each bit in a separate class based on the state?

something like this might be a start, although this is just pseudo code...

public interface IParserState
    {
    IParserState ParseToken (IToken token);
    }

public class BeginState : IParserState
    {
    private readonly Stack<JsonStructure> m_structureStack;
    private readonly Stack<String> m_keyStack;

    public BeginState (Stack<JsonStructure> structureStack, Stack<String> keyStack)
        {
        m_structureStack = structureStack;
        m_keyStack = keyStack;
        }

    public IParserState ParseToken(IToken token)
        {
        switch (token.Type)
            {
            case TokenType.OpenBrace:
                return new ObjectKeyParserState(m_structureStack,m_keyStack);
            case TokenType.OpenBracket:
                return new ArrayValueParserState(m_structureStack, m_keyStack);
            default:
                throw new JsonException (token);    
            }
        }
    }

public class ObjectKeyParserState : IParserState
    {
    private readonly Stack<JsonStructure> m_structureStack;
    private readonly Stack<String> m_keyStack;
    private readonly JsonObject m_current;

    public ObjectKeyParserState (Stack<JsonStructure> structureStack, Stack<String> keyStack)
        {
        m_current = new JsonObject();
        }

    public IParserState ParseToken (IToken token)
        {
        switch (token.Type)
            {
            case TokenType.StringLiteral:
                key = (string)token.Value;
                return new ColonSeperatorParserState(m_structureStack, m_keyStack, m_current,key);
            default:
                throw new JsonException(token);
            }
        }
Sam Holder
I prefer delegates in this case, instead of single-method interfaces.
Dykam
@Dykam: maybe but with interfaces and classes you can separate the code out into separate files which are logically different. and the OP may need other methods, I was only giving an example to show what I meant. I imagine that things may need to change in the actual implementation...
Sam Holder
I am still hoping to get other opinions but the state pattern is of course an elegant option.
ChaosPandion
In case of delegates you can also implement it the same way, but instead of the class you pass it's method. I advocate this way because it is the same but just more flexible. Though when you want 2 methods, interfaces are the only way.
Dykam
@ChaosPandion, it seems so.
Dykam
Thanks for taking the time to write out an answer.
ChaosPandion
+2  A: 

The 'conceptual design' in this case is production rules. If you were to design json yourself, would you think in terms of "A pair is a key followed by a colon followed by a value" or would you think in terms like "Colons will do 'a' in this case 'A' and do 'b' in case 'B' and do 'c' in case 'C'"? Look at http://www.json.org/. You'll see the 'conceptual design' stated in terms of production rules.

Since the 'structural design' of your code does not have the form of the 'conceptual design', no amount of refactoring will help. Changing the 'conceptual design' a small amount, would lead a code change that is hard to code and hard to test. You need a rewrite the code in terms of the 'conceptual design'.

// object
//   "{" "}"
//   "{" members "}" 
private static JsonObject ProduceJsonObject(Tokens tokens)
{
    var result = new JsonObject();

    tokens.Accept( TokenType.OpenBrace );
    result.members = ProduceJsonMembers(tokens);
    tokens.Accept( TokenType.CloseBrace );

    return result;
}

// members 
//   pair 
//   pair { "," pair }
private static JsonMembers ProduceJsonMembers(Tokens tokens)
{
    var result = new JsonMembers();

    result.Add( ProduceJsonPair(tokens) );
    while (tokens.LookAhead == TokenTag.Comma)
    {
       tokens.Accept( TokenType.Comma );
       result.Add( ProduceJsonPair(tokens) );
    }

    return result;
}

//pair 
//  string ":" value 
private static JsonPair ProduceJsonPair(Tokens tokens)
{
    var result = new JsonPair();

    result.String = tokens.Accept( TokenType.ID );
    tokens.Accept( TokenType.Colon );
    result.Value = ProduceJsonValue( tokens );

    return result;
}


// and so forth
jyoung
Please explain what *"The 'design' in this case is production rules."* means.
ChaosPandion
I wouldn't exactly say my code is lacking design. Its a finite state machine. It may not follow OO principles but if you follow through the states you'll find it is actually pretty good. I do like your idea though.
ChaosPandion
yeah, nice idea.
Sam Holder
I am sorry you you got the feeling that I thought you code had no or bad code design. Your code was very well written. I guess I am saying that structural design should follow conceptual design.
jyoung
I think we are on the same page now. I will definitely be going over the design again.
ChaosPandion