Problem
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;
}
}
Solution
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.
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);
}