current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 4 down vote favorite

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;
 }
 });
share|improve this question
    
What is your question? –  janos♦ Jan 13 at 6:27

1 Answer 1

up vote 4 down vote accepted

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);
    }
share|improve this answer
    
How about varags for that nifty notEmpty() method? :) –  h.j.k. Jan 13 at 9:03
    
@h.j.k. now that would be really nifty ;-) Good tip, thanks, updated my post. –  janos♦ Jan 13 at 9:14
    
@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/… –  janos♦ Jan 13 at 9:28
    
oops, I only read java, so no you are right –  Heslacher Jan 13 at 9:34
    
@Heslacher I thought so, and yeah, it's a real bummer eh ;) –  janos♦ Jan 13 at 9:37

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.