OOP & OOAD in OpenLMIS

[Paweł Albecki]

Hi all,

I took a look at areas in the OpenLMIS code that we can improve using OOAD approach.

I found following Technical Gaps:

1. Lots of Utility classes with procedural static methods. Many of util classes can be changed to proper object giving us all OOP benefits. See example: https://github.com/OpenLMIS/openlmis-cce/commit/b5223be19be9ae72dd6a7797044de282650b59c5.

2. Domain logic outside of domain layer. To suppress this, encapsulation should be used. Setters and getters on domain classes should be avoided and used only if it’s required/reasonable. See example of how Encapsulation can be implemented and what benefits it gives: https://review.openlmis.org/cru/FEOLMIS-2178

3 Using map for request parameters or request body in search endpoints:

- causes a lot of boilerplate code in service classes (https://github.com/OpenLMIS/openlmis-referencedata/blob/master/src/main/java/org/openlmis/referencedata/service/SupervisoryNodeService.java#L82),

- forces to create literal string for keys (it’s easy to introduce a bug).

There are two possible approach on how to handle it:

- use object: example implementation: https://review.openlmis.org/cru/FEOLMIS-2150,

- use wrapper object that will make use of Map methods like isEmpty() and implement boilerplate code in one place, leaving service class clean.

4. Validators are using domain objects. Data validation should always be done before domain object is created so we are sure it’s always in valid state. We can validate when constructing an object or use Dtos in Validator classes.

5. Dtos in domain object (https://groups.google.com/forum/#!topic/openlmis-dev/590SBoZcG50). Beyond what Josh said, we sometimes use Dtos in domain logic. One way to solve this would be to store needed data in domain object as snapshot.

Places where above issues are common:

- Requisition domain logic logic in RequisitionService, RequisitionHelper and LineItemFieldsCalculator classes (last two are not object oriented at all, just a collection of procedural static methods).

- RequisitionBuilder has no reason to be. We should be able to create new requisitions using Requisition object, without RequisitionBuilder and RequisitionService which call setter methods on Requisition object.

- Requisition domain object use OrderableDto, ApprovedProductDto and ProofOfDeliveryLineItemDto. First one is used for calculations and we should snapshot needed values. Second and Third one are used as a bag for data that would be stored in requisition or line item during initiation. Instead we should pass to constructor everything that is needed directly.

For now I only took deeper look at CCE and Requisition services. I will investigate other services as well and extand list of gaps and dark places.

What do you think about presented tech debts and possible solutions? Please feel free to share your experience where and how we break OOP in OpenLMIS.

Best Regards,

Paweł

[Nikodem Graczewski]

Hi Paweł,

I have one suggestion regarding Requisition object creation. As this is a quite robust object I believe we should lean toward using a factory to actually build objects of the Requisition class. Putting that responsibility inside a constructor can be a little too much.

Best regards,
Nikodem

Nikodem Graczewski

Software Developer

ngrac...@soldevelo.com

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to openlm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/b9335acb-c3b3-4945-a906-2cd53b184b67%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

[Paweł Albecki]

I think the most important is to construct an object in one place. I rushed too far with "We should be able to create new requisitions using Requisition object". I can agree that Requisition class is robust. It can be good to encapsulate creation of object when process is complex and it should be done in factory class so every class have singe responsibility. I can't say now how complex would it be to create requisition after we clean this object from dtos but after looking at initiate methods in requisitionService and requisition or newRequisition in builder, such factories (one for requisition, one for line item that first one would call) can be helpful.

--

Paweł Albecki
Software Developer
palb...@soldevelo.com

[Paweł Gesek]

A few responses:

Re 1. Not sure if this is the best example - string manipulations like this are generally the perfect examples for actually using util classes (doesn't StringUtils from Apache cover this method by the way?). I believe we have utils that deal with business logic like calculations - those should be the focus.

Re 2. You might be right about setters, they are in general always added for the benefit of framework like ORMs and testing purposes. How will w deal with those if we stop adding setters?

Re 3. Removing the boilerplate always sounds good. 

Re 4. I believe putting validations in the domain classes is more DDD driven, but I understand that Spring kind of 'promotes' it's validator patterns. However, since we are using an exception based approach anyway, I think putting the validations in the domain would be a better fit for us.

Re 5. I think it's mostly botched exporters, I don't think there are many places where we use dtos in business logic. However if such places exist, we should obviously fix. I know that in business logic we use dtos from other services (i.e. ProductDto in requisitions), which I think it's acceptable.

Do you have an estimate of how much effort fixing these issues would take in, lets say, requisitions only? 

Regards,

Paweł

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAAJzpfkkMCGwhrEpuj9YNmxdXW7O1eKvjzcuM0c1pjbr8LRRJw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Paweł Gesek
Technical Project Manager
pge...@soldevelo.com / +48 690 020 875

[Paweł Albecki]

About 1. It's possible that some external library cover this (although I didn't find such method in StringUtils) but it's not most important here. I used trivial example for a reason. Classes in OOP should have single responsibility, we don't want to have big *Utils (good "bad" example https://github.com/OpenLMIS/openlmis-requisition/blob/master/src/main/java/org/openlmis/requisition/utils/ReportUtils.java) that are doing everything and nothing. These can be split into small and nice classes. If utils are better or object are better is individual preference but I think we should be consistent here. I can't find pros for using "static" classes instead of object oriented classes. I look forward others opinion in this subject.

About 2. What ORM benefits do you see with setters? I didn't experienced any performance problem after removing setters in inventory items. Moreover, performance tests had a little better results after my change but this could have been a random thing. About tests, it's good that Paweł raise it. We should have constructors that deal fully with object creation. So why would we need setters? From my experience, setters are tricky in unit tests because when we call setter on object to check some values, reference in tested code is changed as well and test always pass. We should (almost) always create new object instead of change existing. Since we use data builder pattern, it's really easy and clean.

About 4. Going towards DDD, we may start thinking about multiple layers of validation. Obviously, we can't check in domain layer if object is unique (this should be done on database layer) nor check by UUID/code if some resource exists in other service (application layer). But logic validation (like correctness of calculations, not null checks etc.) should be in domain. If we need some configuration setting for validations (like here https://github.com/OpenLMIS/openlmis-requisition/blob/master/src/main/java/org/openlmis/requisition/validate/RequisitionValidator.java#L230) we could use Policy building block and have it in Entity object with @Transient annotation. 

I would estimate this on 3 to 4 days. However, in my opinion we should take the same approach like with data builder pattern. Start require some rules during code review and look how fast our codebase will improve.

[Mateusz Kwiatkowski]

Hi Paweł,

I agree with most of those bullet points, especially with #3 and #4. 

I want to mention that on the UI side we are moving towards more objective code. Here is a mention in UI Architecture v7. Especially #2 and #4 are things that we are lacking. Introducing those in JS can be little tricky and we still need to introduce some patterns for that. Thing that is bothering me lately is that we have single constructor with all parameters which means, that in some cases we are passing a lot of null/undefined values. Maybe some factory/builder pattern would help resolving that issue?

Regards,

Mateusz

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAAJzpfn0B%3DCbUNCUz_u8cyWKz_2im5bXvfbTeJCt_qGWMGc%2BfQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Paweł Gesek]

Re 1. I would stick with the util approach for code that is not connected with OpenLMIS business logic, i.e. string manipulation. The general benefit is that you don't need to create an instance for these utils in each class that uses them (nor have them defined as a Spring bean). You can also static import these methods (what we do with Mockito and asserts constantly). But in general, I don't think we will have many methods like this in the OpenLMIS codebase - plus such methods are candidates for the service-util library, since they are independent from the domain.

Re.2 Some frameworks fall apart if you don't have setters/getters. If none of our frameworks have this issue, that's good. Regarding test code, not having getters/setters, will require us to put more effort into object creation. For example when testing approvals, you will need the requisition to go through all the previous steps as a pre-requisite. The builder patterns we intend to introduce might alleviate these issues.

Re.4. Yes, database checks are a separate issue from this.

Regarding massive constructors - we should be careful, as this usually signals not enough encapsulation. Why does an object need that many params, can't they be gathered into classes? Builders will hide the problem, but not necessarily solve it. 

Regards,

Paweł

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAHq-FDOaVUCaGL_KfPgwSDRnxTEpNc_ezSrnwZaTyk77pWC4kg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Paweł Gesek
Technical Project Manager
pge...@soldevelo.com / +48 690 020 875

[Paweł Albecki]

From my understanding of Factories, they are helpful just when we have many objects in Aggregate and we want to call one method to construct e.g. requisition including line items and every other object that requisition consist of. Additionally validations, calculations and other parts of complex object creation can be done there.

Regarding object creation in JS, I don't see how Factories could help with issue Mateusz is talking about. Name every method in factory differently and emulate function overloading? We don't want to have builders on backend (I think everyone would agree that factories or constructors should be used) but maybe this would be good option for UI?

-Paweł

[Łukasz Lewczyński]

I have one question about how domain object should be created. I noticed that in the stock management service a lot of domain classes have builder (@Builder annotation). Should we remove builders from the domain classes and use only importer method or constructor with all fields ?

- Lukasz

Łukasz Lewczyński
Software Developer
llewc...@soldevelo.com

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAAJzpfn-G0V47mJeTsFdeBGxxj7bCyHJuXEHgu9vgDDiXrVgtA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Paweł Albecki]

Yes, I believe so. It was raised in one review that stock management design should be more coherent with other services. I think the list of things to change should be created. I planned to work on this so if you see any other issue please feel free to share it here. 

-Paweł

[Nikodem Graczewski]

Wouldn't passing a whole object to the constructor to extract only couple of fields from it lead to a tighter coupling of those classes? This is something we should avoid when following DDD.

Best regards,
Nikodem

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/b9335acb-c3b3-4945-a906-2cd53b184b67%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--

Paweł Albecki
Software Developer
palb...@soldevelo.com

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAAJzpfkkMCGwhrEpuj9YNmxdXW7O1eKvjzcuM0c1pjbr8LRRJw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Paweł Gesek
Technical Project Manager
pge...@soldevelo.com / +48 690 020 875

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--

Paweł Albecki
Software Developer
palb...@soldevelo.com

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAAJzpfn0B%3DCbUNCUz_u8cyWKz_2im5bXvfbTeJCt_qGWMGc%2BfQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--

You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAHq-FDOaVUCaGL_KfPgwSDRnxTEpNc_ezSrnwZaTyk77pWC4kg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Paweł Gesek
Technical Project Manager
pge...@soldevelo.com / +48 690 020 875

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--

Paweł Albecki
Software Developer
palb...@soldevelo.com

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/CAAJzpfn-G0V47mJeTsFdeBGxxj7bCyHJuXEHgu9vgDDiXrVgtA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--

Paweł Albecki
Software Developer
palb...@soldevelo.com

[Josh Zamor]

Wow, just catching up with all that's happened in this thread.  I'm excited the discussion is moving forward.  Thanks all and thanks Pawel for kicking it off.

From a quick scan, a couple things jump out at me:

1. Validation and creation of an object go together.  Put another way, you need to consider what the plan for an object is going to be when you start.  There are two general approaches:
1. Allow "invalid" objects to be created.  After you have the object, you either need to ask it "are you valid" or you need to have some other object capable of validating it.  In my experience, this is usually the most useful approach to "complex" objects - domain objects / aggregate roots.
2. Don't allow "invalid" objects from even being created.  This is pretty self explanatory, and it's simple to reason about:  if you have an object, it's valid, as there's no other way to get that object.  In my experience this is nice to try, but for anything but the most basic object, you run up against a wall pretty quickly.  I'd suggest keeping these to only the most simple value types, and even then, serialization/de-serialization might foil your plans.
3. (okay I said 2), Don't forget that you can use both approaches.  I'd recommend favoring the first option more often than not.  And remember that "validations" such as uniqueness constraints won't even fit into Java based checking very well.  It should be encapsulated by the domain repository, and the actual check is an implementation detail of the database (so have an integration test for it on the repository).  This SO answer is related, and one that I think is well formed.
2. I agree the example about OOP is confusing.  Perhaps it's just the example, but I wouldn't discourage using simple static methods and utility classes for grouping them for simple functional operations:  make this a that.  I don't think that's overall what you're saying, so perhaps a different example is in order.  Perhaps turning one part of our the LineItemCalculator into a more OOP design would clarify your message.
3. I wouldn't encourage us to prioritizing changing built Services such as Requisition.  It's great to fix tech-debt, however we can put our ideas to the test in the new objects we're designing and building today.  I don't want to discourage fixing tech-debt, however I think this thread shows we're still diving into the depths of OOD and even DDD.  It'll be easier to try these ideas in new code than try to refactor some of the existing non-OOP code we have.  And that'll give us room to learn.  Speaking of learning, a few books which I like on this subject are:  Clean Code, POJOs in Action (a bit aged), Patterns, Principles and Practices of Domain Driven Design.
   

One topic I don't see in this list, which is curious, is that of identity.  The simplest way to talk about identity is to ask if all our Objects implement Object.equals(Object) and Object.hashCode() in ways that make sense?  Thinking through Identity seems overly basic, but it's more often done wrong and it lies at the heart of how we conceptualize an object.

I'm excited that we're discussing this.  Our agenda for tomorrow's tech committee might already be full, but I think this would be a good topic to also discuss on the phone in the near future in addition to pursuing the discussion here.

Best,
Josh

[Paweł Albecki]

Thanks for your input, Josh. Patterns, Principles and Practices of Domain Driven Design looks interesting (and really wide). It can be interesting reading.
To the points:

1. I don't see much value of first approach in our system. We don't serialize domain entities, why not ensure that every domain object is valid and don't have to worry about isValid method?

2 and 3. I wonder if turning one part into a more OOP would give us good example, the real value can be seen in overall redesign. Some basic example about logic encapsulation can bee seen here where all Inventory logic is inside of InventoryItem class instead of in controller/application service/static methods.

4. Identity. The question is what make sense. If we have simple value object like UpdateDetails we want to check equality of both updaterId and updatedDate. But what check in entities? Should we check only by id or by almost all fields and exclude fields like statusChanges?

Best regards,

Paweł

--

You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/02bd93ba-8825-4c51-82f4-afc5d3bc9bf9%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Paweł Albecki
Software Developer
palb...@soldevelo.com

[Paweł Albecki]

Hi all,

I would like to continue the discussion and talk about some DDD building blocks that we can start using right away.

1. Value Objects: 
• Objects that are immutable (final keyword on class and fields) and equality is based on state, not identity. 
• We can make use of it and extract into VO some fields with its logic from large entities/classes. 
• The best candidates for VO are fields that have similar meaning across different entities, like code, amount or dispensable. 
• Fields that are strongly linked or can change only together like latitude and longitude (location), startDate and endDate (date range), updaterId and updatedDate (update details), receivedBy and receivedDate (receive details) are also very good examples.
  

I found above examples in Referencedata and Fulfillment. We already have few value classes like Code, Dispensable or UpdateDetails but we use them only in few places so we can use them more and create additional classes e.g. for DateRange.

What do you think about Value Objects and my suggestions? I am pretty sure there is more places with bunch of fields or simple data structures that can form Value Object so feel free to share your thoughts.

Regards,

Paweł

[Łukasz Lewczyński]

I think createdDate and createdBy could also be extract to VO as CreationDetails. In my opinion this is very good suggestion but there is one problem that the only way to achieve this is to add related comments in reviews. Because each service is independent we could not create a share library (there is a topic about this). Also I don't think it is a good idea to create ticket for only those changes basically because each change should add/fix something in the service and for me this is more like code refactor which could be done together with other changes.

Łukasz Lewczyński
Software Developer
llewc...@soldevelo.com

SolDevelo Sp. z o.o. [LLC] / www.soldevelo.com
Al. Zwycięstwa 96/98, 81-451, Gdynia, Poland
Phone: +48 58 782 45 40 / Fax: +48 58 782 45 41

--

You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to openlm...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/a6305c72-13a8-4e36-afb0-596b9083fd8f%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Paweł Albecki]

A way to achieve these improvements is something good to discuss and final decision should be made on Tech Committee.

On Wednesday, November 22, 2017 at 8:08:07 PM UTC+1, Paweł Albecki wrote:

[Paweł Albecki]

It's time to continue with next building block that is worth to introduce in OpenLMIS.

Domain Services:

1. Can be used for creating one object based on another or transform one Aggregate to another.
   
2. OLMIS examples: converting requisition into an order, converting order into proof of delivery or shipment.
   
3. The question is: can Domain Service be annotated as Spring @Service so that would allow to inject external dependencies (like reference data services)? On example of requisition convert to order: to create order we currently need many injections (for products, programs, facilities, etc.) to build order but I believe this is wrong, why do we need to get e.g. whole facility from reference data, can't we send just facility id to fulfillment? On the other hand if Domain Service need e.g. some data from repository, we can provide such data directly in method params or provide Repository (which interface should be also in Domain) injected in Application Service.

Do you have an opinion in this topic? 

Regards, Paweł

To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/a6305c72-13a8-4e36-afb0-596b9083fd8f%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Paweł Albecki]

I would like to raise another important block which is Aggregate: graph of objects bound together by aggregate root. By definition the aggregate root guarantees the consistency of changes being made within the aggregate by forbidding external objects from holding references to its members. One of aggregate in our system is Requisition which should control business rules and consistency of objects that requisition consists of. That means that we should never manipulate e.g. line items on their own and instead read requisition from repository with its line items as a whole and add/remove/modify line items using aggregate root (requisition object). Line items make sense only in light of the requisition that enforce rules e.g. only one line item can exist for some product in one requisition.

Currently there is a getter for requisition line items that allow manipulating them directly instead of by requisition object which is done in requisition application service class. I think we should avoid such things in future and refactor requisition code when there will be some work to do in Requisition component. There can be need to view requisition line items outside of aggregate and this can be achieved by providing a view for line items. I recently implemented this in Shipment and ShipmentDraft (Fulfillment component) by returning copy of every line item wrapped in unmodifiable list. You can see this here: https://github.com/OpenLMIS/openlmis-fulfillment/blob/master/src/main/java/org/openlmis/fulfillment/domain/ShipmentDraft.java. What do you think about this solution?

In Aggregate we can have entities like requisition, requisition line item, some value objects. We should also have one repository interface for every aggregate root. But I wonder if we should hold interface of repository in the same package with entities?   

Best regards,

Paweł

On Wednesday, November 22, 2017 at 8:08:07 PM UTC+1, Paweł Albecki wrote: