Fetch, Parse, and Save JSON

Question

I've written a program in Java that will fetch JSON information from a service through their API and save it. As I'm new to Java, I want my code looking clean as possible and to understand the language's cultural coding standards as soon as possible. I know JSON data is usually mapped to a POJO, but I still haven't been able to quiet figure out how to do that. Other than that, please feel free to be pedantic!

Main function

public class WeatherTracker {
 public static Boolean validData (JsonNode node) {
 //get() returns "null" if key does not exist 
 if (node.get("response").get("error") == null) { return true; }
 else { return false; }
 }
 public static void saveDataAsFile(JsonNode node, String dirPath, String fileName) throws JsonGenerationException, JsonMappingException, IOException {
 File dir = new File(dirPath);
 boolean success = dir.mkdirs() | dir.exists();
 if (success) {
 ObjectMapper objectMapper = new ObjectMapper();
 objectMapper.writeValue(new File(dirPath+fileName), node); 
 System.out.println("File created at: " + dirPath+fileName);
 } else {
 System.out.println("Could not make file at " + dirPath); //TODO Raise Exception if !success
 }
 }
 public static void historicalData(String apiKey, String city, String state, String date) throws MalformedURLException, IOException {
 // Do not attempt to get historical data unless all parameters have been passed
 if (city == null | state == null | date == null) {
 System.out.println("City, State, and Date must be provided when using historical look-up");
 System.exit(1);
 } else {
 JsonNode json = new WundergroundData(apiKey).fetchHistorical(city, state, date); //.path("history").path("dailysummary");
 if (validData(json)) {
 String sysFileSeperator = System.getProperty("file.separator");
 //Files will be saved in the format of ${cwd}/${city}/${date}.json
 String dirPath = String.format("%s%s%s%s", System.getProperty("user.dir"), sysFileSeperator, city, sysFileSeperator);
 String fileName = String.format("%s.json", date);
 saveDataAsFile(json.path("history"), dirPath, fileName);
 }
 else { 
 System.out.println(json.get("response").get("error"));
 }
 }
 }
 public static void main(String args[]) throws IOException, ParseException {
 String feature = null;
 String city = null;
 String state = null;
 String date = null;
 String apiKey = "*********074e7f5";
 //Initialize and set up CLI help options
 Options options = new Options();
 options.addOption("f", "feature", true , "Feature requested");
 options.addOption("c", "city" , true , "City requested");
 options.addOption("s", "state" , true , "");
 options.addOption("d", "date" , true , "Format as YYYMMDD. Date of look-up when doing a historical query");
 options.addOption("k", "key" , true , "Wunderground API Key");
 options.addOption("h", "help" , false, "Show help");
 //Initialize CLI Parsers/Formatters
 HelpFormatter formater = new HelpFormatter();
 CommandLineParser parser = new GnuParser();
 // Parse CLI input
 CommandLine cmd = parser.parse(options, args);
 // Set CLI input to variables
 if (cmd.hasOption("f")) { feature = cmd.getOptionValue("f"); }
 if (cmd.hasOption("c")) { city = cmd.getOptionValue("c"); }
 if (cmd.hasOption("d")) { date = cmd.getOptionValue("d"); }
 if (cmd.hasOption("s")) { state = cmd.getOptionValue("s"); }
 if (cmd.hasOption("k")) { apiKey = cmd.getOptionValue("k"); }
 // History Feature
 if (cmd.hasOption("f") && feature.equals("history")) { // Check hasOption to avoid Null Pointer Exception
 historicalData(apiKey, city, state, date);
 } else if (cmd.hasOption("h")) {
 formater.printHelp("Query Wunderground for weather data", options);
 }
 }
}

Wunderground Class/API Interface

public class WundergroundData {
 private static final String PROTOCOL = "Http";
 private static final String WU_HOST = "api.wunderground.com";
 private String apiKey; // Wunderground requires a registered key to use services
 public void setApiKey(String apiKey) {
 this.apiKey = apiKey;
 }
 public String getApiKey() {
 return apiKey;
 }
 public URL createUrl(String feature) throws MalformedURLException {
 String relativePath = new String(String.format("/api/%s/%s", apiKey, feature));
 URL url = new URL(PROTOCOL, WU_HOST, relativePath);
 return url;
 }
 public JsonNode fetchHistorical(String city, String state, String date)
 throws MalformedURLException, IOException {
 URL url = createUrl(String.format("history_%s/q/%s/%s.json", date, state, city));
 return JsonReader.readJsonFromUrl(url);
 }
 public WundergroundData() {
 }
 public WundergroundData(String key) {
 setApiKey(key);
 }
}

JSON Reader Class

public class JsonReader {
 public static JsonNode readJsonFromFile(File file)
 throws JsonProcessingException, IOException {
 ObjectMapper mapper = new ObjectMapper();
 JsonNode root = mapper.readTree(file);
 return root;
 }
 public static JsonNode readJsonFromUrl(String url)
 throws MalformedURLException, IOException {
 InputStream inputStream = new URL(url).openStream();
 ObjectMapper mapper = new ObjectMapper();
 try {
 JsonNode root = mapper.readTree(inputStream);
 return root;
 } finally {
 inputStream.close();
 }
 }
 public static JsonNode readJsonFromUrl(URL url)
 throws MalformedURLException, IOException {
 InputStream inputStream = url.openStream();
 ObjectMapper mapper = new ObjectMapper();
 try {
 JsonNode root = mapper.readTree(inputStream);
 return root;
 } finally {
 inputStream.close();
 }
 }
}

• Phrancis: Welcome to Code Review! That's an interesting question, I'm sure our Java experts will have some useful advice for you!

Answer 1

Use boolean instead of Boolean when possible

Boolean can have 3 values: true, false, null.

If you don't need the null value (typical), then use boolean.

Use || instead of | for boolean conditions

What's the difference between these two conditions?

if (cond1 || cond2) { ... }
if (cond1 | cond2) { ... }

The first one uses the boolean OR operator, which short-circuits in Java. Short-circuiting means that if the first condition is true, the second won't be evaluated, because it's unnecessary.

The second uses the bitwise OR operator, which doesn't short-circuit. So even when cond1 is true, cond2 will also be evaluated, pointlessly.

Everywhere you used the | operator is a misuse, not the intended purpose of the bitwise OR operator. Replace everywhere with ||, the boolean OR operator.

File paths in Java

Many people don't seem to know, but you can use / as path separator in Java in many places, for example in path strings when creating File objects. In Windows it will be correctly replaced with \.

So instead of this:

String sysFileSeperator = System.getProperty("file.separator");
//Files will be saved in the format of ${cwd}/${city}/${date}.json
String dirPath = String.format("%s%s%s%s", System.getProperty("user.dir"), sysFileSeperator, city,

sysFileSeperator); String fileName = String.format("%s.json", date);

saveDataAsFile(json.path("history"), dirPath, fileName);

You could write this and the code will still work:

//Files will be saved in the format of ${cwd}/${city}/${date}.json
String dirPath = String.format("%s/%s/", System.getProperty("user.dir"), city);
String fileName = String.format("%s.json", date);
saveDataAsFile(json.path("history"), dirPath, fileName);

Use the 2-param constructor of File for dir + file

Still staying with the previous example, the trailing path separator / is dirty:

String dirPath = String.format("%s/%s/", System.getProperty("user.dir"), city);

I know why you did it this way: saveDataAsFile will form a path by concatenating dirPath and fileName. If you omit the trailing / here, the program will stop working. Notice that this is a hidden rule when calling saveDataAsFile, it's not obvious, and callers of saveDataAsFile might forget to preformat the dirPath correctly.

There's a better, clean way: in saveDataAsFile, instead of forming a path by dirPath and fileName strings, use the 2-parameter version of new File, like this:

File file = new File(dirPath, fileName);
objectMapper.writeValue(file, node); 
System.out.println("File created at: " + file);

Avoiding NPE

This code:

 if (cmd.hasOption("f") && feature.equals("history")) { // Check hasOption to avoid Null Pointer Exception

First of all, note that you have to scroll to right to see the comment. It's not a good practice to put long comments at line ends. Put it on the line before.

You added the comment because feature might be null. Unfortunately my IDE still warns me of a potential null, and warnings should not be ignored. In fact, a better way of writing the same thing:

 if (feature != null && feature.equals("history")) {

In this version there is no warning, and it's so clear you don't need a comment either. (Code that needs a comment is usually smelly.)

Even better, as @Marc-Andre suggested in a comment:

 if ("history".equals(feature)) {

In fact it is the recommended practice to put string constants on the left-hand side of .equals(...), to prevent accidental NPE.

Unusual help option

Traditionally, when the -h or --help flag is specified, it overrides everything else, the command prints the help no matter what other flags were present. Usually. In this program that's not the case: for -f history -h the program will print the historical data instead of help.

It's a small thing, but I'd stick to traditional approaches: check first if help was requested, and if yes, print the help and exit early.

• Marc-Andre: Nice answer! If you really want to avoid a NPE and check if a String is equals you can use "history".equals(feature). I tend to like this, since it remove one evaluation and still provide safety.
• janos: Thanks @Marc-Andre, that's an excellent suggestion, and it's even the recommended practice to write that way (Sonar warns, for example). I updated my answer, thanks!

Answer 2

Returning boolean

public static Boolean validData (JsonNode node) {
 //get() returns "null" if key does not exist 
 if (node.get("response").get("error") == null) { return true; }
 else { return false; }
}

You're already evaluating a boolean in the if condition, you could simply use this :

public static Boolean validData(JsonNode node) {
 //get() returns "null" if key does not exist 
 return node.get("response").get("error") == null;
}

Styling

This is king of a personnal point, but the Java normal style guide suggest this :

 if (cmd.hasOption("f")) {
 feature = cmd.getOptionValue("f");
 }
 if (cmd.hasOption("c")) {
 city = cmd.getOptionValue("c");
 }
 if (cmd.hasOption("d")) {
 date = cmd.getOptionValue("d");
 }
 if (cmd.hasOption("s")) {
 state = cmd.getOptionValue("s");
 }
 if (cmd.hasOption("k")) {
 apiKey = cmd.getOptionValue("k");
 }

instead of this :

 if (cmd.hasOption("f")) { feature = cmd.getOptionValue("f"); }
 if (cmd.hasOption("c")) { city = cmd.getOptionValue("c"); }
 if (cmd.hasOption("d")) { date = cmd.getOptionValue("d"); }
 if (cmd.hasOption("s")) { state = cmd.getOptionValue("s"); }
 if (cmd.hasOption("k")) { apiKey = cmd.getOptionValue("k"); }

If you worry about vertical space in your main method, just extract that piece of code as a function and be done with it. It really easier to read it with the normal style guide.

System.exit(1)

 if (city == null | state == null | date == null) {
 System.out.println("City, State, and Date must be provided when using historical look-up");
 System.exit(1);
 }

Normally, I would go something like this : No, using System.exit(1) is bad... But in this is case I guess it's ok. The thing I would prefer to do is, throwing an Exception. Why ? Because well, when you'll re factored your code or upgrade it to more than just a simple command line program, well System.exit(1) is a rather drastic thing to do. Instead, if you do something like :

 if (city == null | state == null | date == null) {
 System.out.println("City, State, and Date must be provided when using historical look-up");
 throw InvalidArgumentException("City, State, and Date must be provided when using historical look-up");
 }

You can just let the exception propagate itself to the main or you can catch it in the main, and exit cleanly with System.exit(1) (since you're still a command line). The thing is histocialData should not have the responsability to terminate the program, only let the caller know that it cannot do it's job since it doesn't have the required arguments.

Constant

In your code, you use some specific labels for Json fields like response, error and history. You repeat them a couple of times in your code. I would make them a constant like private static final String RESPONSE = "response";.

Java code style

I've already mentioned it earlier, but you do not follow exactly the Java code style, I would suggest you look at the one from Oracle or the one from Google. It's easy to setup a rule in your preferred IDE to auto-format your code.

Mkdirs and exits

In your comment you've explained what you used this following line of code :

boolean success = dir.mkdirs() | dir.exists();

What you want to do (and you've confirmed me that in the comments) is to check if the folders are created, and if not created the directory. Then let's do this! We do not need "clever" tricks when you sacrifice readability for it. Code should be readable, and should almost read like a book.

public static void saveDataAsFile(JsonNode node, String dirPath, String fileName) throws JsonGenerationException, JsonMappingException, IOException {
 File dir = new File(dirPath);
 if(!dir.exists()) {
 boolean success = dir.mkdirs();
 if(!success) {
 System.out.println("Could not make file at " + dirPath); //TODO Change this to proper logging
 throw new IOException("Could not make file at " + dirPath);
 }
 }
 ObjectMapper objectMapper = new ObjectMapper();
 objectMapper.writeValue(new File(dirPath, fileName), node);
 System.out.println("File created at: " + dirPath + fileName);
}

Do you see that the code now is very explicit about what is going on ? Yes I needed to use more lines of code, but now I'm pretty sure that the intention of the code is clear. I even use a boolean variable, since I don't like when something important (like the creation of a directory) happens in the condition of a if.

• Jon.H: Thanks your suggestions! About the dirs.exists() check: it's because for most iterations of this program, the directory will already be there, however, I want the following code block to execute so long as there is a directory to write into. If it has to be made first with mkdirs(), do that, then execute, but since mkdirs doesn't return true for instances when the directory already exists (the normal case) I had check for that as well. Don't hesitate to let me know if there is a better way to do this :)
• Marc-Andre: In fact, I think what you're testing is if the folder exists, if not create the directory. I edit my answer with what I would do when I'll be back home (I'm at work at the moment).
• Jon.H: You are correct, that's the behaviour I'm going for
• Marc-Andre: @Jon.H Sorry if it was long but I edited the answer about how I would have write the directory check. If you have any comments about just say so! Hope it will help you!
• Marc-Andre: If you don't want to open a new question, we could start a chat room and answer your question. Do know that a new question you be a good option. It would not be an unnecessary thread since we can review the evolution of your code, and someone could still suggest some new things with your code.

Answer 3

@Marc-Andre and @janos answers are very informative, consider mine to be an expansion to theirs... :)

WeatherTracker

HelpFormatter formater = new HelpFormatter();

Minor typo: this should be formatter.

If you happen to be on Java 8, you may want to consider this alternative approach for retrieving a command-line option and setting your class fields: the use of Optional. Since CommandLine.getOptionValue(String) returns null when the option is not specified, we can combine it with the following two methods from Optional to drive these configuration settings for us:

• (static) ofNullable(T value)

  Returns an Optional describing a non-null value, else you get an empty Optional.

• ifPresent(Consumer<? super T> consumer)

  For the Optional object, if there is a non-null value, pass it to the Consumer to operate on.

Assuming if you have setter methods in place, what looks like this:

if (cmd.hasOption("f")) { feature = cmd.getOptionValue("f"); }

Can be coded like this too:

Optional.ofNullable(cmd.getOptionValue("f")).ifPresent(this::setFeature);

This means that what are 'hard-coded' assignments using if statements now can be represented as mappings between the command-line option and the relevant setter method, as a method reference. So, in the future, you can build a Map<String, Consumer<String>> (options) that has the following key-value pairs:

"f" ==> this::setFeature
"c" ==> this::setCity
"s" ==> this::setState
...

And you can iterate through the map to set them as such:

options.forEach((k, v) -> { Optional.ofNullable(cmd.getOptionValue(k)).ifPresent(v); });

WundergroundData

Just two things here:

• You don't need a new String() below:

  String relativePath = new String(String.format("/api/%s/%s", apiKey, feature));
  

• Inline more, inline often

  I feel there are some places where you can neatly inline methods, reducing the number of variable assignments. For example, I may choose to write createUrl() in the following way:

  public URL createUrl(String feature) throws MalformedURLException {
   return new URL(PROTOCOL, WU_HOST, String.format("/api/%s/%s", apiKey, feature));
  }
  

JsonReader

One issue here is the code repetition you have. Now, I'm not too familiar with the Jackson library to know if you need a new ObjectMapper() every time, and neither do I know which version you are using (it looks like there are already methods to read from either URL, File or InputStream as far back as 2.2.0), but just focusing on the code you have, you should consider calling readJsonFromUrl(URL) from the String input equivalent as such:

public static JsonNode readJsonFromUrl(String url)
 throws MalformedURLException, IOException {
 return readJsonFromUrl(new URL(url));
}

And if you happen to be on Java 7, consider using try-with-resources:

public static JsonNode readJsonFromUrl(URL url) throws IOException {
 try (InputStream inputStream = url.openStream()) {
 return new ObjectMapper().readTree(inputStream);
 }
}

• Marc-Andre: I wanted to talk about the try-with-resources, but I find that I've covered a lot of things in my answer. Nice answer!
• Jon.H: Thank you for your reply! I wanted to ask more about your suggestions in the JsonReader class. Your comment on using try-with-resources is great and I will be implementing it. On the subject of the ObjectMapper class and the methods provided, however, you are right that readTree() can already read from URL, File, and InputStream indeed, but since I do not know which type is going to be used a head of time, isn't the only solution Function Overriding?
• h.j.k.: @Jon.H ObjectMapper is already providing those overridden methods for you, so unless there are other reasons for JsonReader to exist (e.g. it's safe to use just one instance of ObjectMapper), you should consider substituting your current calls to JsonReader with the same method from ObjectMapper, e.g. return new ObjectMapper().readTree(url); instead of return JsonReader.readJsonFromUrl(url);
• Jon.H: @h.j.k. Oh my god, of course! I can't believe I didn't see that! Thank you for your post!