Problem
I have a code that adds a user to the database, getting the username from the dialog.My fragment implements the interface with the onInputSend method, which is called by the dialog when the user clicks the desired button.
Here is the code that adds the user to the database. I want to hear criticism of my code.
UsersActivity
public void onInputSend(@NonNull InputTextAlertDialog dialog, @NotNull String input) {
userActionManager.isEmptyUser(input)
.doOnSubscribe(disposable -> disposables.add(disposable))
.subscribe(empty -> insertUser(dialog, input, empty));
}
private void insertUser(InputTextAlertDialog dialog, String userName, boolean isEmpty) {
if (isEmpty) {
if (!userName.trim().isEmpty()) {
userActionManager.insertUser(new User(userName));
dialog.cancel();
} else {
dialog.setErrorMessage(getString(R.string.user_not_have_name));
}
} else {
dialog.setErrorMessage(getString(R.string.user_already_exists));
}
}
InputTextAlertDialog
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
var onInputOkListener: OnInputOkListener? = null
private var input: EditText? = null
private var error: TextView? = null
var colorError: Int = Color.RED
set(colorError: Int) {
field = colorError
error!!.setTextColor(colorError)
}
init {
error = view.findViewById(R.id.error_text)
input = view.findViewById(R.id.input)
error!!.setTextColor(colorError)
}
fun setErrorMessage(errorMessage: String?) {
error!!.text = errorMessage
input!!.backgroundTintList = ColorStateList.valueOf(colorError);
}
override fun initOkClickListener() {
ok.setOnClickListener { v: View? ->
ok()
}
}
override fun ok() {
onInputOkListener?.onInputSend(this, input!!.text.toString())
}
override fun getLayout(): Int {
return R.layout.input_text_layout
}
interface OnInputOkListener {
fun onInputSend(dialog: InputTextAlertDialog, input: String)
}
}
BaseAlertDialog
public abstract class BaseAlertDialog {
AlertDialog alertDialog;
protected Button cancel;
protected Button ok;
protected View view;
private AlertDialog.Builder builder;
protected Context context;
BaseAlertDialog(Context context) {
this.context = context;
initDialog(context);
initViews(view);
initCancelClickListener();
initOkClickListener();
}
private void initDialog(Context context) {
if (getLayout() != 0) {
view = LayoutInflater.from(context).inflate(getLayout(), null);
} else {
throw new RuntimeException("In getLayout () you should return the layout id");
}
builder = new AlertDialog.Builder(context);
builder.setView(view);
alertDialog = builder.create();
}
private void initViews(View view) {
cancel = view.findViewById(R.id.cancel);
ok = view.findViewById(R.id.ok);
}
private void initCancelClickListener() {
cancel.setOnClickListener(v -> {
alertDialog.cancel();
});
}
protected void initOkClickListener() {
if (ok != null) {
ok.setOnClickListener(v -> {
ok();
alertDialog.cancel();
});
}
}
public void cancel(){
alertDialog.cancel();
}
public void show(){
alertDialog.show();
}
abstract void ok();
abstract int getLayout();
}
Solution
Nullable
Your fields are optionals: you put a ?
after the type.
This means every time you need to access them, you need to check if it is null…
If you want the fields to be always present, throw the NullPointerException when findViewById
doesn’t return a view.
Then you can make your field not nullable and you can remove all the (now) redundant checks…
oneline function
override fun getLayout(): Int {
return R.layout.input_text_layout
}
This function doesn’t need 3 lines of attention…
Change it to one:
override fun getLayout(): Int = R.layout.input_text_layout
you could remove the return-type as well:
override fun getLayout() = R.layout.input_text_layout
init
Personal choice:
I like to make everything as short as possible.
This can be done by declaring the fields straight away instead of in the init block:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
var onInputOkListener: OnInputOkListener? = null
var colorError: Int = Color.RED
set(colorError: Int) {
field = colorError
error!!.setTextColor(colorError)
}
private var input: EditText = view.findViewById(R.id.input)!!
private var error: TextView = view.findViewById(R.id.error_text)!!
init {
error!!.setTextColor(colorError)
}
}
also
We can make the code above a bit simpler by using also
:
also
is a function which is called upon a variable (called the receiver).
also
will return the receiver and accepts a lambda as parameter.
Inside that lambda, it gives one parameter: the receiver.
Therefor the following code:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
private var error: TextView = view.findViewById(R.id.error_text)!!
init {
error!!.setTextColor(colorError)
}
}
can be reduced to:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
private var error: TextView = view.findViewById(R.id.error_text)!!
.also{ v: TextView -> v.setTextColor(colorError) }
}
And if there is only one param, it can be accessed using it:
class InputTextAlertDialog(context: Context) : BaseAlertDialog(context) {
private var error: TextView = view.findViewById(R.id.error_text)!!
.also{ it.setTextColor(colorError) }
}