Fetch, Parse, and Save JSON

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");

//F

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.

StackExchange Code Review Q#88592, answer score: 5