Too many methods and requisitions

Hello,

  recently a few quite not so pretty classes were added as part of getting around the PMD method limit, which we started violating in requisitions and requisition line items.

  First of all, the method number is pretty strict and unforgiving for classes that have multiple fields. We should make a call however - do we increase the limit, remove it, or start figuring ways to conform to it. The worse thing we can do is to keep ignoring it everywhere. Disabling it in tests would also be a bonus.

  That being said, I think for now it is fine to ignore it - until we figure out a way to split these classes, either using composition or some other technique (and get rid of procedular code that gets around this). If we do not really care for splitting these classes, we should probably consider changing this limit.

Regards,

Paweł

···


Paweł Gesek

    Software Developer / Team Leader

      / +48 690 020 875

SolDevelo Sp. z o. o. [LLC]

     Office:  +48 58 782 45 40 / Fax:  +48 58 782 45 41  Al. Zwycięstwa 96/98  81-451, Gdynia

     [http://www.soldevelo.com](http://www.SolDevelo.com)

               Place of registration: Regional Court for the City of Gdansk            KRS: 0000332728, TAX ID: PL5862240331, REGON: 220828585,            Share capital: 60,000.00 PLN

pgesek@soldevelo.com

I believe that it’s sometimes appropriate to exceed the PMD method limit of 10 methods per class. When there is good reason to do so, you can just add the SuppressWarnings annotation onto the class, like we do in many places in the OpenLMIS codebase:

@SuppressWarnings({“PMD.TooManyMethods”})

For example, a grep reveals 5 places we do this in the ReferenceData service v3. I believe it’s fine to do, and it can help to add an explanation of why into the commit message too.

The default limit is 10 methods, and sometimes there is good reason to exceed that:

http://pmd.sourceforge.net/pmd-4.3.0/rules/codesize.html#TooManyMethods

My feeling is that separating code out of a class (when it really deserves to be in the same class) can be just as confusing or troublesome as it can be to have a lengthy class. For example, I heard lately that some of the field calculations in the Requisition service have been moved into “Helper” classes. Chongsun mentioned that it would be more appropriate to keep all that business logic inside the Domain classes, not using any Helpers.

In summary, I’m suggesting we leave the default limit at 10 for now. The limit causes developers to ask themselves, “should I really make this class so big?” And if they decide that it is appropriate to make the class big, then they should use the SuppressWarnings annotation.

-Brandon

···

From: openlmis-dev@googlegroups.com on behalf of Paweł Gesek pgesek@soldevelo.com

Date: Monday, January 16, 2017 at 2:59 AM

To: OpenLMIS Dev openlmis-dev@googlegroups.com

Subject: [openlmis-dev] Too many methods and requisitions

Hello,

recently a few quite not so pretty classes were added as part of getting around the PMD method limit, which we started violating in requisitions and requisition line items.

First of all, the method number is pretty strict and unforgiving for classes that have multiple fields. We should make a call however - do we increase the limit, remove it, or start figuring ways to conform to it. The worse thing we can do is to keep ignoring it everywhere. Disabling it in tests would also be a bonus.

That being said, I think for now it is fine to ignore it - until we figure out a way to split these classes, either using composition or some other technique (and get rid of procedular code that gets around this). If we do not really care for splitting these classes, we should probably consider changing this limit.

Regards,

Paweł

Paweł Gesek

Software Developer / Team Leader

pgesek@soldevelo.com / +48 690 020 875

SolDevelo Sp. z o. o. [LLC]

Office: +48 58 782 45 40 / Fax: +48 58 782 45 41 Al. Zwycięstwa 96/98 81-451, Gdynia

http://www.soldevelo.com

Place of registration: Regional Court for the City of Gdansk KRS: 0000332728, TAX ID: PL5862240331, REGON: 220828585, Share capital: 60,000.00 PLN

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/f65f1e09-6a57-7cdc-51ae-d74b4c461a5d%40soldevelo.com
.

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

I agree with Brandon. Additionally, we do not want to sacrifice object-oriented design and programming in order to satisfy the PMD method limit, which is what I’ve been seeing (the helper classes being created simply contain procedural subroutines).

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<mailto:chongsun.ahn@villagereach.org>
Software Development Engineer

VillageReach<http://villagereach.org/> 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<http://www.villagereach.org>
Connect on Facebook<https://www.facebook.com/pages/VillageReach/103205113922>, Twitter<https://twitter.com/VillageReach> and our Blog<http://villagereach.org/see-our-blog/thoughts-from-the-last-mile/>

···

On Jan 17, 2017, at 11:11 AM, Brandon Bowersox-Johnson <brandon.bowersox-johnson@villagereach.org<mailto:brandon.bowersox-johnson@villagereach.org>> wrote:

I believe that it’s sometimes appropriate to exceed the PMD method limit of 10 methods per class. When there is good reason to do so, you can just add the SuppressWarnings annotation onto the class, like we do in many places in the OpenLMIS codebase:

@SuppressWarnings({"PMD.TooManyMethods"})

For example, a grep reveals 5 places we do this in the ReferenceData service v3. I believe it’s fine to do, and it can help to add an explanation of why into the commit message too.

The default limit is 10 methods, and sometimes there is good reason to exceed that:
http://pmd.sourceforge.net/pmd-4.3.0/rules/codesize.html#TooManyMethods

My feeling is that separating code out of a class (when it really deserves to be in the same class) can be just as confusing or troublesome as it can be to have a lengthy class. For example, I heard lately that some of the field calculations in the Requisition service have been moved into “Helper” classes. Chongsun mentioned that it would be more appropriate to keep all that business logic inside the Domain classes, not using any Helpers.

In summary, I’m suggesting we leave the default limit at 10 for now. The limit causes developers to ask themselves, “should I really make this class so big?” And if they decide that it is appropriate to make the class big, then they should use the SuppressWarnings annotation.

-Brandon

From: <openlmis-dev@googlegroups.com<mailto:openlmis-dev@googlegroups.com>> on behalf of Paweł Gesek <pgesek@soldevelo.com<mailto:pgesek@soldevelo.com>>
Date: Monday, January 16, 2017 at 2:59 AM
To: OpenLMIS Dev <openlmis-dev@googlegroups.com<mailto:openlmis-dev@googlegroups.com>>
Subject: [openlmis-dev] Too many methods and requisitions

Hello,

recently a few quite not so pretty classes were added as part of getting around the PMD method limit, which we started violating in requisitions and requisition line items.

First of all, the method number is pretty strict and unforgiving for classes that have multiple fields. We should make a call however - do we increase the limit, remove it, or start figuring ways to conform to it. The worse thing we can do is to keep ignoring it everywhere. Disabling it in tests would also be a bonus.

That being said, I think for now it is fine to ignore it - until we figure out a way to split these classes, either using composition or some other technique (and get rid of procedular code that gets around this). If we do not really care for splitting these classes, we should probably consider changing this limit.
Regards,
Paweł
--

<image001.png><http://soldevelo.com/>

Paweł Gesek
Software Developer / Team Leader
pgesek@soldevelo.com<mailto:pgesek@soldevelo.com> / +48 690 020 875

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

Place of registration: Regional Court for the City of Gdansk KRS: 0000332728, TAX ID: PL5862240331, REGON: 220828585, Share capital: 60,000.00 PLN

--
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<mailto:openlmis-dev+unsubscribe@googlegroups.com>.
To post to this group, send email to openlmis-dev@googlegroups.com<mailto:openlmis-dev@googlegroups.com>.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/f65f1e09-6a57-7cdc-51ae-d74b4c461a5d%40soldevelo.com<https://groups.google.com/d/msgid/openlmis-dev/f65f1e09-6a57-7cdc-51ae-d74b4c461a5d%40soldevelo.com?utm_medium=email&utm_source=footer>.
For more options, visit https://groups.google.com/d/optout.

--
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<mailto:openlmis-dev+unsubscribe@googlegroups.com>.
To post to this group, send email to openlmis-dev@googlegroups.com<mailto:openlmis-dev@googlegroups.com>.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/C687143F-A379-4BDD-84CD-3BDFFF353C13%40villagereach.org<https://groups.google.com/d/msgid/openlmis-dev/C687143F-A379-4BDD-84CD-3BDFFF353C13%40villagereach.org?utm_medium=email&utm_source=footer>.
For more options, visit https://groups.google.com/d/optout.