Determinging the correct class structure

[Ashley Bye]

I'm after some advice on whether a design is correct and how to move forward.

I am writing a simple program to keep track of various logbooks. I have a "Logbook" class, which has an "ArrayList<LogbookEntry>" field; this is an abstract class which other "LogbookEntry" subclasses extend. Because each logbook has a different set of fields, I need to find a way to summarise the mathematical aspects of the entries in each logbook. Since a summary is not something that would be done on a single entry, it does not feel right to put an abstract "summarise()" method in the "LogbookEntry" class, with each subclass then defining how entries are summarised. Therefore, it seems logical that "Logbook" should also be abstract, with an abstract "summarise()" method that is implemented by its subclasses. Each subclass would take an instance of its related "ArrayList<LogbookEntry>" and all seems right with the world again. However, when impementing, I can't override the super constructor for "Logbook" with a more specific "ArrayList<LogbookEntry>". It would be bad if a TVLogbookEntry were passed in to a CookingLogbook and then passed the summarise() message, since half the fields would be missing or have different names. I also think, for the same reasons, that I will need a separate display class for each of the Logbook subclasses. This all seems to be rather clunky.

I think I have done the right thing: it certainly meets SRP, OCP, and DIP, and I think conforms to LSP also. ISP doesn't seem to be a factor. I believe this is a combination of the Strategy and Template patterns. Have I approached this problem in the right way? Can someone please give me some tips for moving forward, as I can't seem to hit th enail on the head?

I've uploaded a copy of the code to gist.

Thanks.

[Jesper de Jong]

It's not exactly clear to me what you think is not OK in the design.

You could use generics to make it more type-safe. Instead of having class Logbook store LogbookEntry objects, use a type parameter:

public abstract class Logbook<T extends LogbookEntry> {

    protected List<T> entries;

    public Logbook(List<T> entries) {
        this.entries = entries;
    }

    public void addEntry(T entry) {
        this.entries.add(entry);
    }

    protected abstract T getEntry(int entryId);

    protected abstract List<T> getAllEntries();

    protected abstract List<T> getEntriesBetween(Date from, Date to);
}

Then subclasses such as RNRWLogbook should fill this in with the appropriate subclass of LogbookEntry:

public class RNRWLogbook extends Logbook<RNRWLogbookEntry> {

    public RNRWLogbook(List<RNRWLogbookEntry> entries) {
        super(entries);
    }

    @Override
    protected RNRWLogbookEntry getEntry(int entryId) {
        // ...
    }

    @Override
    protected List<RNRWLogbookEntry> getAllEntries() {
        // ...
    }

    @Override
    protected List<RNRWLogbookEntry> getEntriesBetween(Date from, Date to) {
        // ...
    }
}

[Stephan van Hulst]

Hi Ashley,

I've once done something similar, except with a Calendar that can hold different types of Events.

What you could try is to make your Logbook generic. It could look something like this:

interface Logbook<E extends LogbookEntry, S extends LogbookSummary> {
  
  Collection<E> entries();

  S summarize();

}

final class Diary implements Logbook<DiaryEntry, DiarySummary> {
  
  Collection<DiaryEntry> entries() {
    //
  }

  DiarySummary summarize() {
    //
  }
}

[Stephan van Hulst]

Wow... so slow XD

[Campbell Ritchie]

I think this is too difficult a question for “beginning” so I shall move it.

[Ashley Bye]

Jasper,

Re-reading what I wrote, I will expand. My bigest concern was that I could pass any LogbookEntry to the RNRWLogbook at runtime, which would cause an error if it didn't have the correct fields to summarise. I initialy tried:

public RNRWLogbook(ArrayList<RNRWLogbookEntry> entries) {
        super(entries);
    }

However, that gives me an incompatible type error (ArrayList<RNRWLogbookEntry> cannot be converted to ArrayList<LogbookEntry>). I'd appreciate it if someone could explain this error and why it occurs, because I thought I could substitute a subclass in place of a superclass, a la "LogbookEntry entry = new RNRWLogbookEntry()". Clearly I have misunderstood something here.

I looked at using generics but couldn't work out how to implement it - it's still on my list f things to learn in more depth. Thank you for your solution, it looks like it will do exactly what I need and it gives me something to read through to understand a bit more about the correct syntax fo creating my own generic implementations.

[Stephan van Hulst]

Yes you can substitute a subtype for a supertype. However, ArrayList<RNRWLogbookEntry> is not a subtype of ArrayList<LogbookEntry>. Look at it this way:

Let's say you have a Cage<Animal> cage where you would like to put all sorts of animals. You can do cage.add(lion). If we could do cage = new Cage<Butterfly>(), then suddenly you can't do cage.add(lion) anymore, so obviously it's not a subtype.

I really recommend learning generics as soon as possible. It's an incredibly important part of the core language, and you will not be able to solve many problems easily without generics.

Another tip: do not use ArrayList in your method signatures, unless you need to guarantee your arguments have random access. Use less specific interfaces instead. This is what your constructors should look like:

abstract class Logbook<E extends LogbookEntry> {

  protected final List<E> entries;

  Logbook(Collection<? extends E> entries) {
    this.entries = new ArrayList<>(entries);
  }
}

final class RNRWLogbook<E extends RNRWLogbookEntry> extends Logbook<E> {

  RNRWLogbook(Collection<? extends E> entries) {
    super(entries);
  }
}

[Junilu Lacar]

By declaring fields and parameters as ArrayList, you are already violating LSP. Prefer to declare these as List instead so that your code could, theoretically at least, use any List implementation.

Also, there are a number of names that you use that are cryptic. What is acType and acReg? What about "RNRW", what's that supposed to mean? Why make the reader even need to ask these questions?

Data Access Objects are usually two-way deals, handling reads and writes. As you have it now, the RNRWLogbookDAO only does reads. The name is misleading. A name like LogbookReader would be more fitting, IMO.

You may think that names are not that important and I'm nitpicking at trivial things but names are in fact very important to getting to a good design and I would argue that poor names are one of the leading causes of poor designs.

[Stephan van Hulst]

Junilu Lacar wrote:By declaring fields and parameters as ArrayList, you are already violating LSP.

Not really. It's just not convenient.

[Junilu Lacar]

Stephan van Hulst wrote:

Junilu Lacar wrote:By declaring fields and parameters as ArrayList, you are already violating LSP.

Not really. It's just not convenient.

Ok, LSP may not technically be violated but practically speaking, I see no reasonable justification for extending ArrayList for this so the code is basically bound to accepting and using ArrayLists rather than any List. At any rate, you should prefer this instead:

   List<E> foo = new ArrayList<>();

   public void foo(List<E> bar) {...}

[Ashley Bye]

Stephan, thank you. I will refactor my code and move Generics up my study list. I'm working on all the minute details of the OCA8 syllabus at the moment, which is the priority.

Junilu, points noted. It is not yet a complete implementation, as I wanted to be able to import a CSV to work with initially. The names are abbreviations of various types of logbook or aircraft detail. I guess I could make them clearer for someone who doesn't understand the topic so wouldn't know the abbreviation.