This code takes a JSONObject, passes it to another class which is responsible for mapping the data in the JSONObject to a different format (Still a JSONObject though). I'd just like confirmation if the code below is bad practice or not, I feel it is:
public class DeliveredCommunication{
private String deviceId;
private int versionMajor;
private int versionMinor;
private int versionPatch;
private JSONArray messages;
private DataMapper dataMapper = null;
private Logger log = (Logger) LogManager.getLogger();
public DeliveredCommunication(String deviceId, int versionMajor, int versionMinor, int versionPatch,
JSONArray messages) {
this.deviceId = deviceId;
this.versionMajor = versionMajor;
this.versionMinor = versionMinor;
this.versionPatch = versionPatch;
this.messages = messages;
}
public String getDeviceId() {
return deviceId;
}
public int getVersionMajor() {
return versionMajor;
}
public int getVersionMinor() {
return versionMinor;
}
public int getVersionPatch() {
return versionPatch;
}
public JSONArray getMessages() {
return messages;
}
public void mapData(){
dataMapper = new DataMapper();
JSONObject msg = null;
for(int i = 0; i < messages.length(); i++){
try {
msg = messages.getJSONObject(i);
dataMapper.map(msg);
} catch (JSONException e) {
log.warn(deviceId + ": A JSONException occurred trying to map Data for database insertion: " + msg, e);
}
}
}
public DataMapper getDataMapper(){
return dataMapper;
}
}
I'm not 100% sure though, because the mapData()
method in the DataMapper
only maps one object
at a time.
Would it be a better design to pass the whole list into the DataMapper
first, then return the DataMapper
object without the data being mapped and let the class that called getDataMapper()
call dataMapper.mapData()
to map all the data in one go?
Does that make sense?
Or Instead of having getDataMapper()
I could just have a method called getMappedData()
and have that method do the work in mapData()
and just get and return a list of mapped objects from the DataMapper
.