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.

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.

@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);
    }
}