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.
···
On Fri, Nov 24, 2017 at 9:26 AM, Łukasz Lewczyński llewczynski@soldevelo.com wrote:
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 ?
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
–
Łukasz Lewczyński
Software Developer
llewczynski@soldevelo.com
On Fri, Nov 24, 2017 at 2:23 AM, Paweł Albecki palbecki@soldevelo.com wrote:
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ł
**
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 openlmis-dev@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.
On Thu, Nov 23, 2017 at 3:42 PM, Paweł Gesek pgesek@soldevelo.com wrote:
**
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
palbecki@soldevelo.com
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ł
On Thu, Nov 23, 2017 at 12:55 PM, Mateusz Kwiatkowski mkwiatkowski@soldevelo.com wrote:
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
**
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 openlmis-dev@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
pgesek@soldevelo.com / +48 690 020 875
On Thu, Nov 23, 2017 at 11:30 AM, Paweł Albecki palbecki@soldevelo.com wrote:
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.
**
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 openlmis-dev@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.
On Thu, Nov 23, 2017 at 10:27 AM, Paweł Gesek pgesek@soldevelo.com wrote:
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ł
**
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
palbecki@soldevelo.com
On Thu, Nov 23, 2017 at 12:38 AM, Paweł Albecki palbecki@soldevelo.com wrote:
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.
**
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 openlmis-dev@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
pgesek@soldevelo.com / +48 690 020 875
–
On Wed, Nov 22, 2017 at 10:33 PM, Nikodem Graczewski ngraczewski@soldevelo.com wrote:
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
**
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
palbecki@soldevelo.com
Nikodem Graczewski
Software Developer
ngraczewski@soldevelo.com
On Wed, Nov 22, 2017 at 8:08 PM, Paweł Albecki palbecki@soldevelo.com wrote:
Hi all,
I took a look at areas in the OpenLMIS code that we can improve using OOAD approach.
I found following Technical Gaps:
- 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.
- 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:
- 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 wrapper object that will make use of Map methods like isEmpty() and implement boilerplate code in one place, leaving service class clean.
- 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.
- 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ł
**
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 openlmis-dev@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.
Paweł Albecki
Software Developer
palbecki@soldevelo.com
