Simple JSON writer

Question

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() + "}";
    }
}

Answer 1

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.

Answer 2

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.)