Serializing types with a common superclass

Question

I have written code responsible for saving data to a file using Serialization. I think that a natural way to avoid repetition is to use inheritance but I'm not sure if it would work because of generics.

How can I remove all the repetition in the below code?

Some general information:

1. There is a class StoredItem and classes PipeCable, Element, ValveMotor which extend StoredItem.

2. There is Data class in which are ObservableLists of PipeCable, Element, ValveMotor

The code that I'd like to improve but I don't know how:

private void savePipeCableList(Data data) {
    try {
        Path path = Paths.get(Strings.dataPipeCableFileName);
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(new ArrayList<PipeCable>(data.getPipeCableList()));
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

private void saveElementList(Data data) {
    try {
        Path path = Paths.get(Strings.dataElementFileName);
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(new ArrayList<Element>(data.getElementList()));
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

private void saveValveMotorList(Data data) {
    try {
        Path path = Paths.get(Strings.dataValveMotorFileName);
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(new ArrayList<ValveMotor>(data.getValveMotorList()));
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

I will appreciate any suggestions.

Answer 1

I think in this case you want a generic method, something on this general order:

static <T> private void saveGeneric(ArrayList<T> data, Path path) {
    try {
        OutputStream fos = Files.newOutputStream(path);
        ObjectOutputStream oos = new ObjectOutputStream(fos);
        oos.writeObject(data);
        oos.close();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

Then you might want to write the individual wrappers to match the functions you started with:

private void savePipeCableList(Data data) {
    saveGeneric(new ArrayList<PipeCable>(data.getPipeCableList()),
                Paths.get(Strings.dataPipeCableFileName));
}

private void saveElementList(Data data) {
    SaveGeneric(new ArrayList<Element>(data.getElementList()),
                Paths.get(Strings.dataElementFileName));
}

private void saveValveMotorList(Data data) {
    SaveGeneric(new ArrayList<ValveMotor>(data.getValveMotorList()),
                Paths.get(Strings.dataValveMotorFileName));
}

As for the code otherwise, the point that seems most obvious to me would be to replace the mostly manual opening and closing of the stream with a try with-resources statement:

// Note: I generally avoid Java, so I probably got at least a few details of
// the syntax wrong here.
OutputStream fos = Files.newOutputStream(path);
try (ObjectOutputStream oos = new ObjectOutputStream(fos))
{       
    oos.WriteObject(data);
}
catch(FileNotFoundException e) {
    // ...

This will automatically close oos upon exit from the try block, regardless of whether you exit normally or via an exception. With the code as you've written it, if an exception is thrown while writing, oos.close() won't execute.

It's not immediately clear whether fos might benefit from similar treatment--without knowing how File.newOutputStream works and what it does, I can't really even guess about that (but perhaps you now know more about what to look for and consider). Right now, you have the Files.newOutputStream inside the try block (which I don't) but if you're just getting an existing object that you know exists, that won't matter. If it could throw an exception, you may have a more difficult problem, where you'd need nested try blocks (the inner of the "with resources" variety) to ensure correct behavior.

One other point. I hesitate to mention it, because I might just be demonstrating my ignorance of Java, but at least offhand, it seems like you probably don't need to create a complete copy of the data in memory before you write it to the output stream:

oos.writeObject(new ArrayList<ValveMotor>(data.getValveMotorList()));

At least offhand, it seems like it should be sufficient to use:

oos.writeObject(data.getValveMotorList());

The obvious reason we'd copy the data before serializing it would be if writeObject were asynchronous, and we need want to ensure we get a stable snapshot of the data at the time we started the serialization. If memory serves, writeObject is synchronous though.

Answer 2

I think the main problem in your code is the concept of "Data": when Data is a single class which represents a logical entity, you should not save it in three different places. In that case, write one method which writes the complete data object to whatever store, and be done.

On the other hand, if Data is just a holder for various parts that do not have a logical connection, Data should not be passed to the various serialization methods. (The serialization method does three things here: extract the values to save from the data object, identify the place to save, and actually save.) If you go for separation of concerns, have the caller to the extract and identify parts, and have a save method simply save a given list-of-whatever to a given file.

At that point, smoothly go over to Jerry's answer... :-)