Problem
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.
Solution
It’s always a bad sign if you do have different levels of presentations inside a class.
It would be much better to separate the concerns and implement based on the “single responsibility principle”.
That would mean that you have the class DeliveredCommunication which holds a set of Strings/Objects for the messages. This is also known as a DTO (Data Transfer Object).
Everyone who wants to use it, can now decide how the presentation should look like.
Transforming data to a specific exchange format should be done at the level of your application where the exchange happens.
Hope it is clear enough and that it helps
P.S.: I would change DeliveredCommunicaton to Communication with a property called state which could be realized with an enum of which one value would be ‘delivered’.