• Post Reply Bookmark Topic Watch Topic
  • New Topic

Determinging the correct class structure  RSS feed

 
Ashley Bye
Ranch Hand
Posts: 140
2
Java Mac
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Java Cowboy
Sheriff
Posts: 16060
88
Android IntelliJ IDE Java Scala Spring
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:

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

 
Stephan van Hulst
Saloon Keeper
Posts: 7991
143
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:
 
Stephan van Hulst
Saloon Keeper
Posts: 7991
143
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Wow... so slow XD
 
Campbell Ritchie
Marshal
Posts: 56546
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I think this is too difficult a question for “beginning” so I shall move it.
 
Ashley Bye
Ranch Hand
Posts: 140
2
Java Mac
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:



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
Saloon Keeper
Posts: 7991
143
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:


 
Junilu Lacar
Sheriff
Posts: 11493
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Saloon Keeper
Posts: 7991
143
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:By declaring fields and parameters as ArrayList, you are already violating LSP.


Not really. It's just not convenient.
 
Junilu Lacar
Sheriff
Posts: 11493
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:

 
Ashley Bye
Ranch Hand
Posts: 140
2
Java Mac
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
 
Don't get me started about those stupid light bulbs.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!