4
\$\begingroup\$

I have set alert dialog on textview's onclicklistener, and there I have added some items using array adapter. Now what I want to do is check if the user selects an item from the alert dialog and clicks on submit. It should submit, otherwise not. But as per my if I select or not it displays message fill the details.

 boolean isSetDay = false;
 sp3=(TextView)findViewById(R.id.daydates);
 btnsubmit=(Button)findViewById(R.id.btnreg);

 final String[] items = new String[] {"01","02","03","04","05","06","07","08","09","10","11","12","13","14","15","16","17","18","19","20","21","22","23","24","25","26","27","28","29","30","31"};
 adapter123 = new ArrayAdapter<String>(this,
    android.R.layout.simple_spinner_dropdown_item, items);


 sp3.setOnClickListener(new OnClickListener() {

@Override
public void onClick(View w) {
      new AlertDialog.Builder(RegistrationForm.this)
      .setTitle("Select Day")
      .setAdapter(adapter123, new DialogInterface.OnClickListener() {

        @Override
        public void onClick(DialogInterface dialog, int which) {

            sp3.setText(adapter123.getItem(which).toString());
            isSetDay=true;

          dialog.dismiss();
        }
      }).create().show();
    }
  });
     btnsubmit.setOnClickListener(new OnClickListener()
  {

 @Override
 public void onClick(View v) 
 {
    if(emailtext.getText().toString().trim().length() > 0 &&   passtext.getText().toString().trim().length() > 0 && confirmpass.getText().toString().trim().length() > 0 && username.getText().toString().trim().length() > 0 && firstname.getText().toString().trim().length() > 0 && lastname.getText().toString().trim().length() > 0 && isSetDay==true && isSetMonth==true &&    isSetYear==true && isSetPro==true)
{
isSetDay=false;
isSetMonth=false;
isSetYear=false;
isSetPro=false;
new AttemptLogin().execute();


}

  else
 {
    Toast.makeText(getApplicationContext(), "Fill the details", Toast.LENGTH_LONG).show();
 }
  break;
 }
 });
\$\endgroup\$
4
\$\begingroup\$

Please pay attention to formatting. The code as you posted is hard to read, and not pleasant to review.

Most notably, the indentation is inconsistent or sometimes random, and you have very long horizontal lines forcing readers to scroll. For example this one:

if(emailtext.getText().toString().trim().length() > 0 &&   passtext.getText().toString().trim().length() > 0 && confirmpass.getText().toString().trim().length() > 0 && username.getText().toString().trim().length() > 0 && firstname.getText().toString().trim().length() > 0 && lastname.getText().toString().trim().length() > 0 && isSetDay==true && isSetMonth==true &&    isSetYear==true && isSetPro==true)

Broken up to multiple lines it becomes a lot easier to read:

    if (emailtext.getText().toString().trim().length() > 0
            && passtext.getText().toString().trim().length() > 0
            && confirmpass.getText().toString().trim().length() > 0
            && username.getText().toString().trim().length() > 0
            && firstname.getText().toString().trim().length() > 0
            && lastname.getText().toString().trim().length() > 0
            && isSetDay == true
            && isSetMonth == true
            && isSetYear == true
            && isSetPro == true)

Note that using == true with boolean expressions is really pointless, as you could use the expressions directly: if (something) { ... } is exactly the same as if (something == true) { ... }. So you could drop all the == true from the above.

When checking if a string is empty, instead of str.length() > 0, a better way is !str.isEmpty().

To avoid code duplication of .getText().toString().trim().isEmpty() it would be a good idea to add a helper function:

private boolean notEmpty(EditText input) {
    return !input.getText().toString().trim().isEmpty();
}

So that you can rewrite the condition in the if like this:

    if (notEmpty(emailtext)
            && notEmpty(passtext)
            && notEmpty(confirmpass)
            && notEmpty(username)
            && notEmpty(firstname)
            && notEmpty(lastname)
            && isSetDay
            && isSetMonth
            && isSetYear
            && isSetPro)

Even better, as @hkj suggested, the notEmpty method could take varargs:

private boolean notEmpty(EditText ... args) {
    for (EditText input : args) {
        if (input.getText().toString().trim().isEmpty()) {
            return false;
        }
    }
    return true;
}

which will allow you to shorten the condition even more:

    if (notEmpty(emailtext, passtext, confirmpass, username, firstname, lastname)
            && isSetDay
            && isSetMonth
            && isSetYear
            && isSetPro)

This line is also too long, and also error-prone to type it:

final String[] items = new String[] {"01","02","03","04","05","06","07","08","09","10","11","12","13","14","15","16","17","18","19","20","21","22","23","24","25","26","27","28","29","30","31"};

It would be better to fill this array programmatically, for example:

    final String[] items = new String[31];
    for (int i = 0; i < items.length; ++i) {
        items[i] = String.format("%02d", i + 1);
    }
\$\endgroup\$
  • \$\begingroup\$ How about varags for that nifty notEmpty() method? :) \$\endgroup\$ – h.j.k. Jan 13 '15 at 9:03
  • \$\begingroup\$ @h.j.k. now that would be really nifty ;-) Good tip, thanks, updated my post. \$\endgroup\$ – Stop ongoing harm to Monica Jan 13 '15 at 9:14
  • \$\begingroup\$ @Heslacher I don't have an Android dev env here to verify, but I think it's needed. Are you really sure it's not? developer.android.com/reference/android/widget/… \$\endgroup\$ – Stop ongoing harm to Monica Jan 13 '15 at 9:28
  • \$\begingroup\$ oops, I only read java, so no you are right \$\endgroup\$ – Heslacher Jan 13 '15 at 9:34
  • \$\begingroup\$ @Heslacher I thought so, and yeah, it's a real bummer eh ;) \$\endgroup\$ – Stop ongoing harm to Monica Jan 13 '15 at 9:37

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.