2
\$\begingroup\$

I working with a lot of Strings which I get from a web-form:

final String user = request.getParameter("user");
final String pw = request.getParameter("password");
final String name = request.getParameter("name");
final String shortname = request.getParameter("shortname");
final String appl_name = request.getParameter("appl_name");
final String db_size_str = request.getParameter("db_size");
final String was_admin_string = request.getParameter("was_admin");
final String sap_nr = request.getParameter("sap_nr");
final String etl_string = request.getParameter("etl");
final String mq_string = request.getParameter("mq");
final String msg_broker_string = request.getParameter("msgbroker");
final String java_batch = request.getParameter("javab");
final String server = request.getParameter("server");
final String apverantw1 = request.getParameter("appliverantw1");
final String apverantw2 = request.getParameter("appliverantw2");
final String psp = request.getParameter("psp");
final String version = request.getParameter("version");
final String applialias = request.getParameter("applialias");
final String applityp = request.getParameter("applityp");
final String data = request.getParameter("data");
final String yesdata = request.getParameter("yesdata");
final String moreoptions = request.getParameter("moreoptions");

If only one of them is empty, the program should be stopped. At this time I check that with an ugly if-statement if no one of the strings is empty:

if (user != null && name != null && shortname != null && appl_name != null && db_size_str != null && was_admin_string != null && sap_nr != null && etl_string != null && mq_string != null
                && msg_broker_string != null && java_batch != null && server != null && apverantw1 != null && apverantw2 != null && psp != null && version != null && applialias != null
                && applityp != null && data != null && yesdata != null && moreoptions != null)

This was produced over the time when I added one string after one.
How can I improve this? I mean, it is a really big, ugly if-statement. But I don't have a clue how can I write this even better.

\$\endgroup\$
3
\$\begingroup\$

i assume that you don't want to proceed the request, and send error message about missing string to the user, and also you are not using any MVC framework.

First you need to define a list of parameter names. Then define the below method,

 List<String> paramNames =  ... // you prepared a list of parameter names

List<String> validate(HttpServletRequest req ) {
   List<String> emptyParams = new ArrayList<>();

   for(String name : paramNames ){
      if(StringUtils.isBlank( req.getParameter(name))){
          emptyParams.add(param);
       }
   }

   return emptyParams;  
 }

Note: StringUtils class comes from the apache common library.

now the above method can tell you what parameters are empty. you can decide what to do based on the result.

if you use any mvc framework, it is better to use form binding logic and validation provided by the framework.

\$\endgroup\$
  • \$\begingroup\$ This version looks good. With some changes it is perfect for me. And yes, i'm using the mvc framework but i'm not very familiar with it. I'll take a look to some documentations. Thanks \$\endgroup\$ – Michael Schmidt Nov 5 '13 at 12:11
  • \$\begingroup\$ you are preparing a list of parameters of type String. This is only going to work when all your parameters are String which is fine as per current code of @dTDesign. However as soon as you start getting values other than String, it would be a problem. \$\endgroup\$ – Kinjal Nov 5 '13 at 14:54
  • \$\begingroup\$ @Kinjal: Thats true, but in my application, I always will get Strings. So for me the best answer \$\endgroup\$ – Michael Schmidt Nov 5 '13 at 15:07
  • \$\begingroup\$ the drawback would be you need to maintain that paramNames list every time there is a new request parameter \$\endgroup\$ – Kinjal Nov 5 '13 at 15:17
2
\$\begingroup\$

You can build a method which pulls a String from request and check if it is empty:

private static String getString(Request request, String parameter){

    String result = request.getParameter(parameter);
    if(result==null||result.isEmpty()){
        System.exit(0);
    }
    return result;
}
\$\endgroup\$
  • \$\begingroup\$ Looks very good and simple. Will give it a try. \$\endgroup\$ – Michael Schmidt Nov 5 '13 at 12:14
  • \$\begingroup\$ So when there are more than 20 parameters(as in @dTDesign's code), you will end up calling the method the same number of times. \$\endgroup\$ – Kinjal Nov 5 '13 at 14:43
1
\$\begingroup\$

You may (beside PKopachevsky's suggestion) as well create a new class for this with Request request being a member, and a getParameter method with the same signature as the one of Request, so you spare repeating the same request parameter again and again:

class RequestAdapter {

private Request request;
public RequestAdapter(Request request) {
    this.request = request;
}
public String getParameter(String parameterName) {
    String result = request.getParameter(parameterName);
    if (result==null) {
        System.exit(0);
    }
    return result;
}

Your may use it like this:

RequestAdapter request = new RequestAdapter(originalRequest);
final String user = request.getParameter("user");
final String pw = request.getParameter("password");
final String name = request.getParameter("name");
// and so on...
\$\endgroup\$
  • 1
    \$\begingroup\$ @tintinmj: Many thanks for your corrections :) you see, I'm from the C++ planet ;) \$\endgroup\$ – Wolf Nov 5 '13 at 11:46
  • \$\begingroup\$ yes i can see ;) and correct the indentation. \$\endgroup\$ – Anirban Nag 'tintinmj' Nov 5 '13 at 11:51
  • \$\begingroup\$ I don't think so this will reduce my code when I add a new class for such a simple task... \$\endgroup\$ – Michael Schmidt Nov 5 '13 at 12:13
  • \$\begingroup\$ @dTDesign: I add classes when I get rid of error-prone code duplications. I think you fundamental problem is to do each future edit twice for not breaking the implementation. Imagine the request provides another parameter after a while... \$\endgroup\$ – Wolf Nov 5 '13 at 12:26
  • \$\begingroup\$ @WolfP.: You're right, but I think in this case a method is the better way. But I'll take your solution also in my presentation (it's not the source code of my own program. I should improve this program). So +1 for an other way. \$\endgroup\$ – Michael Schmidt Nov 5 '13 at 12:35
0
\$\begingroup\$

nvseenu's answer is good but I like varargs. In your getParameter method

{
    String user = .... // all the parameters
    .
    .
    .
    // then
    validate(user, pwd, ...) // and all this may look big but with auto-completion 
                             // it's not a big headache
}

private void validate(String... parameters) {
    for(String parameter : parameters) {
        if(parameter == null || "".equals(parameter)) {
            System.exit(0); // but it's bad IMO
        }
    }
    return;
}
\$\endgroup\$
0
\$\begingroup\$

Let me assume your class name is SomeRequestBuilder. With the following solution I will try to validate and get the request value of a specific parameter at the same time. So you do not have to fetch same value twice - one for validation and other one to process the value. I have used reflection to do this. I see you have declared all parameters as final, so I had to use setAccessible as true. In case you do not need those as final then setAccessible can be removed. This code can be improved further with better exception handing. Here I wanted to show you how to do validation with less code.

class SomeRequestBuilder { 
  private final String user = "";
  private final String password = "";                                                                                                                                                                                                                                                   
  private final String name = "";
  private final String shortname = "";                                                                                                                                                                                                                                                  
  private final String applName = ""; 
  private final String dbSize = "";                                                                                                                                                                                                                                                    
  private final String wasAdmin = "";
  private final String sapNr = "";                                                                                                                                                                                                                                                     
  private final String etl = "";                                                                                                                                                                                                                                                        
  private final String mq = "";
  private final String msgbroker = "";                                                                                                                                                                                                                           
  private final String javab = ""; 
  private final String server = ""; 
  private final String appliverantw1 = ""; 
  private final String appliverantw2 = "";                                                                                                                                                                                                                                              
  private final String psp = "";  
  private final String version = ""; 
  private final String applialias = ""; 
  private final String applityp = "";
  private final String data = ""; 
  private final String yesdata = ""; 
  private final String moreoptions = "";  

  public void validateAndSetValues(SomeRequest request) {
     String input = "";  
     Field[] fields = getClass().getDeclaredFields();
     for (Field field : fields) { 
       try {
         input = request.getParameter(field.getName()); 
         if (null == input) { 
           // return here...
         } 
         field.setAccessible(true);
         field.set(this, input);
       } catch(IllegalArgumentException e) {  
         throw new IllegalArgumentException("caught : " + e);  
     }
   }
  }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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