Problem
I am trying to fetch weather data from the OpenWeatherMap API. The responded JSON will be deserialized into a object which has the same structure of the JSON, then we need to transform this object into another class to meet our needs.
Following is the class which the JSON returned by OpenWeatherMap will be deserialized:
public class OpenWeatherMapResponse implements Serializable {
private static final long serialVersionUID = 8841521123253407726L;
private City city;
private int cod;
private Item[] list;
// lots of accessors
public class static Item {
// members of inner class Item
}
public class static Weather {
// members of inner class Weather
}
}
The following is a snippet of the second class. This class has a constructor which takes a OpenWeatherMapResponse
and use the value in the response to construct the object will actually be used by my system:
public class CityWeather implements Serializable {
private static final long serialVersionUID = -4883661697465418806L;
private static final String ISO8601_FORMAT = "yyyy-MM-dd HH:mm:ss";
private double temp;
private double minTemp;
private double maxTemp;
private long weatherId;
private String weatherName;
private String weatherDesc;
public CityWeather() {}
public CityWeather(OpenWeatherMapResponse openWeatherResponse) {
Item[] list = openWeatherResponse.getList();
Weather weather = findWeather(list); // Get the first element of the Weather list
// OpenWeatherMap doesn't provide API for fetching a specific day's forecast, so we have to build a day's weather data by assembling ourselves.
Date tomorrow = getDateOfTomorrow();
double tempMax = 0.0, tempMin = 99.0;
List<Double> temps = new ArrayList<Double>();
for (Item item : list) {
Date dt = null;
if((dt = parseDtTxt(item.getDt_txt())) == null) {
continue;
}
// We only use weather data in recent 24 hours.
if (dt.after(tomorrow)) {
break;
}
Main main = item.getMain();
temps.add(main.getTemp());
tempMax = Math.max(tempMax, main.getTemp_max());
tempMin = Math.min(tempMin, main.getTemp_min());
}
this.weatherId = weather.getId();
this.weatherName = WeatherType.fromId((int) weather.getId()).name();
this.weatherDesc = weather.getDescription();
this.temp = calculateMean(temps);
this.maxTemp = tempMax;
this.minTemp = tempMin;
}
// lots of accessors
// Parse the datetime string
private Date parseDtTxt(String dtTxt) {
try {
return new SimpleDateFormat(ISO8601_FORMAT).parse(dtTxt);
} catch (ParseException ex) {
return null;
}
}
private Date getDateOfTomorrow() {
Calendar temp = Calendar.getInstance();
temp.add(Calendar.DATE, 1);
return temp.getTime();
}
private Weather findWeather(Item[] list) {
for (Item item : list) {
if (item.getWeather() !=null && item.getWeather().length > 0) {
return item.getWeather()[0];
}
}
return null;
}
private double calculateMean(List<Double> numbers) {
if (isEmpty(numbers)) return 0;
double sum = 0;
for (double d : numbers) {
sum += d;
}
return sum/numbers.size();
}
}
As you can see, although this CityWeather
looks like a data object, there is quite a little logic in this class to initialize its attributes.
Is this a good practice? If not, how should I modify my code?
Solution
I would suggest splitting this into two classes, one being your CityWeather
POJO and a separate class for performing the logic currently in CityWeather
. The primary reason for this is to separate the business logic here (the parsing of the response from OpenWeatherMap) and your representation of the response (CityWeather
). In doing so, whichever class you have that is currently invoking the call to OpenWeatherMap should then pass the OpenWeatherMapResponse
response that is returned from the call to this new class, which we’ll call OpenWeatherMapParser
. OpenWeatherMapParser
should take this response, and instantiate CityWeather
objects based on the logic you currently have in CityWeather
by calling it’s setter methods