DTOs in Domain objects

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

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.

···

On Saturday, September 16, 2017 at 1:44:37 AM UTC+2, Josh Zamor wrote:

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

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

···

On Sunday, September 17, 2017 at 11:18:55 PM UTC-7, Nikodem Graczewski wrote:

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.

On Saturday, September 16, 2017 at 1:44:37 AM UTC+2, Josh Zamor wrote:

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

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 | chongsun.ahn@villagereach.org

Software Development Engineer

Village****Reach* ** 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

···

On Sep 18, 2017, at 12:51 AM, Josh Zamor josh.zamor@villagereach.org wrote:

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

On Sunday, September 17, 2017 at 11:18:55 PM UTC-7, Nikodem Graczewski wrote:

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.

On Saturday, September 16, 2017 at 1:44:37 AM UTC+2, Josh Zamor wrote:

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

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/6f132d2b-a3c4-444f-8773-6e83f1ea3743%40googlegroups.com.

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

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ł


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

···

On Sat, Sep 16, 2017 at 1:44 AM, Josh Zamor josh.zamor@villagereach.org wrote:

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

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

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

Paweł Albecki

    Software Developer

     palbecki@soldevelo.com

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

···

On Monday, September 18, 2017 at 9:51:27 AM UTC+2, Josh Zamor wrote:

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

On Sunday, September 17, 2017 at 11:18:55 PM UTC-7, Nikodem Graczewski wrote:

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.

On Saturday, September 16, 2017 at 1:44:37 AM UTC+2, Josh Zamor wrote:

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