3
\$\begingroup\$

I am writing a system for doctors who are regularly visited by patients. The doctor adds each patient's visit to collection/database. The class can save data to an .xml file and restore it. Can you give your feedback about this code?

// https://github.com/veitsi/jcrf-noweb
import java.util.ArrayList;
import java.util.Date;
import java.io.File;

import javax.xml.bind.JAXBContext;
import javax.xml.bind.JAXBException;
import javax.xml.bind.Marshaller;
import javax.xml.bind.Unmarshaller;
import javax.xml.bind.annotation.XmlElement;
import javax.xml.bind.annotation.XmlRootElement;

@XmlRootElement
public class Jcrf {
    @XmlElement
    Jcrf jcrf;
    @XmlElement
    ArrayList<Visit> vst = new ArrayList<Visit>();

    public Jcrf() {
        super();
    }

    public static void main(String[] args) {
        Jcrf jcrf = new Jcrf();
         jcrf.addTestData();

        System.out.println(jcrf.vst);

        jcrf.toXml();
        jcrf.fromXml();

        System.out.println(jcrf.vst);
    }

    public void toXml() {
        try {
            File file = new File("jcrf.xml");
            JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
            Marshaller jaxbMarshaller = jaxbContext.createMarshaller(); // output
            jaxbMarshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
            jaxbMarshaller.marshal(this, file);
            jaxbMarshaller.marshal(this, System.out);
        } catch (JAXBException e) {
            e.printStackTrace();
        }
    }

    public void fromXml() {
        try {
            File file = new File("jcrf.xml");
            JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
            Unmarshaller jaxbUnmarshaller = jaxbContext.createUnmarshaller();
            jcrf = (Jcrf) jaxbUnmarshaller.unmarshal(file);
            vst = jcrf.vst;
        } catch (JAXBException e) {
            e.printStackTrace();
        }
    }

    public void addTestData() {
        vst.add(new Visit(1, 1, 366));
        vst.add(new Visit(1, 2, 365));
        vst.add(new Visit(2, 1, 364));
    }
}

class Visit {
    @XmlElement
    int ptn;
    @XmlElement
    int nmr;
    @XmlElement
    static final Date dt = new Date();
    @XmlElement
    int ttr;

    public Visit() {
    }

    public Visit(int ptn, int nmr, int ttr) {
        super();
        if (ttr < 300 || ttr > 420)
            throw new IllegalArgumentException("bad temperature");
        this.ptn = ptn;
        this.nmr = nmr;
        this.ttr = ttr;
    }

    @Override
    public String toString() {
        return "Visit [ptn=" + ptn + ", nmr=" + nmr + ", ttr=" + ttr + ", dt="
                + dt + "]";
    }

}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

File-systems normally make poor databases because you lose the concept of 'atomicity'... let's take a worst-case example:

public void toXml() {
    try {
        File file = new File("jcrf.xml");
        JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
        Marshaller jaxbMarshaller = jaxbContext.createMarshaller(); // output
        jaxbMarshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
        jaxbMarshaller.marshal(this, file);
        jaxbMarshaller.marshal(this, System.out);
    } catch (JAXBException e) {
        e.printStackTrace();
    }
}

What happens if there is an IOException part-way through the jaxbMarshaller.marshal(this, file); ? You end up overwriting the old copy, and losing the new one. Your data is gone.

A more traditional way to prevent this sort of loss is to write the output to a new file, and then do an atomic 'move' operation:

public void toXml() {
    try {
        File file = new File("jcrf.xml.tmp");
        JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
        Marshaller jaxbMarshaller = jaxbContext.createMarshaller(); // output
        jaxbMarshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
        jaxbMarshaller.marshal(this, file);
        jaxbMarshaller.marshal(this, System.out);

        // move tmp file to complete output file.
        Path tmp = file.toPath();
        Path target = Paths.get("jcrf.xml");
        Files.move(tmp, target, CopyOption.REPLACE_EXISTING, CopyOption.ATOMIC_MOVE);

    } catch (JAXBException e) {
        e.printStackTrace();
    }
}

Note the use of the atomic Files.move(...)` command.

One day, when your file-system is full, and you run out of space... you will appreciate that change.

\$\endgroup\$
2
\$\begingroup\$

Variable Names

Your variable names tell a reader basically nothing:

  • I know that vst means visits, but only because of the declaration as ArrayList<Visit> (otherwise, it could just as well mean visitors, or even something completely different). Inside the code (for example when reading vst = jcrf.vst;), I have to perform the mapping from vst to visitors each time myself, which makes it harder to understand the meaning of it.
  • I have no idea what ptn, nmr, or ttr are. My guesses are patient (but patient what? Not name, as it's an integer), number (what kind of number? social security?), and temperature (because of the exception method. And I'm assuming kelvin, because of the constraints). To make your code more readable (to future maintainers of the code, but also to future you), you should not abbreviate variable names. Just write patientID, socialSecurityNumber, and temperatureKelvin (if those are correct).
\$\endgroup\$

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.