Too many methods and requisitions
Paweł Gesek
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
pge...@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
Brandon Bowersox-Johnson
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
--
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/f65f1e09-6a57-7cdc-51ae-d74b4c461a5d%40soldevelo.com.
For more options, visit https://groups.google.com/d/optout.
Chongsun Ahn
Shalom,
Chongsun
--
There are 10 kinds of people in this world; those who understand binary, and those who don’t.
On Jan 17, 2017, at 11:11 AM, Brandon Bowersox-Johnson <brandon.bowe...@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: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.-BrandonFrom: <openlm...@googlegroups.com> on behalf of Paweł Gesek <pge...@soldevelo.com>
Date: Monday, January 16, 2017 at 2:59 AM
To: OpenLMIS Dev <openlm...@googlegroups.com>
Subject: [openlmis-dev] Too many methods and requisitionsHello,
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
pge...@soldevelo.com / +48 690 020 875SolDevelo 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...@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/f65f1e09-6a57-7cdc-51ae-d74b4c461a5d%40soldevelo.com.
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...@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/C687143F-A379-4BDD-84CD-3BDFFF353C13%40villagereach.org.