CODEWITHSUNDEEP
javarefactoring
Kumar
Kumar

Reputation: 31

void method refactoring with multiple checks

I have this method:

public void validate(param1, pararm2, param3 ...) {
    if(check1)
        add error
        return
    if check2
        add error
        return
    getDbObject
        check3
        exception
            return

There is another use case where along with the above checks, if check1 and check2 are passed then after check3 the db object should be returned. What should be the best practice for implementing this?

Should I write a new method with the same checks and return the db object after check3, or add more parameters and simplify the existing method?

I read the best practice is to have a maximum of 5 parameters, so if we add more parameters eventually the first method call will have 8-9 parameters and if we reduce the number of parameters then it will require more checks (if-statements) on every call which is also against OOPs basic principles. So is there an alternate solution?

Upvotes: 3

Views: 427

Answers (3)

Jul10
Jul10

Reputation: 503

You could apply the Strategy Pattern.

  1. Define a ValidationStrategy interface that expose the method public abstract void validate()
  2. Define a class ConcreteValidationA that implements the method validate() with the behaviour of your validate(param1, pararm2, param3 ...)
  3. Define a class ConcreteValidationB that implements the method validate() with the behaviour of the alternative method
  4. In the Context class (i.e. the class where your method stays) add a reference to a ValidationStrategy and modify validate(param1, pararm2, param3 ...) to choose a validation strategy and calling the relative validate() method by forwarding.

Context example

class Context {
   /*... your methods and members */

   public Context(){
      //...
      strategy=new ConcreteStrategyA();   //defalut strategy
   
   private ValidationStrategy strategy;

   public void validate(int param1, int param2,int param3){
         if(param1<param2 && param1<param3)
            strategy=new ConcreteStrategyA();
         else
            strategy=new ConcreteStrategyB();
         
         strategy.validate();
   }


}

Upvotes: 1

geckiss
geckiss

Reputation: 33

I would simply rewrite the method with same amount of parameters and return value of DbObject's type. Don't really understand what are those checks doing(they check if object's atributes are in range of parameters?..), but i would code something like this:

public DbObjectType validate(param1, param2, ...) {
    // if one of the checks fail, validation failed
    if (check1 || check2 || ...checkN) {
        add error;
        return null;
    }
    else {
        DbObject obj = getDbObject();
        if (lastCheck) {
            exception
            return null;
        }
        return obj;
    }
}

You don't have to have every if on separate line, if the actions performed after if are the same for every if. Chaining them together makes the code more readable. As the return value is DbObject, returning null will not break the program because object types can be null.

Upvotes: 1

Alan Blyth
Alan Blyth

Reputation: 195

I would rewrite to use Exceptions, intentionally returning null isn't that helpful to the caller, perhaps like this:

public DbObjectType validate(param1, param2, ...) throws Check1FailedException, Check2FailedException, Check3FailedException {

    check1();
    check2();
    check3();

    return getDbObject();
}

private check1() throws Check1FailedException() {
    //DoCheck and throw Exception if failed
}

private check2() throws Check2FailedException() {
    //DoCheck and throw Exception if failed
}

private check3() throws Check3FailedException() {
    //DoCheck and throw Exception if failed
}

Upvotes: 1

Related Questions