2
\$\begingroup\$

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();

}
\$\endgroup\$
5
  • \$\begingroup\$ I'm only here for the Kotlin... Would it be an error if the input or error would be absent? \$\endgroup\$ – tieskedh May 6 '20 at 13:47
  • \$\begingroup\$ Also it seems you're missing some code in UsersActivity... (the surrounding class eg.) \$\endgroup\$ – tieskedh May 6 '20 at 13:56
  • \$\begingroup\$ I left here only the code that interests me for verification.input NotNull he cannot be absent.What means error would be absent? \$\endgroup\$ – Destroyer May 6 '20 at 14:00
  • \$\begingroup\$ I mean should findViewById always find the view, or would it be ok if it didn't? \$\endgroup\$ – tieskedh May 6 '20 at 14:07
  • \$\begingroup\$ ok can not be found, cancel is required \$\endgroup\$ – Destroyer May 6 '20 at 15:37
1
\$\begingroup\$

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) }
}
\$\endgroup\$

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.