Filtering JSONArray by Branding & Modelling

Question

I have a small function that takes in a list of JSON objects that has the format below. The function below that wants to see if the brand and model of car passed to it can be matched against the List parameter called jsonfiltersList. The code I have works, but being somewhat new to coding and Java I wanted to know if I can optimize the code snippet to make it more compact somehow. Any thoughts?

{ auto_filters : [ { "brand" : "Toyota" }, { "brand" : "Honda", "models" : [ "Civic", "Accord" ] } ] } 

And the code:

 public static boolean filterMatch(String brand, String model, ArrayList<JSONObject> jsonFiltersList) { boolean brandMatch = false; boolean modelMatch = false; if ((jsonFiltersList == null) || (jsonFiltersList.size() < 1)) return true; try { for (JSONObject j : jsonFiltersList) { if (j.get("brand").equals(brand)) { brandMatch = true; } if (brandMatch) { JSONArray jArr = null; if (j.has("models")) { jArr = (JSONArray) j.get("models"); } ArrayList<String> modelsArr = new ArrayList<String>(); if (jArr != null) { for (int i = 0; i < jArr.length(); i++) modelsArr.add(jArr.get(i).toString()); if (modelsArr.contains(model)) { modelMatch = true; break; } } else { modelMatch = true; break; } } brandMatch = false; } } catch(Exception e) { e.printStackTrace(); } return brandMatch && modelMatch; } 

Answer 1

As it happens I have been working with JSON formatted data a little bit recently. I have some ideas that will help you simplify your code.

As for the efficiency, performance wise it looks good. I don't think it can go much faster. In terms of code structure though, there's a few things that can help.

I'll put together a laundry-list of items, and then follow it up with your code restructured...

• make your parameters final when you can. It helps with discipline, and occasionally performance too.
• Use interfaces instead of concrete classes if you can. The input Parameter ArrayList<JSONObject> should be reduced to an interface, but, not just List, and not even Collection, but Iterable. You should have Iterable<JSONObject>.... which means pretty much anything that has an iterator() method.
• jsonFiltersList.size() < 1 is better expressed as jsonFiltersList.isEmpty()

• Don't use parenthesis unless they're needed:

  if ((jsonFiltersList == null) || (jsonFiltersList.size() < 1)) 

  should be:

  if (jsonFiltersList == null || jsonFiltersList.isEmpty()) 

• 1-liners should be expressed with a full braced block:

  if ((jsonFiltersList == null) || (jsonFiltersList.size() < 1)) return true; 

  should be

  if (jsonFiltersList == null || jsonFiltersList.isEmpty()) { return true; } 

• The check above shows that you know how to return early (you don't need to wait for the end to return... so, use that to your advantage. Return when you have a result.

• You have redundant brandMatch checks:

  if (j.get("brand").equals(brand)) { brandMatch = true; } if (brandMatch) { ..... 

  can simply be:

  if (j.get("brand").equals(brand)) { ..... 

• There is no need to populate the full models array... you can just return if you find a match...

• Don't catch Exception, catch JSONException because that's what's thrown.

Putting this all together, your code ends up like:

public static boolean filterMatch(final String brand, final String model, final Iterable<JSONObject> jsonFiltersList) { if (jsonFiltersList == null) { return true; } try { for (JSONObject j : jsonFiltersList) { if (j.get("brand").equals(brand)) { if (!j.has("models")) { return true; } JSONArray jArr = (JSONArray) j.get("models"); if (jArr == null) { return true; } for (int i = 0; i < jArr.length(); i++) { if (model.equals(jArr.get(i).toString())) { return true; } } } } } catch (Exception e) { e.printStackTrace(); } return false; }