DTOs in Domain objects

[Josh Zamor]

Hi all,

I happened to notice in one corner of the code-base that we've started to change/break some patterns that we don't want to change.

The first is the use of DTOs in Domain classes.  e.g. https://github.com/OpenLMIS/openlmis-referencedata/blob/master/src/main/java/org/openlmis/referencedata/domain/Orderable.java#L20

Domain classes should never have references to DTOs.  Domain classes should be isolated from outside concerns:  how serialization works, how db access works, etc...

There are other (7) domain classes which now suddenly hold references to DTOs that should be removed.  I wonder where else we're doing this and why we started to do this?

Best,
Josh

[Nikodem Graczewski]

I'm not sure why this was added, but the one thing I would suggest is creating some tickets for those tech debts and fixing them in the future.

[Josh Zamor]

Thanks for the suggestion Nikodem.

Perhaps there's something more we can learn from this?

Git tells me that this was added from a Malawi ticket, which ended up adding code by pull request:

• the code review was done in GitHub, not review.openlmis.org
• the size of the code review was over 2000 lines across 100 files
• there were 4 reviewers added (including myself)
• the github review wasn't actually passed, yet it was merged.

This is not the first time I've seen code reviews that were:

• too large (> 500 LOC).  With too many lines of codes, it can't be reviewed well.
  
• had too many reviewers (> 1).  With too many it's not clear who's responsible.
  

We've put time into having code-review guidelines and a checklist.  I think we should revisit these and make a new effort here, especially to reduce the size of code reviews and the number of people whom are responsible for each one.

Nikodem could you pull together the developers there in the office(s) and help the team re-focus on better code review practices?  Perhaps the team has some further ideas on how to improve.

Best,
Josh

[Chongsun Ahn]

Hey everyone,

Just wanted to add that I think re-focusing on better code review practices is important for us.

I am about to revert some changes that were made from a Malawi pull request (https://github.com/OpenLMIS/openlmis-referencedata/pull/22), which had annotated that the RightAssignmentRepository was being audited by Javers. This should not have happened, as right assignments are not part of the domain and do not have business logic, but is a data structure used for performance. Therefore, it does not make sense to be auditing it and having an audit trail for it.

I suspect that enabling auditing for the RightAssignmentRepository is what is causing startup times to have taken an hour plus, as the AuditLogInitializer (which runs at the startup of Reference Data Service) is creating an initial snapshot for each row in the right_assignments table. For Malawi, this is about 130,000 records. I created a similar scenario on my local machine, adding about 200,000 records into the right_assignments table, and then letting the AuditLogInitializer run. It was running for a while, average about 2.5-3 minutes for each 10,000 snapshots (1 snapshot per 1 row in right_assignments). Extrapolated, creating all snapshots for right-assignments would take about 45-60 minutes. When I removed the code that enabled auditing of right assignments, startup took 8 minutes total, half of which was the AuditLogInitializer.

Even though this pull request was reviewed, it had:

• about 100 files changed
• over 2000 lines changed

This pull request was just too large for any person to review in-depth, which is probably why this issue was missed.

Shalom,
Chongsun
​
-- ​
There are 10 kinds of people in this world; those who understand binary, and those who don’t.

Chongsun Ahn | chongs...@villagereach.org

Software Development Engineer

 

VillageReach Starting at the Last Mile

2900 Eastlake Ave. E, Suite 230,  Seattle, WA 98102, USA

DIRECT: 1.206.512.1536   CELL: 1.206.910.0973   FAX: 1.206.860.6972

SKYPE: chongsun.ahn.vr

www.villagereach.org

Connect on Facebook, Twitter and our Blog

-- 
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/6f132d2b-a3c4-444f-8773-6e83f1ea3743%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Paweł Albecki]

Hi,

About review quality, it would be good if one person is always assigned to review. I do not think it matters how many people are requested to review but who is assignee. The same person should be assigned to jira ticket and merge changes.

It could be helpful if source code in reviews is annotated, especially when it is about fixing a bug or provide new functionality.

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/4a11d0a1-e0d6-43f5-a253-be38fb2bcacc%40googlegroups.com.

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

--

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

[Nikodem Graczewski]

Hi,

Josh, since we had a talk about code review at the Tech Committee Call today with the most of the core developers present should I still pull them together for a meeting? I think all the points were mentioned during the meeting and I doubt I would able to add anything more.

Regards,
Nikodem