Problem
This is part 2 to Fetch, Parse, and Save JSON. As the code evolved, I wanted to post the program with changes for review again.
The objective of the program is to fetch JSON information from a service through their API and save it. Currently, I dislike the way the program handles bad user input in the WeatherTracker class by having two similar checks with the same error message back-to-back. I’m also beginning to feel that the job of saving the data is not necessarily the responsibility of the fetchHistoricalData function. I’m thinking that fetchHistoricalData ought to return a JSON on success, and null on failure, and another function should save the data instead. Please let me know what you think.
Lastly, in the near future there will be a script initiated by a cron job that will call this program with changing parameters at set intervals of time. I need a way for this program to exit on failure in such a way that a top level script can catch it and stop running. The only way I know how to do this now is with Sys.exit(#), but is this the best way?
Please feel free to be pendantic/critical. I would like to be able to submit this as an example of side projects I’ve done to future employers with confidence 😀
Main Class
public class WeatherTracker {
private static final String RESPONSE = "response";
private static final String HISTORY = "history";
private static final String ERROR = "error";
private static final String INVALID_OPTION = "Invalid option. Please use option -h or "
+ "--help a list of available commands";
private static final String USAGE_MSG = "WeatherTracker -k [api_key] -f [feature] [-options]n"
+ "Query Wunderground for weather data.n The 'k' option must "
+ "be used for all feature requests";
public static Boolean validData (JsonNode node) {
return node.get(RESPONSE).get(ERROR) == null;
}
public static void saveDataAsFile(JsonNode node, String dirPath, String fileName)
throws JsonGenerationException, JsonMappingException, IOException {
File dir = new File(dirPath);
if (!dir.exists()) {
if (!dir.mkdirs()) {
throw new IOException("Could not make file at " + dirPath);
}
}
File file = new File(dirPath, fileName);
new ObjectMapper().writeValue(file, node);
System.out.println("File created at: " + file);
}
public static boolean fetchHistoricalData(String apiKey, String city, String state, String date, String savePath)
throws MalformedURLException, IOException {
// Do not attempt to get historical data unless all parameters have been passed
if (city == null || state == null || date == null) {
throw new IllegalArgumentException("City, State, and Date must be provided when requesting historical data");
}
else {
JsonNode json = new WundergroundData(apiKey).fetchHistorical(city, state, date);
if (validData(json)) {
//Files and full path will be saved in the format of ${savePath}/${city}/${date}.json
String dirPath = String.format("%s/%s", savePath, city);
String fileName = String.format("%s.json", date);
saveDataAsFile(json.path(HISTORY), dirPath, fileName);
return true;
}
else {
System.out.println(json.get(RESPONSE).get(ERROR));
return false;
}
}
}
public static void main(String args[]) throws IOException, ParseException {
String feature = null;
String city = null;
String state = null;
String date = null;
String apiKey = null;
String savePath = System.getProperty("user.dir");
//Initialize and set up CLI help options
Options options = new Options();
options.addOption("f", "feature", true , "Feature requested");
options.addOption("p", "path" , true , "Location to save file (defaults to current working directory)");
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
CommandLineParser parser = new BasicParser();
// 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("p")) {
savePath = cmd.getOptionValue("p") ;
}
if (cmd.hasOption("c")) {
city = cmd.getOptionValue("c");
}
if (cmd.hasOption("s")) {
state = cmd.getOptionValue("s");
}
if (cmd.hasOption("d")) {
date = cmd.getOptionValue("d");
}
if (cmd.hasOption("k")) {
apiKey = cmd.getOptionValue("k");
}
// Main entry point
if (cmd.hasOption("h") || args.length == 0) {
new HelpFormatter().printHelp(USAGE_MSG, options);
}
else if (cmd.getOptionValue("k") != null) {
if ("history".equals(feature)) {
fetchHistoricalData(apiKey, city, state, date, savePath);
}
else {
System.out.println(INVALID_OPTION);
}
}
else {
System.out.println(INVALID_OPTION);
}
}
}
API Interface Class
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 featureUrl(String feature) throws MalformedURLException {
// Standard Request URL Format: ${protocol}${WU_HOST}/api/${key}/${features}/${settings}/q/${query}
return new URL(PROTOCOL, WU_HOST, String.format("/api/%s/%s", apiKey, feature));
}
public JsonNode fetchHistorical(String city, String state, String date)
throws MalformedURLException, IOException {
return new ObjectMapper().readTree(featureUrl(String.format("history_%s/q/%s/%s.json"
, date, state, city)));
}
public WundergroundData() {
}
public WundergroundData(String key) {
setApiKey(key);
}
}
Solution
Just one suggestion for starters…
nesting-else
When a method is strictly doing only one thing or another:
private T method(Object... args) {
if (condition) {
// do something for true
} else {
// do something for false
}
}
You don’t need the else and the extra level of indentation. I will actually recommend this especially for cases where the code block can get a bit long. Therefore, you can can improve your main method as such:
public static boolean fetchHistoricalData(String... )
throws MalformedURLException, IOException {
if (city == null || state == null || date == null) {
throw new IllegalArgumentException(...);
}
// no need for else here and the extra indentation
JsonNode json = new WundergroundData(apiKey).fetchHistorical(city, state, date);
...
}
This also applies for all if-blocks that are returning from either branches…
JsonNode json = new WundergroundData(apiKey).fetchHistorical(city, state, date);
if (!validData(json)) {
System.out.println(json.get(RESPONSE).get(ERROR));
return false;
}
String dirPath = String.format("%s/%s", savePath, city);
...
return true;
In this case, I flipped the clauses around since the code for handling invalid JSON data is a little shorter.
And finally for your main() method:
if (cmd.hasOption("h") || args.length == 0) {
new HelpFormatter().printHelp(USAGE_MSG, options);
} else if (cmd.getOptionValue("k") != null && "history".equals(feature)) {
fetchHistoricalData(apiKey, city, state, date, savePath);
} else {
// final catch-all
System.out.println(INVALID_OPTION);
}