I have a review comment (over my detail-design/code) like this:-
Lack of encapsulation 1. The nature of object oriented design embraces encapsulation http://dictionary.reference.com/search?q=encapsulation. The ability to provide users with a well-defined interface to a set of functions in a way which hides their internal workings. In object-oriented programming, the technique of keeping together data structures and the methods (procedures) which act on them. This concept is not being used with regards to the Batch Object. 2. I had to read through 3 different classes to understand Batch class. (ITPXMLParser.java, BatchParser.java, Configuration.java). Anything related to a batch 'must' be encapsulated insde the Batch class only. Why are you using BatchParser (I explained this and he is ok) and why should BatchParser class know anything about xml elements of a batch xml ?
The Requirement: A 'Batch' object represents a physical tar file. It is bundled with a set of image files (TIFFs) and an xml file having details about each image file added to this batch/tar. There is size limit to each batch (tar file). Every time an image file is added to the batch/tar, - verify if that image can be added to this batch to make sure that the total size would not exceed the size limit of a batch - (re)generate an xml file along with the details of the added image (this is primarily for recovery - so that if an application ended abruptly, the next time the application is started, we identify this xml and prepare the batch object from this xml)
I read through couple of OOAD material/books, I am only able to theoretically understand it. Thought I would take suggessions on how to apply that theory in this situation.
Any anlysis and disection of the above code is welcome.
Would like to know why and how 'encapsulation' is broken here. If it is broken, pls suggest/direct how to fix this ?
I don't really see it. One possible objection is that the knowledge about the XML format isn't held in just one class, but spread across two (Batch knows how to write it, and BatchParser how to read it.) But personally, I'd be inclined to take that knowledge out of Batch and put it someplace else, anyway, which I'm afraid would make the problem worse for your reviewer.
I had to read through 3 different classes to understand Batch class.
This is pretty bogus. Your ITPXMLParser and BatchParser are nicely encapsulated and hidden from Batch users. You don't have to read them at all to "understand Batch". BatchParser is not even referenced from Batch. If your reviewer only wants to read one file to understand a system he might prefer COBOL.
We don't emphasize encapsulation just for its own sake. One goal of encapsulation is to protect one class from changes in another. When one class knows too much about another changes can ripple through your whole system. Do you and your reviewer agree on what kind of change you want to protect yourself from? For example, if there is mandate to switch from XML storage to database or CSV or some other format you're in trouble. All the classes talk about XML at length. If you judge that is unlikely then you can accept knowing about XML and go on. If some tag in the XML has to change you have to modify the parser and the writer, two places. As Ernest said, maybe that's a bad sign. If you consider these three classes as one tightly coupled component maybe it's not.
I don't know what kind of risk it might be to your career to ask your reviewer "What bad things can you see happening because of this problem?" If he has a really strong answer you learn something, if not maybe he does.
A good question is never answered. It is not a bolt to be tightened into place but a seed to be planted and to bear more seed toward the hope of greening the landscape of the idea. John Ciardi
Joined: Mar 04, 2005
Thank you for the clarification.
I replied for this review comment that: "Regarding strictly adhering to 'encapsulation' - it may result in change in my detail design (of removing utility classes and other xml related handler classes) to make the Batch fully aware of itself w.r.to code readability. This means that we are talking of additional time. Note that this would apply to more cases (TransmRequest, Email and Fax transm classes ) where xml handlers were used. Pls suggest and I would go that way."
Yet to hear back on this one. (I presume no action point for me on this one for now).
Note: My reviewer (also the high level designer) is a customer of my detailed design and code. [ October 31, 2005: Message edited by: Ranga Kalidindi ]