Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am currently reworking our entity code generator, that uses JaxB for schema validation, and Stringtemplate for code generation.

As we are giving our professional service the possibility to add Validators to the attributes of the entities they create, I need to wrap each parsed Field, in order to add the Validators.

The following code works, but I do not feel comfortable with it:

@SuppressWarnings( "boxing" )
private Map<String, Object> wrapField( Field f )
  {
    HashMap<String, Object> result = new HashMap<>();
    if ( f.isGenerateGetter() )
     {
      DecimalMaxValidator decMax = null;
      DecimalMinValidator decMin = null;
      NotBlankValidator notBlank = null;
      NotNullValidator notNull = null;
      LengthValidator lengthVal = null;

      result.put( "generateGetters", f.isGenerateGetter() );
      result.put( "getterAddOverride", f.isGetterAddOverride() );
      for ( Validator v : f.getValidators().getValidators() )
      {
        HashMap<String, Object> tmp = new HashMap<>();
        if ( v instanceof DecimalMaxValidator )
        {
          decMax = addDecimalMaxValidator( result, decMax, v, tmp );
        }
        else if ( v instanceof DecimalMinValidator )
        {
          decMin = addDecimalMinValidator( result, decMin, v, tmp );
        }
        else if ( v instanceof NotBlankValidator )
        {
          notBlank = addNotBlankValidator( notBlank, v, tmp );
        }
        else if ( v instanceof NotNullValidator )
        {
          notNull = addNotNullValidator( notNull, v, tmp );
        }
        else if ( v instanceof LengthValidator )
        {
          lengthVal = addLengthValidator( lengthVal, v, tmp );
        }
        else
        {
          // TODO: error: Wrong type? type not found?
        }

      }
    }
    return result;
  }

  private NotNullValidator addNotNullValidator( NotNullValidator notNull, Validator v, HashMap<String, Object> tmp )
  {
    if ( notNull == null )
    {
      notNull = (NotNullValidator) v;
      tmp.put( "notNullValidator", true );
      tmp.put( "notBlankValidatorTransKey", v.getTranslationKey() );
    }
    else
    {
      // TODO error: double dec
    }
    return notNull;
  }

  @SuppressWarnings( "boxing" )
  private NotBlankValidator addNotBlankValidator( NotBlankValidator notBlank, Validator v, HashMap<String, Object> tmp )
  {
    if ( notBlank == null )
    {
      notBlank = (NotBlankValidator) v;
      tmp.put( "notBlankValidator", true );
      tmp.put( "notBlankValidatorTransKey", v.getTranslationKey() );
    }
    else
    {
      // TODO error, double dec
    }
    return notBlank;
  }

  @SuppressWarnings( "boxing" )
  private DecimalMinValidator addDecimalMinValidator( HashMap<String, Object> result, DecimalMinValidator decMin, Validator v,
      HashMap<String, Object> tmp )
  {
    if ( decMin == null )
    {
      decMin = (DecimalMinValidator) v;
      tmp.put( "value", decMin.getValue() );
      tmp.put( "inclusive", decMin.isInclusive() );
      tmp.put( "translationKey", decMin.getTranslationKey() );
      result.put( "decimalMinValidator", tmp );
    }
    else
    {
      // TODO error, double dec
    }
    return decMin;
  }

  @SuppressWarnings( "boxing" )
  private DecimalMaxValidator addDecimalMaxValidator( HashMap<String, Object> result, DecimalMaxValidator decMax, Validator v,
      HashMap<String, Object> tmp )
  {
    if ( decMax == null )
    {
      decMax = (DecimalMaxValidator) v;
      tmp.put( "value", decMax.getValue() );
      tmp.put( "inclusive", decMax.isInclusive() );
      tmp.put( "translationKey", decMax.getTranslationKey() );
      result.put( "decimalMaxValidator", tmp );
    }
    else
    {
      // TODO error, double dec
    }
    return decMax;
  }

  private LengthValidator addLengthValidator( LengthValidator lengthVal, Validator v, HashMap<String, Object> tmp )
  {
    if ( lengthVal == null )
    {
      lengthVal = (LengthValidator) v;
      tmp.put( "min", lengthVal.getMin() );
      tmp.put( "max", lengthVal.getMax() );
      tmp.put( "translationKey", lengthVal.getTranslationKey() );
    }
    else
    {
      // TODO error, double dec
    }
    return lengthVal;
  }

There are no explanations yet, but the code should be mostly self-explanatory.

Comparing the outsourced addXXXValidator methods, feels awkward regarding redundant code.

As I am not very familiar with the java generics, I need your help with refatoring this code, so it gets smaller, faster and better maintainable.

How can I make the addXXXValidator methods more "beautiful"?

Note:

I am not allowed to edit the Validator classes.


Explanation to the getValidators()call:

f.getValidators()  // Wrapper, returns a Validators object
 .getValidators()  // "second call", returns a List of Validator objects

while there is the Validators object: (the types are our internal types)

<complexType name="validatorsType">
   <complexContent>
     <restriction base="{http://www.w3.org/2001/XMLSchema}anyType">
       <choice maxOccurs="unbounded" minOccurs="0">
         <element name="notNullValidator" type="notNullValidatorType"/>
         <element name="notBlankValidator" type="notBlankValidatorType"/>
         <element name="decimalMaxValidator" type="decimalMaxValidatorType"/>
         <element name="decimalMinValidator" type="decimalMinValidatorType"/>
         <element name="lengthValidator" type="lengthValidatorType"/>
         <element name="patternValidator" type="patternValidatorType"/>
       </choice>
     </restriction>
   </complexContent>
 </complexType>

and the Validator class:

 <complexType name="validatorType">
   <complexContent>
     <restriction base="{http://www.w3.org/2001/XMLSchema}anyType">
       <attribute name="translationKey" type="{http://www.w3.org/2001/XMLSchema}string" />
     </restriction>
   </complexContent>
 </complexType>

The NotNullValidator :

 <complexType name="notNullValidatorType">
   <complexContent>
     <extension base="validatorType">
     </extension>
   </complexContent>
 </complexType>

The DecimalMaxValidator :

 <complexType name="decimalMaxValidatorType">
   <complexContent>
     <extension base="validatorType">
       <attribute name="value" use="required" type="{http://www.w3.org/2001/XMLSchema}string" />
       <attribute name="inclusive" type="boolean" default="true" />
     </extension>
   </complexContent>
 </complexType>

The DecimalMinValidator is similar to DecimalMaxV. The NotBlankV similar to NotNullV.

The LengthValidator:

 <complexType name="lengthValidatorType">
   <complexContent>
     <extension base="validatorType">
       <attribute name="min" type="{http://www.w3.org/2001/XMLSchema}integer" default="0" />
       <attribute name="max" type="{http://www.w3.org/2001/XMLSchema}integer" default="2147483647" />
     </extension>
   </complexContent>
 </complexType>

While the above Schemafragments represent the

Expected content of the class (taken from the class documentation)

share|improve this question
    
I assume you mean decMax = addDecimalMaxValidator( result, decMax, v, result); in each wrapField else if? – Sylvain Boisse Jan 18 at 15:12
    
What does the field.getValidators() return? It could be interesting. If its a custom validators holder object, it might hold the key to the solution. There is code lacking, would you mind sharing it? I don't like guessing too much. – Sylvain Boisse Jan 18 at 15:23
    
@SylvainBoisse No, I am setting the variables decMax, decMin, notBlank etc.to the return values of the outsourced functions. I will update the question in order to share the meaning of getValidators() – Do Re Jan 18 at 15:38
    
@DoRe You cannot change the Validator class. Can you change the Validators class? I suspect you can't either. – Sylvain Boisse Jan 18 at 16:09
    
@SylvainBoisse No, I cannot change any class besides the one I am currently writing. Anyway, I like the idea mtj had in his answer. I will update my code and question accordingly – Do Re Jan 19 at 7:20
up vote 2 down vote accepted

You could replace your instance-check with a configuration map, which holds the class as the key and a consumer as the values.

Some improvised code to illustrate the principle:

public class Try43 {
    // mockup of your classes and hierarchy
    private static interface Validator {}
    private static class TestValidator implements Validator {}

    public static void main(String[] args) {
        // Build and fill the map at some central point:
        Map<Class<? extends Validator>, Consumer<Validator>> config = new HashMap<>();
        config.put(TestValidator.class, v -> doSomething("constantparam", v));

        // This replaces your if-else-cascade:
        Validator v = new TestValidator();
        if(config.containsKey(v.getClass()))
            config.get(v.getClass()).accept(v);
        // else error-handling
    }

    private static void doSomething(String someParameter, Validator val) {
    }
}

... if you need to pass a local tmp-map, this will be more BiConsumer-ish in your case, but the idea should be clear.

share|improve this answer
    
I like this idea! I will update my code and then update my answer. Maybe there is something to be done about the functions as well. Thank you so far! – Do Re Jan 19 at 7:19
    
after refactoring my code according to your solution, is looks so much better, that I decided to not split the functions anymore, but use enum values as constantParam(as you called it) and simple switch case over it – Do Re Jan 19 at 11:54

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.