Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am working on a way to quickly build JSON strings in Java. To this end, I have created the following two files. Assuming that I do not need to parse JSON strings for Java, is this an efficient implementation? Furthermore, are there any cases where it would output an invalid JSON string?

Thank you in advance.


JSONList.java:

public class JSONList
{
    private ArrayList<JSON> items = null;

    public JSONList()
    {
        this.items = new ArrayList<JSON>();
    }
    public JSONList add(JSON value)
    {
        if(value != null)
        {
            this.items.add(value);
        }
        else
        {
            throw new IllegalArgumentException("Value must be JSON, was " + (value == null ? "null" : value.getClass()));
        }
        return this;
    }

    @Override
    public String toString()
    {
        StringBuilder sb = new StringBuilder();
        boolean first = true;
        for(JSON j : this.items)
        {
            if(first)
            {
                first = false;
            }
            else
            {
                sb.append(",");
            }
            sb.append(j.toString());
        }

        return "[" + sb.toString() + "]";
    }

JSON.java:

public class JSON
{

    private HashMap<String, Object> parts = null;

    public JSON()
    {
        this.parts = new HashMap<String, Object>();
    }

    public JSON add(String key, Object value)
    {
        if(value != null && (value instanceof String || value instanceof Boolean || value instanceof Integer || value instanceof Float || value instanceof JSONList))
        {
            this.parts.put(key, value);
        }
        else if(value == null)
        {
            this.parts.put(key, "null");
        }
        else
        {
            throw new IllegalArgumentException("Value must be either String, Boolean, Integer or Float, was " + (value == null ? "null" : value.getClass()));
        }
        return this;
    }

    @Override
    public String toString()
    {
        StringBuilder sb = new StringBuilder();
        boolean first = true;
        for(Entry<String, Object> entry : this.parts.entrySet())
        {
            Object v = entry.getValue();
            if(first)
            {
                first = false;
            }
            else
            {
                sb.append(",");
            }
            if(v instanceof String)
            {
                sb.append("\"").append(entry.getKey()).append("\":\"").append(v).append("\"");
            }
            else if(v instanceof Boolean || v instanceof Integer || v instanceof Float)
            {
                sb.append("\"").append(entry.getKey()).append("\":").append(v);
            }
            else if(v instanceof JSONList)
            {
                sb.append("\"").append(entry.getKey()).append("\":").append(((JSONList) v).toString());
            }
        }

        return "{" + sb.toString() + "}";
    }
}
share|improve this question
1  
Is there a reason you are not using an existing library to do this? For instance Gson. –  Jeff Vanzella Feb 6 '13 at 18:02
    
@JeffVanzella - Partially to learn how to do it on my own, partially for size (my two files are around 3K in total as opposed to GSON's 185K). –  ChrisForrence Feb 6 '13 at 18:10
    
Do you want to support all JSON? If so, how would you produce a valid JSON string such as ["GML", "XML"] with this code? –  Cyrille Ka Feb 6 '13 at 19:41

2 Answers 2

up vote 6 down vote accepted

First of all, this.items is redundant in most cases - just items would be enough.


if(value != null)
{
    this.items.add(value);
}
else
{
    throw new IllegalArgumentException("Value must be JSON, was " + (value == null ? "null" : value.getClass()));
}
return this;

This format is not what I'd expect. Usually, I'd use a guard clause.

if(value == null) {
    throw new IllegalArgumentException("Value must be JSON, was " + (value == null ? "null" : value.getClass()));
}

items.add(value);
return this;

Java coding conventions show that there should be a space between an if and the parenthesis:

if(value == null) //Doesn't follow the convention
if (value == null) //Follows the convention

return "[" + sb.toString() + "]";

If you are using a StringBuilder why don't you directly append '[' and ']' to the builder? Same in the other class.


If I understand correctly, JSONList is a JSON array and JSON is a JSON object, is this correct? Have you considered renaming them to JSONArray and JSONObject? Also, if you reuse the String, Boolean, integer, etc. classes, why don't you also reuse Java's List?

Your arrays also seem overly restrictive. You allow arrays of objects but not anything else. How would I output ["Hello", "World"] with your classes?

And what if I want an object inside another object? {"foo":{}}


this.parts.put(key, "null");

This is problematic. You later use Object v = entry.getValue(); and check for the type of v, which will be reported as String. This is incorrect. JSON supports null values, but your class will replace them by "null".


sb.append("\"").append(entry.getKey())

You repeat this a lot. Consider moving this to before the type checks.


As a precaution, after the last else if(v instanceof JSONList), consider adding:

else {
    assert false : "This should never happen";
}

as defensive programming.


If I pass a NaN float to your JSON class, it'll generate a JSON like {foo: NaN} which is invalid. NaN is not supported in JSON.

Some Strings will be incorrectly output to the JSON file. Consider Strings using the \ or " character, for instance. And also "control characters". You should properly escape them. Same applies to object keys.

For more information about the JSON format, see http://www.json.org/.


In JSON.add, you never check if the key is null.


You offer add methods but not remove. Is this intentional?


You support Integer and Float, but not Byte, Short, Long and Double. This might cause some unpleasant surprises to users of your application.


.append(((JSONList) v).toString()) is redundant. .append(v) would work (see documentation for StringBuilder and valueOf.

share|improve this answer
1  
Thank you for your feedback luiscubal! I'm still learning JSON formatting, and I've taken many of your ideas into consideration (and fixed some of the immediate problems you've highlighted). –  ChrisForrence Feb 6 '13 at 20:19
  1. The = null is unnecessary here, since it's the default:

    private HashMap<String, Object> parts = null;
    
  2. HashMap<...> reference types should be simply Map<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

    private Map<String, Object> parts;
    
  3. "null" is never used in the following snippet, since value can't be null in the last else block:

    ...
    else if(value == null) {
        this.parts.put(key, "null");
    } else {
        throw new IllegalArgumentException("Value must be either String, Boolean" +
            ", Integer or Float, was " + (value == null ? "null" : value.getClass()));
    }
    
  4. A reference for Jeff's comment: Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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