3
\$\begingroup\$

I currently have an app that uses Firebase for logging users in and I would like to know if I can make this code any better. I currently have 4 files:

auth/models/User.java

auth/BaseActivity.java

auth/CreateAccountActivity.java

auth/LoginActivity.java

User.java:

@IgnoreExtraProperties
public class User {

    public String username;
    public String email;

    public User() {

    }

    public User(String username, String email) {
        this.username = username;
        this.email = email;
    }

}

BaseActivity.java

public class BaseActivity extends AppCompatActivity {

    private ProgressDialog mProgressDialog;

    public void showProgressDialog() {
        if (mProgressDialog == null) {
            mProgressDialog = new ProgressDialog(this);
            mProgressDialog.setMessage(getString(R.string.loading));
            mProgressDialog.setIndeterminate(true);
            mProgressDialog.setCancelable(false);
        }
        mProgressDialog.show();
    }

    public void hideProgressDialog() {
        if (mProgressDialog != null && mProgressDialog.isShowing()) {
            mProgressDialog.dismiss();
        }
    }

    @Override
    public void onDestroy() {
        super.onDestroy();
        hideProgressDialog();
    }

}

CreateAccountActivity.java:

public class CreateAccountActivity extends BaseActivity {

    private EditText mUsernameField, mEmailField, mPasswordField;

    private DatabaseReference mDatabase;

    private FirebaseAuth mAuth;

    private FirebaseAuth.AuthStateListener mAuthListener;

    ProgressDialog mProgressDialog;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_create_account);

        mUsernameField = (EditText) findViewById(R.id.createAccountUsernameInput);
        mEmailField = (EditText) findViewById(R.id.createAccountEmailInput);
        mPasswordField = (EditText) findViewById(R.id.createAccountPasswordInput);

        Button mCreateAccountButton = (Button) findViewById(R.id.createAccountButton);

        TextView mLoginLink = (TextView) findViewById(R.id.loginLink);

        mProgressDialog = new ProgressDialog(this);
        mProgressDialog.setCancelable(false);
        mProgressDialog.setMessage("Please wait...");

        mDatabase = FirebaseDatabase.getInstance().getReference();
        mAuth = FirebaseAuth.getInstance();

        mAuthListener = new FirebaseAuth.AuthStateListener() {
            @Override
            public void onAuthStateChanged(@NonNull FirebaseAuth firebaseAuth) {
                FirebaseUser user = firebaseAuth.getCurrentUser();
                if (user != null) {
                    startActivity(new Intent(CreateAccountActivity.this, MainActivity.class));
                    finish();
                }
            }
        };

        assert mCreateAccountButton != null;
        mCreateAccountButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                createAccount(mEmailField.getText().toString(), mPasswordField.getText().toString());
            }
        });

        assert mLoginLink != null;
        mLoginLink.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                startActivity(new Intent(CreateAccountActivity.this, LoginActivity.class));
                finish();
            }
        });
    }

    public void createAccount(String email, String password) {
        if (!validateForm()) {
            return;
        }

        showProgressDialog();

        mAuth.createUserWithEmailAndPassword(email, password).addOnCompleteListener(this, new OnCompleteListener<AuthResult>() {
            @Override
            public void onComplete(@NonNull Task<AuthResult> task) {
                if (!task.isSuccessful()) {
                    Toast.makeText(CreateAccountActivity.this, "Error: Please try again.", Toast.LENGTH_SHORT).show();
                } else {
                    onAuthSuccess(task.getResult().getUser());
                }
                hideProgressDialog();
            }
        });
    }

    private void onAuthSuccess(FirebaseUser user) {
        String username = mUsernameField.getText().toString();

        writeNewUser(user.getUid(), username, user.getEmail());
    }

    private void writeNewUser(String userId, String name, String email) {
        User user = new User(name, email);

        mDatabase.child("users").child(userId).setValue(user);
    }

    @Override
    public void onStart() {
        super.onStart();
        mAuth.addAuthStateListener(mAuthListener);
    }

    @Override
    public void onStop() {
        super.onStop();
        if (mAuthListener != null) {
            mAuth.removeAuthStateListener(mAuthListener);
        }
    }

    private boolean validateForm() {
        boolean valid = true;

        String username = mUsernameField.getText().toString();
        if (TextUtils.isEmpty(username)) {
            mUsernameField.setError("Required");
            valid = false;
        } else {
            mUsernameField.setError(null);
        }

        String email = mEmailField.getText().toString();
        if (TextUtils.isEmpty(email)) {
            mEmailField.setError("Required");
            valid = false;
        } else {
            mEmailField.setError(null);
        }

        String password = mPasswordField.getText().toString();
        if (TextUtils.isEmpty(password)) {
            mPasswordField.setError("Required");
            valid = false;
        } else {
            mPasswordField.setError(null);
        }

        return valid;
    }

}

LoginActivity.java:

public class LoginActivity extends BaseActivity {

    private EditText mEmailField, mPasswordField;

    private FirebaseAuth mAuth;

    private FirebaseAuth.AuthStateListener mAuthListener;

    ProgressDialog mProgressDialog;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_login);

        mEmailField = (EditText) findViewById(R.id.loginEmailInput);
        mPasswordField = (EditText) findViewById(R.id.loginPasswordInput);

        Button mLoginButton = (Button) findViewById(R.id.loginButton);

        TextView mCreateAccountLink = (TextView) findViewById(R.id.createAccountLink);

        mProgressDialog = new ProgressDialog(this);
        mProgressDialog.setCancelable(false);
        mProgressDialog.setMessage("Please wait...");

        mAuth = FirebaseAuth.getInstance();

        mAuthListener = new FirebaseAuth.AuthStateListener() {
            @Override
            public void onAuthStateChanged(@NonNull FirebaseAuth firebaseAuth) {
                FirebaseUser user = firebaseAuth.getCurrentUser();
                if (user != null) {
                    startActivity(new Intent(LoginActivity.this, MainActivity.class));
                    finish();
                }
            }
        };

        assert mLoginButton != null;
        mLoginButton.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                signIn(mEmailField.getText().toString(), mPasswordField.getText().toString());
            }
        });

        assert mCreateAccountLink != null;
        mCreateAccountLink.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                startActivity(new Intent(LoginActivity.this, CreateAccountActivity.class));
                finish();
            }
        });
    }

    @Override
    public void onStart() {
        super.onStart();
        mAuth.addAuthStateListener(mAuthListener);
    }

    @Override
    public void onStop() {
        super.onStop();
        if (mAuthListener != null) {
            mAuth.removeAuthStateListener(mAuthListener);
        }
    }

    private void signIn(String email, String password) {
        if (!validateForm()) {
            return;
        }

        showProgressDialog();

        mAuth.signInWithEmailAndPassword(email, password).addOnCompleteListener(this, new OnCompleteListener<AuthResult>() {
            @Override
            public void onComplete(@NonNull Task<AuthResult> task) {
                if (!task.isSuccessful()) {
                    Toast.makeText(LoginActivity.this, "Error: Please try again.", Toast.LENGTH_SHORT).show();
                }
                hideProgressDialog();
            }
        });
    }

    private boolean validateForm() {
        boolean valid = true;

        String email = mEmailField.getText().toString();
        if (TextUtils.isEmpty(email)) {
            mEmailField.setError("Required");
            valid = false;
        } else {
            mEmailField.setError(null);
        }

        String password = mPasswordField.getText().toString();
        if (TextUtils.isEmpty(password)) {
            mPasswordField.setError("Required");
            valid = false;
        } else {
            mPasswordField.setError(null);
        }
        return valid;
    }

}
\$\endgroup\$
2
\$\begingroup\$

Field visibility and encapsulation

I think that fields of User can be also private.

Instead of:

public String username;
public String email;

You can use:

private String username;
private String email;

I think that having default constructor for User is not good idea, because somebody can create user without username or email? I don't think so.

The default constructor is the no-argument constructor automatically generated unless you define another constructor

I advise you to remove:

public User() {
}

It allows to avoid future possible errors with user's fields.

DRY(do not repeat yourself)

In BaseActivity you have:

if (mProgressDialog == null) {
  mProgressDialog = new ProgressDialog(this);
  mProgressDialog.setMessage(getString(R.string.loading));
  mProgressDialog.setIndeterminate(true);
  mProgressDialog.setCancelable(false);
}

In CreateAccountActivity you have:

mProgressDialog = new ProgressDialog(this);
mProgressDialog.setCancelable(false);
mProgressDialog.setMessage("Please wait...");

In LoginActivity you have:

mProgressDialog = new ProgressDialog(this);
mProgressDialog.setCancelable(false);
mProgressDialog.setMessage("Please wait...");

Instead of all these lines you can just add some method to BaseActivity for creation progress dialog:

public ProgressDialog createDialog(context Context, isCancelable boolean, String message) 
{
  ProgressDialog mProgressDialog = new ProgressDialog(context);
  mProgressDialog.setMessage(message);
  mProgressDialog.setCancelable(isCancelable);

  return mProgressDialog;
}

Objects of other classes also can use this method because they will inherit it.

Long Method

You have method:

private boolean validateForm() {}

I suggest you to modify name for this method, as example

private boolean isFormValid() {}

Also this method (isFormValid() - I'll use my own name for it) has 'specific smell'. Let's do some refactoring.

Begin with logic of the method:

private boolean isFormValid() { 
  // part 1 - check username

  // part 2 - check email

  // part 3 - check password
}

Now you see that your method has three separate methods, we need to extract them:

private boolean isFormValid() { 
  // part 1 - check username
  boolean isUserNameValid = checkUserName();

  // part 2 - check email
  boolean isEmailValid = checkEmail();

  // part 3 - check password
  boolean isPasswordValid = checkPassword();

  return isUserNameValid && isEmailValid && isPasswordValid;
}

private boolean checkUserName() {
  String username = mUsernameField.getText().toString();

  if (TextUtils.isEmpty(username)) {
    mUsernameField.setError("Required");
    valid = false;
  } else {
    mUsernameField.setError(null);
  }
}

private boolean checkEmail() {
  String email = mEmailField.getText().toString();

  if (TextUtils.isEmpty(email)) {
    mEmailField.setError("Required");

    valid = false;
  } else {
    mEmailField.setError(null);
  }  
}

private boolean checkPassword {
  String password = mPasswordField.getText().toString();

  if (TextUtils.isEmpty(password)) {
    mPasswordField.setError("Required");

    valid = false;
  } else {
    mPasswordField.setError(null);
  }
}

I think that it also seem's not good, we have three method that have the same logic, need to extract similar logic parts into another method:

//you can use another method name
private void checkEditText(EditText et) {
  String field = et.getText().toString();

  if (TextUtils.isEmpty(field)) {
    et.setError("Required");
    return false;
  } else {
    et.setError(null);
    return true;
  }
}

An as a result we have very clean an understandable code for validation the form:

private boolean checkUserName() {
  return checkEditText(mUsernameField);
}

private boolean checkEmail() {
  return checkEditText(mEmailField); 
}

private boolean checkPassword {
  return checkEditText(mPasswordField);
}
\$\endgroup\$
  • \$\begingroup\$ @AlexizHernandez, you are welcome, also you can upvote this review if you like it. \$\endgroup\$ – dshil Jun 21 '16 at 5:05
  • \$\begingroup\$ @martinShil he didn't actually have enough rep to upvote. He does now ;-) \$\endgroup\$ – janos Aug 3 '16 at 8:43
  • \$\begingroup\$ @AlexizHernandez hi there! fyi, now you have enough reputation to vote on posts. Enjoy the site! \$\endgroup\$ – janos Aug 3 '16 at 8:44

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.