Parsing and summarizing an aviation METAR (US) report

Question

I have a list of objects, which contains field rawData (eg rawData="KLAX 141353Z 06003KT 6SM BR FEW002 FEW008 BKN280 12/12 A3007 RMK AO2 SLP182 T01170117 $"). In my case rawData is METAR report from which i want to extract the most severe weather condition (clouds, fog, rain, snow). Currently I am applying parseWeather to each object of my list.

METAR string can contain multiple instance of same tag e.g. (BRN065 BRN033), it even can contain all of them in same time . To my output - I want to display "simplified" worst weather condition, where the worst, for the current task, is considered as fog, then snow, then other weather conditions.

To answer why i return same type as argument - initial argument have some pre parsed detailed report from FlightAware service. However I have different requirenment for simplified report, than provided. Thus function parseWeather is a side effect function which fills extra fields.

 private PollFlightResponse.ResultEntity.MetarEntity parseWeather( PollFlightResponse.ResultEntity.MetarEntity d) { int severity = 0; //how bad is weather from 0 to 2 String wCode = ""; // map key String weather = ""; // human readable weather if (d.getRawData().contains("FG")) { wCode = "FG"; } else if (d.getRawData().contains("+SN")) { wCode = "+SN"; } else if (d.getRawData().contains("SN")) { wCode = "SN"; } else if (d.getRawData().contains("-SN")) { wCode = "-SN"; } else if (d.getRawData().contains("+RA")) { wCode = "+RA"; } else if (d.getRawData().contains("-RA")) { wCode = "-RA"; } else if (d.getRawData().contains("RA")) { wCode = "RA"; } else if (d.getRawData().contains("VV")) { wCode = "VV"; } else if (d.getRawData().contains("OVC")) { wCode = "OVC"; } else if (d.getRawData().contains("BKN")) { wCode = "BKN"; } else if (d.getRawData().contains("SCT")) { wCode = "SCT"; } else if (d.getRawData().contains("FEW")) { wCode = "FEW"; } else if (d.getRawData().contains("NSC")) { wCode = "NSC"; } else if (d.getRawData().contains("CLR")) { wCode = "CLR"; } if (TextUtils.isEmpty(wCode)) { wCode = "CLR"; } weather = SearchActivity.conditions.get(wCode); severity = SearchActivity.severity.get(wCode); if (d.getRawData().contains("MPS")) { int index = d.getRawData().indexOf("MPS"); String wind = d.getRawData().substring(index - 2, index); int s = Integer.parseInt(wind); if (s == 0) { //no wind does not affect results } else if (s < 3) { weather += ", Light wind"; } else if (s < 10) { weather += ", Windy"; if (severity < 1) severity = 1; } else { weather += ", Strong wind"; if (severity < 1) severity = 2; } } d.setGeneraWeather(weather); d.setWeatherSeverity(severity); return d; }

where conditions and severity is static public hashmaps

 private void generateWeatherMap() { conditions.clear(); severity.clear(); //Strings conditions.put("FG", "Fog"); conditions.put("+SN", "Heavy snow"); conditions.put("SN", "Snow"); conditions.put("-SN", "Light snow"); conditions.put("+RA", "Heavy rain"); conditions.put("RA", "Rain"); conditions.put("-RA", "Light rain"); conditions.put("CLR", "Clear sky"); conditions.put("NSC", "Little clouds"); conditions.put("FEW", "Few clouds"); conditions.put("SCT", "Scattered clouds"); conditions.put("BKN", "Broken clouds"); conditions.put("OVC", "Overcast"); conditions.put("VV", "Heavy precipitation"); //Values severity.put("FG", 2); severity.put("+SN", 2); severity.put("SN", 1); severity.put("-SN", 1); severity.put("+RA", 1); severity.put("RA", 1); severity.put("-RA", 1); severity.put("CLR", 0); severity.put("NSC", 0); severity.put("FEW", 0); severity.put("SCT", 1); severity.put("BKN", 1); severity.put("OVC", 2); severity.put("VV", 2); }

Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. — Peilonrayz
– Peilonrayz ♦, Commented Dec 15, 2016 at 10:26
You should write unit tests until you reach 100% code coverage. Doing that you will notice that your code is buggy since it never reports "SN-". — Roland Illig
– Roland Illig, Commented Dec 15, 2016 at 23:04

Peter Taylor · Accepted Answer · 2016-12-16 11:52:22Z

If you find yourself calling the same immutable method dozens of times, you should think about storing the result in a local variable. I count 17 calls to d.getRawData(), which I assume all give the same result. (If not, you have other problems!)

You're using an object-oriented language, so take advantage of its features. An existing answer has pointed out that the if-else chain could be simplified to a loop, but it doesn't go far enough. If you define a class

class WeatherCondition { WeatherCondition(string code, string condition, int severity) { ... } ... }

then you can replace the conditions and severity maps with a single List<WeatherCondition> in priority order and replace the first 41 lines of the first code block with

 private PollFlightResponse.ResultEntity.MetarEntity parseWeather( PollFlightResponse.ResultEntity.MetarEntity d) { // Fallback value WeatherCondition weather = new WeatherCondition("CLR", "Clear sky", 0); String rawData = d.getRawData(); for (WeatherCondition condition : conditions) { if (condition.matches(rawData)) { weather = condition; break; } }

(I'm not up to date on streams: there may be more idiomatic ways to do this in Java 8).

Note that this relies on a method WeatherCondition.matches(String). Your code effectively implements this as

rawData.contains(code)

but that is buggy: it matches SN in any report containing -SN or +SN. An improved hack would be

rawData.contains(" " + code)

but since this is supposed to be a parser it should really use the grammar and not just random substrings which could appear in other contexts. (That's another bug with the original code: FG is a substring of airport codes in Equatorial Guinea, among others, so it would claim that it's always foggy there).

Here's one thing I don't understand because there isn't enough context given: why on Earth is the return type of parseWeather the same as the argument type? If you have the parsed instance already, can't you just return it? If not, shouldn't the argument just be the String?

And one final note: based on my quick background reading to answer this, I think that this method is specific to US METAR rather than the international standard. If that's the case, it should be clearly documented.

Thank you for extended review. I answered aboud method return type in question body — Yarh
– Yarh, Commented Dec 19, 2016 at 12:10

Mario Santini · Accepted Answer · 2016-12-15 12:15:13Z

A question before

It is not clear to me how yor code works, but could as I'm not an expert of this METAR format.

The weather you find is just the last you get from the input string.

If you have, for example, an Heavy show you will miss it as you could even have a Overcast.

This part is much a question, than an answer, sorry.

Assuming your code is correct and the input string contains only one weather condition, here are my considerations.

Advise 1

You should use a for loop with the keys of SearchActivity.conditions, and break as soon as you find the conditions, avoiding matching all others.

So for example:

String weather = SearchActivity.conditions.get("CLR"); for (String condition : SearchActivity.conditions.keySet()) { if (d.getRawData().contains(condition)) { weather = SearchActivity.conditions.get(condition); break; } }

Of course if you really need the last match, than you could not use this loop, as the Set don't preserve the order of the keys.

In this case, if you still want to have a loop, you should create an array with the conditions keys.

Advise 2

You should use constant for the condition labels:

final String CLR = "CLR"; final String FOG = "Fog"; ... so on...

And use those constants in your code.

EDIT

For example in your if else:

if (d.getRawData().contains(FOG)) { wCode = FOG; } else if (... } else { wCode = CLR; }

A review on the code

Your if else statement is not correct, as the last check could be just an else:

.... } else if (d.getRawData().contains("NSC")) { wCode = "NSC"; } else { wCode = "CLR"; }

As after you check if the wCode variable is empty and the you force CLR, that is the last condition.

if (TextUtils.isEmpty(wCode)) { wCode = "CLR"; }

Sorry for missleading, I added Edit to my post to describe more expected input and output. About advise 2, do you mean to use those variables to remove wCode variable, e.g. else if (d.getRawData().contains("SN")) { weather = Weather.SN;} , where SN is a final String with value snow? Thanks for pointing to reduntunt TextUtils.isEmpty(wCode) check — Yarh
– Yarh, Commented Dec 15, 2016 at 10:44
@Yarh I updated the post. About avoid the wCode if you stay with the if else it lead to duplicate the map access. The important stap is to use constant istead of raw strings in your code. — Mario Santini
– Mario Santini, Commented Dec 15, 2016 at 12:17

mtj · Accepted Answer · 2016-12-15 13:28:57Z

First of all, you should get rid of this if-else chain. You look for the first occurence of a string X out of a known list and then assign that X to a variable. So do this in a loop:

 String[] definitions = { "FG", "+SN", "SN", "-SN", "+RA", "-RA", "RA", "VV", "OVC", "BKN", "SCT", "FEW", "NSC", "CLR" }; String wCode = "CLR"; // fallback if nothing matches for(String def : definitions) { if(d.getRawData().contains(def)) { wCode = def; break; } }

(Or alternatives usings streams/filters/findFirst - the possibilities are numerous.)

Then, even if that has been refactored out in the first part of my answer (:-)) don't use whatever library to do basic functions from the standard library:

 TextUtils.isEmpty(wCode)

should be

 wCode.isEmpty()

instead (esp. as you can be sure that wCode != null as you did an initial assignment to the variable) - every java programmer understands that without looking it up.

Jamal · Accepted Answer · 2016-12-15 22:39:08Z

I was reading the code and I would have used this approach:

private PollFlightResponse.ResultEntity.MetarEntity parseWeather( PollFlightResponse.ResultEntity.MetarEntity d) { int severity = 0; //how bad is weather from 0 to 2 String wCode = ""; // map key String weather = ""; // human readable weather Enum urgentCodes{ FG("FG"), PSN("+SN), ......} urgentCodes code = urgentCodes.valueOf(currentCodeRead); switch(code) { case FG: conditions.put("FG", "Fog"); severity.put("FG", 2); break; case ..... default: //Conditons are clear code }

In writing this, I would make this into an object where I would set the values to be static. I would then create a method where the call would look like:

while ( code != '$' and code.hasNext() == true) //Looked like the marker //for the end of the line { int severity = weather.contitionLevel(String code); String weatherCond = weather.condition(String code); }

Now of course try and catch statements will have to be added for IO errors.

Stack Exchange Network

Parsing and summarizing an aviation METAR (US) report

4 Answers 4

You must log in to answer this question.

Hot Network Questions

Parsing and summarizing an aviation METAR (US) report

4 Answers 4

You must log in to answer this question.

Related

Hot Network Questions