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

            } 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.


1 Answer 1


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'.

  • \$\begingroup\$ Thanks for your answer. The data mapper is to be used to convert messages to a standard format for the data access object so it can easily be inserted into the database. I'm realising my approach is very wrong. I'll need to have a think about how to do that. \$\endgroup\$
    – mal
    Commented Apr 8, 2016 at 18:02
  • \$\begingroup\$ I missed the part of your question where you already got the right approach. Yes it's better if the object that requests the DataMapper to also be responsible for when the data will be mapped. \$\endgroup\$ Commented Apr 8, 2016 at 18:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.