ABC is in need of a lot of things, this I saw the first week on the job. So, many obvious things are wrong. Where to start? Well, our "service layer" (Note: we do not have clearly defined layers, and nobody seems to have known what to call bigger parts of the system, so if I could say "hey, we have a problem with our service layer" nobody would understand what I was talking about) has code in it that is very, very long-winded, some made up code:
dto.Part p = DAOFactory.getPartsDAO().getPartById(new Integer(1));
validatePart(p);
if(p.getInventoryID() != null) {
dto.Inventory inv = DAOFactory.getInventoryDAO().getInvetoryById(p.getInventoryID());
//Do more...
}
Hey, we use a factory! Great. But, this code does very little for being so verbose, and note that this sort of code could go on for hundreds of lines. Can we see spot any problems? I can, at least now, previously I really could not. I am not a very experienced J2EE developer, this is my first job with it fresh from university:
- I hate the fact that we have to explicitly do "DAO things". Why is this necessary? It is not, if we for example use hibernate a bit more correctly: ABC is currently not using any mappings between entities for example. Adding this, we could do away with fetching one object in a transaction and then accessing the collection mappings instead of all the "DAOing" as I call it.
(This DAO-ing also has severe performance penalties when done this way: it has a select n+1 problem. It can be solved by being "smart", but takes a lot of manual coding, for something that hibernate solves perfectly for us, practically for free. Logic is honestly more compact in a HQL query than in this sort of convoluted code. 20-lines of HQL did in some instances replace 100+ lines of Java for us, especially manually fetching objects, checking them instead of simply using a WHERE clause.) - We are fetching dtos from a database, by the looks of it. Is this really the intended use for DTO pattern? No (as far as I know, anyway), and it turns out these are not DTOs, firstly, and secondly, they should not be. Our "DTO layer" here is misnamed, and misguided. It is actually our domain model, although an almost completely anemic such[1]. We do in fact have another DTO layer, that is really a DTO layer, and which is needed and should be a DTO layer. This is used for sending data from webservices, and decoupling this from our domain model is on the contrary not a bad idea.
So we have a, more or less, procedural service layer with 95% of our business logic in it, and a domain model layer with data and only trivial behavior in it. It seems straightforward enough to just move behavior into our domain model, and be done with it. One slight problem here is that I have introduced alternative "DTOs" (entity beans, domain objects) that are properly mapped using hibernate, and adding _Mapped to their classname, inheriting from its non-collection-mapped hibernate class. For example we might have:
Part has a subclass PartMapped. PartMapped has collection mappings, Part does not.
It can also look like so: We have PartBase, PartMapped and Part. Part is the "original" DTO, with a foreign key in the form of a Integer. We don't really want this integer in our hibernate-collection-mapped variant, so we create a base class which does not have it, and extend that while adding the collections for hibernate to populate.
Modifying the domain model to fit hibernate is a bad idea, I realize that. We need to stop depending on hibernate. I have been looking at spring lately (which ABC of course does not use) and I think it could help with a lot of this. One reason for having alternate classes for collection-mapped entities was that I do not yet trust hibernate to actually save those back: we still always use the simple save() call for that on a entity with no collection mappings. Makes it very explicit what is happening. This is simply a matter of education though, I need to learn how hibernate works in this regard and, say, filtered collection.
This inheritance structure makes it not hard, far from, but a bit smelly to add behaviour to these objects. Mostly due to the structure being hibernate induced, but also because of the duplication arising from that: We would need two different implementations of a "
public Inventory getInventory()" for a Part and a PartMapped, one using straight accessors (hibernate needs objects to use the accessors for its collection proxies to work) and the other one using old-style DAO-ing to fetch the object. We have a lot of these methods to implement if we were to move them into the domain model proper.I assume the right course of action is to abstract away the database stuff such as
Integer ID; in all domain objects, and let "something" (not specifically hibernate) manage persistence for us, and then start modeling the domain properly. And then document it all and educate everyone that, now, ABC is much more agile and can withstand changes easily.It cannot today, that is for sure. I draw the conclusion that earlier developers have been "fooled" by EJB patterns or something similar, and was unable to see anything wrong with our anemic domain model. Design patterns are used, but those are localized, and in general there "is no design". No explicit design anyway, there is one that can be sort-of inferred from looking at the code, but it is very vague and lots of boundaries in that are often broken by code. It can still server as a "new vision" for the overall design though.
