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

Answer 1

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.

Answer 2

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

Answer 3

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.

Answer 4

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.