Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've answered this question on Stack Overflow. The problem is - there are 30 columns in an Excel sheet and I need to validate each of the fields with different validation logic. The code should avoid a lot of if-else blocks. I have used strategy pattern to solve this. I would like to get a review of this code.

interface IValidator<T> {
  boolean validate(T field);
}

class SomeFieldOne<T> implements IValidator<T> {
  public boolean validate(T field) {
    System.out.println("SomeFieldOne validation");
    return true; // return true/false based on validation
  }
}

class SomeFieldTwo<T> implements IValidator<T> {
  public boolean validate(T field) {
    System.out.println("SomeFieldTwo validate");
    return true; // return true/false based on validation
  }
}

class Context {
  private IValidator validator;

  public Context(IValidator validator) {
    this.validator = validator;
  }

  public boolean validate(String field) {
    return this.validator.validate(field);
  }
}

public class TestValidation {
  public static void main(String[] args) {
    Context context;

    context = new Context(new SomeFieldOne());
    System.out.println(context.validate("some field one"));

    context = new Context(new SomeFieldTwo());
    System.out.println(context.validate("some field two"));

    // test other fields ....
    // .........
  }
}
share|improve this question
add comment

3 Answers

I have heard from some people that you don't want to use design patterns as templates for writing code, the patterns are meant to show you how different pieces of code fit together, on a larger scale you may use many different Patterns by the time you are done with an application. so don't just pick a pattern and try to make your idea fit that pattern, it may or may not work the way that you want or need it to. keep thinking outside the box.

not really a code review, but I think that the question warranted an answer that talked about patterns even if I only floated on the surface

share|improve this answer
    
@Ravi Kumar's implementation looks more like trying to "fit" Strategy Pattern into Validation class.. –  Kinjal Nov 22 '13 at 7:51
add comment

At times using pattern may overkill the structure. I am not saying that the pattern you used here is not good, but I wanted show that there is also another way where you can pass different block of code to achieve validation for different type of fields.

interface Block<T> {
  public boolean value(T arg);
}

class FieldValidator {
  public static <T> boolean validate(T element, Block<T> block) {
    return block.value(element);
  }
}

public class UnitTest {
  public static void main(String[] args) {
    // some int field validattion
    System.out.println(FieldValidator.validate(2, new Block<Integer>() {
        public boolean value(Integer arg) {
          System.out.println("validating - " + arg);
          return true; // return true/false 
        }
      }));

    // some string field validation
    System.out.println(FieldValidator.validate("some field 1", new Block<String>() {
        public boolean value(String arg) {
          System.out.println("validating - " + arg);
          return true; // return true/false 
        }
      }));
  }
}
share|improve this answer
1  
return block.value(element) ? true : false; can be simplified to return block.value(element); –  Simon André Forsberg Nov 17 '13 at 12:16
    
yes, I missed that, let me edit it –  Kinjal Nov 17 '13 at 15:30
add comment

First of all, you're not using your generics properly here:

private IValidator validator;

I would change this to

private final IValidator<String> validator;

Because:

  1. final because the value does not change, and should not change. The field should be immutable
  2. <String> to use generics properly. Otherwise you should get a compiler warning saying IValidator is a raw type. References to generic type IValidator<E> should be parameterized

However, I think your Context is not very useful. I don't see any reason for why you need it. You can accomplish the exact same things by using:

public class TestValidation {
  public static void main(String[] args) {
    IValidator<String> context;
    context = new SomeFieldOne();
    System.out.println(context.validate("some field one"));
    context = new SomeFieldTwo();
    System.out.println(context.validate("some field two"));
    // test other fields ....
    // .........
  }
}

This is less code and has the same effect. And it also looks better and improves readability as changing strategy now calls only one constructor instead of two.

I assume you're compiling with Java 1.6 or above, and therefore your classes that implements the IValidator interface should mark the validate method with @Override to comply with the Java coding conventions. (Marking methods with @Override that overrides interface methods was not supported at all in previous Java versions)

@Override
public boolean validate(T field) {
share|improve this answer
    
very useful comment. I will take note of this. –  Ravi Kumar Nov 16 '13 at 20:13
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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