HL7 message builder and unit tests

Question

I had to code a project due to my final exams in April. It is an application which simulates a hospital. There are admissions, transfers and discharges of patients in this hospital. For each of those, there is a hl7 message generated (and in progress send to a server). What is HL7?

For this reason, I implemented the class HL7BuilderImpl with the interface HL7Builder. This Builder is part of my exam and I have to show it to the audit commission.

The HL7Builder gets a Patient (with data like birthday, gender, name and stuff), type (this is an enum class, which contains admission, transfer and discharge) and a format (pipe or xml, but I only had to implement the pipe option), it returns a String. This is, how one of my generated HL7 Messages for a patients admission would look like:

MSH|^~\&|KIS|Testsender|HIS|Testreceiver|201802281055||ADT^A01^ADT_A01|62|T|2.4
EVN|A01|201802281055
PID|||HL7SIM000000041||Doe^John||19780320|M
PV1||I|KAR-2^^^KAR||||||||||||||||HL7SIM000000041C1||||||||||||||||||||KAR|||||201802281055

I used the HAPI Framework to map the patients data to the fields of an hl7-message. Finally, the messaged is parsed to a String and returned to a sender (which is not part of my exams, so i will not ask for reviewing this here).

It is working well and does, what it should, the tests are running green, but i also am an greenhorn in programming, so I would like to ask you some questions:

• What do you think about the structure? I tried to order the methods from general to special an i also tried to seperate as much as necessary and as less as possible.
• First, I implemented 2 methods, which only contain some checks and then delegate to further methods. Due to my less experience, is this a good way to code in practice?
• At this point, I dont know a lot about security with my code. Is there some point, which could be a problem with security with my code?
• I dont have a lot of experience with Unit Tests, i coded some and got a coverage > 80% for my class but i am not sure, if my tests are kinda "too easy"?
• Did I miss something to test?

I would be very thankful for some tips, advice and improvement proposals with my code. Here the HL7BuilderImpl class:

package com.hl7sim.hl7;


import java.io.IOException;
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
import com.hl7sim.patient.Patient;
import ca.uhn.hl7v2.DefaultHapiContext;
import ca.uhn.hl7v2.HL7Exception;
import ca.uhn.hl7v2.HapiContext;
import ca.uhn.hl7v2.model.AbstractMessage;
import ca.uhn.hl7v2.model.DataTypeException;
import ca.uhn.hl7v2.model.v24.message.ADT_A01;
import ca.uhn.hl7v2.model.v24.message.ADT_A02;
import ca.uhn.hl7v2.model.v24.message.ADT_A03;
import ca.uhn.hl7v2.model.v24.segment.EVN;
import ca.uhn.hl7v2.model.v24.segment.MSH;
import ca.uhn.hl7v2.model.v24.segment.PID;
import ca.uhn.hl7v2.model.v24.segment.PV1;
import ca.uhn.hl7v2.parser.Parser;


public class HL7BuilderImpl implements HL7Builder {


private static AtomicInteger messageControlId; 
private List<String> allHL7s;


public HL7BuilderImpl() {
    HL7BuilderImpl.messageControlId = new AtomicInteger(0);
    this.allHL7s = new ArrayList<String>();
}


public static int getMessageControlId() { 
    return messageControlId.get(); 
}

public static void setMessageControlId(AtomicInteger messageControlId) {
    HL7BuilderImpl.messageControlId = messageControlId;
}

@Override
public List<String> getAllHL7s() {
    return allHL7s;
}

@Override
public void setAllHL7s(List<String> allHL7s) {
    this.allHL7s = allHL7s;
}


@Override
public String createMessage(Patient patient, Type type, Format format) {
    if (patient == null) {
        throw new IllegalArgumentException("No Patient");
    }
    if (type == null) {
        throw new IllegalArgumentException("No Messagetype");
    }
    if (format == null) {
        throw new IllegalArgumentException("Argument 'format' must not be null");
    }
    String message = "";
    if (format == Format.PIPE) {
        message = createPipeMessage(patient, type);
    } else if (format == Format.XML) {

    } else {
        throw new IllegalArgumentException("Format '" + format.name() + "' is not supported.");
    }
    return message;
}

@Override
public String createPipeMessage(Patient patient, Type type) {
    if(type == null) {
        throw new IllegalArgumentException("Type may not be null");
    }
    if(patient == null) {
        throw new IllegalArgumentException("No Patients");
    }
    switch (type) {
    case ADMISSION:
        return createPipeMessageAdmission(patient, type);
    case DISCHARGE:
        return createPipeMessageDischarge(patient, type);
    case TRANSFER:
        return createPipeMessageTransfer(patient, type);
    default:
        throw new IllegalArgumentException("Unknown Type, Builder not working with this Type");
    }
}

@Override
public String createPipeMessageAdmission(Patient patient, Type type) {
    ADT_A01 adt = new ADT_A01();
    patient.setAdmissionDateTime(LocalDateTime.now());
    try {
        adt.initQuickstart("ADT", "A01", "T");
        setMshSegment(adt.getMSH(), patient);
        setPidSegment(adt.getPID(), patient);
        setEvnSegment(adt.getEVN(), type);
        setPv1Segment(adt.getPV1(), patient);
        return parseMessage(adt);
    } catch (IllegalArgumentException e) {
        throw new IllegalArgumentException("Illegal Argument Exception", e);
    } catch (DataTypeException e) {
        throw new HL7BuilderException("Data type exception", e);
    } catch (HL7Exception e) {
        throw new HL7BuilderException("HL7 exception", e);
    } catch (IOException e) {
        throw new HL7BuilderException("IO exception", e);
    }
}

@Override
public String createPipeMessageTransfer(Patient patient, Type type) {
    ADT_A02 adt = new ADT_A02();
    try {
        adt.initQuickstart("ADT", "A02", "T");
        setMshSegment(adt.getMSH(), patient);
        setPidSegment(adt.getPID(), patient);
        setEvnSegment(adt.getEVN(), type);
        setPv1Segment(adt.getPV1(), patient);
        setPriorLocation(adt, patient);
        return parseMessage(adt);
    } catch (DataTypeException e) {
        throw new HL7BuilderException("Data Type Exception", e);
    } catch (HL7Exception e) {
        throw new HL7BuilderException("HL7 Exception", e);
    } catch (IOException e) {
        throw new HL7BuilderException("I/O Exception", e);
    }
}

@Override
public String createPipeMessageDischarge(Patient patient, Type type) {
    ADT_A03 adt = new ADT_A03();
    patient.setDischargeDateTime(LocalDateTime.now());
    try {
        adt.initQuickstart("ADT", "A03", "T");
        setMshSegment(adt.getMSH(), patient);
        setPidSegment(adt.getPID(), patient);
        setEvnSegment(adt.getEVN(), type);
        setPv1Segment(adt.getPV1(), patient);
        return parseMessage(adt);
    } catch (DataTypeException e) {
        throw new HL7BuilderException("Data Type Exception", e);
    } catch (HL7Exception e) {
        throw new HL7BuilderException("HL7 Exception", e);
    } catch (IOException e) {
        throw new HL7BuilderException("I/O Exception", e);
    }
}

private void setMshSegment(MSH msh, Patient patient) throws DataTypeException {
    LocalDateTime now = LocalDateTime.now();
    msh.getSendingFacility().getNamespaceID().setValue("Testsender");
    msh.getSendingApplication().getNamespaceID().setValue("KIS");
    msh.getReceivingFacility().getNamespaceID().setValue("Testreceiver");
    msh.getReceivingApplication().getNamespaceID().setValue("HIS");
    msh.getDateTimeOfMessage().getTimeOfAnEvent().setDateMinutePrecision(now.getYear(), now.getMonthValue(),
            now.getDayOfMonth(), now.getHour(), now.getMinute());
    msh.getMessageControlID().setValue(String.valueOf(messageControlId.incrementAndGet()));
}

private void setEvnSegment(EVN evn, Type type) throws DataTypeException {
    LocalDateTime now = LocalDateTime.now();
    evn.getRecordedDateTime().getTimeOfAnEvent().setDateMinutePrecision(now.getYear(), now.getMonthValue(),
            now.getDayOfMonth(), now.getHour(), now.getMinute());
    if (type == Type.DISCHARGE) {
        evn.getEventTypeCode().setValue("A03");
    } else if (type == Type.TRANSFER) {
        evn.getEventTypeCode().setValue("A02");
    } else if (type == Type.ADMISSION) {
        evn.getEventTypeCode().setValue("A01");
    }
}

private void setPidSegment(PID pid, Patient patient) throws DataTypeException {
    pid.getPatientIdentifierList(0).getCx1_ID().setValue(String.valueOf(patient.getId()));
    pid.getAdministrativeSex().setValue(patient.getGender());
    pid.getPatientName(0).getGivenName().setValue(patient.getFirstname());
    pid.getPatientName(0).getFamilyName().getSurname().setValue(patient.getLastname());
    pid.getDateTimeOfBirth().getTimeOfAnEvent().setDatePrecision(patient.getBirthday().getYear(),
            patient.getBirthday().getMonthValue(), patient.getBirthday().getDayOfMonth());
}

private void setPv1Segment(PV1 pv1, Patient patient) throws DataTypeException {
    setPV1Time(pv1, patient);
    setPV1Location(pv1, patient);
}

private void setPV1Time(PV1 pv1, Patient patient) throws DataTypeException {
    if (patient.getAdmissionDateTime() != null) {
    pv1.getAdmitDateTime().getTimeOfAnEvent().setDateMinutePrecision(patient.getAdmissionDateTime().getYear(),
            patient.getAdmissionDateTime().getMonthValue(), patient.getAdmissionDateTime().getDayOfMonth(),
            patient.getAdmissionDateTime().getHour(), patient.getAdmissionDateTime().getMinute());
    }
    if (patient.getDischargeDateTime() != null) {
        pv1.getDischargeDateTime(0).getTimeOfAnEvent().setDateMinutePrecision(patient.getDischargeDateTime().getYear(),
                patient.getDischargeDateTime().getMonthValue(), patient.getDischargeDateTime().getDayOfMonth(),
                patient.getDischargeDateTime().getHour(), patient.getDischargeDateTime().getMinute());
    }

}

private void setPV1Location(PV1 pv1, Patient patient) throws DataTypeException {
    pv1.getServicingFacility().setValue(patient.getDepartment());
    pv1.getAssignedPatientLocation().getPl1_PointOfCare().setValue(patient.getWard());
    pv1.getAssignedPatientLocation().getPl4_Facility().getNamespaceID().setValue(patient.getDepartment());
    pv1.getPatientClass().setValue(patient.getStatus());
    pv1.getPv119_VisitNumber().getCx1_ID().setValue(String.valueOf(patient.getInstance()));
}

private void setPriorLocation(ADT_A02 adt, Patient patient) throws DataTypeException {
    adt.getPV1().getPriorPatientLocation().getPl1_PointOfCare().setValue(patient.getPriorWard());
    adt.getPV1().getPriorPatientLocation().getPl4_Facility().getNamespaceID()
            .setValue(patient.getPriorDepartment());
}

private String parseMessage(AbstractMessage message) throws HL7Exception {
    try (HapiContext context = new DefaultHapiContext()) {
        context.getExecutorService();
        Parser parser = context.getPipeParser();
        final String result = parser.encode(message);
        allHL7s.add(result);
        return result;
    } catch (IOException e) {
        throw new HL7BuilderException("Error on parsing message.", e);
    }
} 


}

And here the belonging JUnit tests:

package com.hl7sim;


import java.time.LocalDate;
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;
import static org.junit.Assert.*;
import org.junit.Before;
import org.junit.Test;
import com.hl7sim.hl7.Format;
import com.hl7sim.hl7.HL7Builder;
import com.hl7sim.hl7.HL7BuilderImpl;
import com.hl7sim.hl7.Type;
import com.hl7sim.patient.Patient;


public class HL7BuilderImplTest {


HL7Builder testHl7builder;
List<String> testAllHl7s;
Patient testPatient;
Patient testPatientTwo;
List<Patient> testBothPatients;


@Before
public void setUp() throws Exception {

    testHl7builder = new HL7BuilderImpl();

    testPatient = new Patient.Builder().build();
    testPatient.setBirthday(LocalDate.of(1911, 11, 11));
    testPatient.setFirstname("Test");
    testPatient.setLastname("Mann");
    testPatient.setGender("M");
    testPatient.setAdmissionDateTime(LocalDateTime.now());
    testPatient.setDischargeDateTime(LocalDateTime.now());

    testPatientTwo = new Patient.Builder().build();
    testPatientTwo = testPatient;

    testBothPatients = new ArrayList<Patient>();

    testAllHl7s = new ArrayList<String>();

    testBothPatients.add(testPatient);
    testBothPatients.add(testPatientTwo);
}


@Test
public void testGetMessageControlId() {

    int result = HL7BuilderImpl.getMessageControlId();

    assertTrue(result == 0);
}

@Test
public void testAllHl7sAtBegin() {

    int result = testAllHl7s.size();

    assertTrue(result == 0);
}

@Test
public void testAddingOneHl7ToAllHl7s() {

    String test = "Test";
    testAllHl7s.add(test);

    int result = testAllHl7s.size();

    assertTrue(result == 1);
}

@Test(expected = IllegalArgumentException.class)
public void testCreateMessageWithNullValuesForOnePatient() {

    testHl7builder.createMessage((Patient)null, null, null);
}

@Test(expected = IllegalArgumentException.class)
public void testCreateMessageOnePatientWithoutType() {

    testHl7builder.createMessage(testPatient, null, Format.PIPE);
}

@Test(expected = IllegalArgumentException.class)
public void testCreateMessageOnePatientWithoutFormat() {

    testHl7builder.createMessage(testPatient, Type.ADMISSION, null);
}

@Test(expected = IllegalArgumentException.class)
public void testCreateMessageOnePatientWithWrongFormat() {

    testHl7builder.createMessage(testPatient, Type.ADMISSION, Format.UNKNOWN);
}

@Test(expected = IllegalArgumentException.class)
public void testCreateMessageOnePatientWithWrongType() {

    testHl7builder.createMessage(testPatient, Type.UNKNOWN, Format.PIPE);
}

@Test(expected = IllegalArgumentException.class)
public void testCreatePipeMessageOnePatientNoType() {

    testHl7builder.createPipeMessage(testPatient, null);
}

@Test(expected = IllegalArgumentException.class)
public void testCreatePipeMessageWithoutOnePatient() {

    testPatient = null;

    testHl7builder.createPipeMessage(testPatient, Type.ADMISSION);
}

@Test
public void testCreatePipeMessageOnePatientWrongType() {

    testHl7builder.createPipeMessage(testPatient, Type.TRANSFER);
}

@Test
public void testPatientSetFirstNameName() {
    // Given
    testPatient.setFirstname("Albert");
    // When
    String result = testHl7builder.createMessage(testPatient, Type.ADMISSION, Format.PIPE);
    // Then
    assertTrue(result.contains("^Albert|"));        
}

@Test
public void testPatientSetLastName() {

    testPatient.setLastname("Meier");

    String result = testHl7builder.createMessage(testPatient, Type.ADMISSION, Format.PIPE);

    assertTrue(result.contains("|Meier^"));     
}

@Test
public void testcreatePipeMessageAdmission() {

    String result = testHl7builder.createMessage(testPatient, Type.ADMISSION, Format.PIPE);

    assertTrue(result.contains("|ADT^A01^ADT_A01"));
}

@Test 
public void testcreatePipeMessageDischarge() {

    String result = testHl7builder.createMessage(testPatient, Type.DISCHARGE, Format.PIPE);

    assertTrue(result.contains("|ADT^A03^ADT_A03"));
}

@Test
public void testWithIncompletePatientData() {

    testPatient.setLastname("");

    String result = testHl7builder.createPipeMessage(testPatient, Type.ADMISSION);

    System.out.println(result);
}


}

Answer 1

Welcome to Code Review and thanks for a nicely prepared question!

Structure

Concerning the design of this code, there is clearly an issue that should be solved with OOP approach. I mean that the following methods:

public String createPipeMessage(Patient patient, Type type);
public String createPipeMessageAdmission(Patient patient, Type type);
public String createPipeMessageTransfer(Patient patient, Type type);
public String createPipeMessageDischarge(Patient patient, Type type);

all have same role: creating a message.

Since the HL7Builder interface is in the same package as the implementation, I suppose that this interface does not come from a third party API and it is something that you can define and change.

I suggest defining an interface dedicated to pipe message building:

public interface PipeMessageBuilder {
  public String createPipeMessage(Patient patient, Type type);
}

And there should be an implementation for each type of message:

public class AdmissionMessageBuilder implements PipeMessageBuilder {

  @Override
  public String createPipeMessage(Patient patient, Type type) {
    // contents of current createPipeMessageAdmission() impl
  }
}

// and so on for other message types

HL7Builder should provide a method that returns a pipe message builder depending on the format, for example:

public class HL7Builder {

  public PipeMessageBuilder getBuilderFor(Format format) {
    // logic of bulder choice
  }

}

I think that with this approach it is not mandatory to bind HL7Builder to an interface. The getBuilderFor method may even be static. The returned builder references may be either constant or instantiated for each call: it depends on other details.

With this approach, building a message will be reduced to this chaining:

String message = HL7Builder.getBuilderFor(format).createPipeMessage(patient, type);

The advantages are significant: 1) flexibility and separation of concerns; 2) tests are much easier.

The private methods set*Segment, used by all the builders, should be grouped in an abstract class and be accessible from the implementations, so the builders class headers should finally be like public class AdmissionMessageBuilder extends AbstractPipeMessageBuilder. setEvnSegment should be abstract and implemented for each concrete message builder, without the if conditions.

Other Issues

Args Validation

Multiple not-null checks might be extracted:

private static void ensureNotNull(String value, String argName) {
  if (value == null) {
    throw new IllegalArgumentException(argName + " must not be null");
  }
}

This will significantly reduce the validation code, for example:

ensureNotNull(patient, "patient");
ensureNotNull(type, "type");
ensureNotNull(format, "format");

Exceptions Handling

There is a lot of pollution with unnecessary information or layers:

catch (IllegalArgumentException e) {
    throw new IllegalArgumentException("Illegal Argument Exception", e);
} catch (DataTypeException e) {
    throw new HL7BuilderException("Data type exception", e);
} catch (HL7Exception e) {
    throw new HL7BuilderException("HL7 exception", e);
} catch (IOException e) {
    throw new HL7BuilderException("IO exception", e);
}

IAE should not be caught, because it is wrapped in yet another IAE. Is it useful?

The messages in HL7BuilderException are not informative at all. The type of the wrapped exception is known anyway, so the messages should be changed or removed, to have simply throw new HL7BuilderException(e).

Law of Demeter

Multiple calls like

msh.getSendingApplication().getNamespaceID().setValue("KIS");

represent violations of Law of Demeter. It looks like there are more problems with other entities design.

JUnit Tests

The test case class must be in the same package than the tested entity.

There is a mess in testPatient* initializations:

testPatientTwo = new Patient.Builder().build();
testPatientTwo = testPatient;

I'd also suggest removing Patient fields at all from the test class and create instances of Patient only in the methods that need to use this object.

Calls like assertTrue(result == 0) should be avoided, because they lack explicitness when there are test failures. It is more expressive when reformulated as assertEquals(0, result), or even with an explanation/details message.

If there is no way to avoid assertTrue, like in assertTrue(result.contains("|ADT^A01^ADT_A01")), there should be an explanation message, for example:

String expected = "|ADT^A01^ADT_A01";
assertTrue(String.format("Result did not contain '%1$s'", expected), result.contains(expected));

Otherwise, a test failure will produce the message "false was not true".

And of course, if you choose to extract message builders in separate implementing classes, there should be separate test classes corresponding to each builder.