0
\$\begingroup\$

I have a set of rules as follows:

Product One:

  • If either part is acceptable, update both parts

Product Two:

  • If either part is acceptable, and one age is > 55, update both parts

  • If either part is acceptable, and neither age is > 55, then update whichever part needs updating (or both)

I know it's not the most nested if statements you'll see, but it feels like I'm repeating code unnecessary and there should be a simpler version of what I've attempted.

The Code:

 if (this.isAcceptablePartOne || this.isAcceptablePartTwo) {
    if (this.productName.equals(PRODUCT_ONE)) {
        updatePartOne();
        updatePartTwo();
    }
    if (this.productName.equals(PRODUCT_TWO)) {
        if (Math.max(this.ageLifeOne, this.ageLifeTwo) > 55) {
            updatePartOne();
            updatePartTwo();
        } else {
            if (this.isAcceptablePartOne) {
                updatePartOne();
            }
            if (this.isAcceptablePartTwo) {
                updatePartTwo();
            }
        }
     }
 }
\$\endgroup\$

closed as off-topic by 200_success, Toby Speight, IEatBagels, Graipher, Sᴀᴍ Onᴇᴌᴀ Jan 30 at 18:55

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success, Toby Speight, IEatBagels, Graipher, Sᴀᴍ Onᴇᴌᴀ
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 5
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. Also, there isn't enough context to review the code. \$\endgroup\$ – Zeta Jan 29 at 9:31
  • \$\begingroup\$ @Zeta fair enough on the title. But I'm not sure how I could provide more information regarding what I was trying to do with the code. Depending on specific scenarios, I need to update part one, part two or both. \$\endgroup\$ – achAmháin Jan 29 at 9:36
  • 1
    \$\begingroup\$ If it is possible could you show us your Product-class. I think it could be too big and should be dismembered into several small ones.. \$\endgroup\$ – Roman Jan 29 at 12:24
  • 3
    \$\begingroup\$ Please show more context. What method does this code appear in? What class is this? I suspect that you may be able to improve the code further with a better design altogether. \$\endgroup\$ – 200_success Jan 29 at 15:27
2
\$\begingroup\$

From the requirements

I have a set of rules as follows:

Product One:

If either part is acceptable, update both parts

Product Two:

If either part is acceptable, and one age is > 55, update both parts

If either part is acceptable, and neither age is > 55, then update whichever part needs updating (or both)

Follows the next, almost literal, straightforward implementation:

Straightforward implementation

So, you could implement this using two Checkers, a Product1Checker and a Product2Checker.

if (this.productName.equals(PRODUCT_ONE))
{
    new Product1Checker().check(this);
}
else if (this.productName.equals(PRODUCT_TWO))
{
    new Product2Checker().check(this);
}

Like this:

class Product1Checker 
{

    public check (Product product)
    {
         if (product.isAcceptablePartOne || product.isAcceptablePartTwo) 
         {
               product.updatePartOne();
               product.updatePartTwo();
         }
    }
} 

Alternative approach

You could have the Checkers implement a ProductChecker interface. Also, each ProductChecker can have a boolean method indicating if this checker applies to a product. Then you can just add all the checkers to a Set, check if they are applicable, and if so, run them on the product

interface ProductChecker
{
    boolean check     (Product p);
    boolean appliesTo (Product p);
}

Set<ProductChecker> checkers = new HashSet<ProductChecker>();
checkers.add(new Product1Checker());
checkers.add(new Product2Checker());

for (ProductChecker checker: checkers)
{
      if (checker.appliesTo(this))
      {
          checker.check(this));
      }
}
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.