2
\$\begingroup\$

I am working on saving the text messaging draft of a user. The flow currently works but I am looking to make this block of code easier to read and smaller. Could anyone help me achieve this?

if (!hasDraft) {
    /**
     * This will get triggered when there is no draft
     * and you create one therefore we need a refresh.
     */
    if (!TextUtils.isEmpty(enteredText)) {
        saveDraftAndRefreshList();
    } else {
        /**
         * This should be the happy case.
         * User has just sent a message so no need to refresh the list
         */
        sendMessage();
    }
 /**
  * This else will only be reached if there has been a draft 
  **/
 } else {
     if (TextUtils.isEmpty(enteredText)) {
         /**
          * This will get reached if you had a draft but now removed the text.
          * Thus needing a refresh
          */
         removePreviousDraft();
    } else if (!TextUtils.isEmpty(enteredText)) {
        /**
         * This will get triggered if there was a draft and
         * now they decided to overwrite it.
         */
        overwritePreviousDraft();
    }
}
\$\endgroup\$

closed as off-topic by tinstaafl, Sᴀᴍ Onᴇᴌᴀ, Mast, Heslacher, Toby Speight Jan 28 at 9:26

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – tinstaafl, Sᴀᴍ Onᴇᴌᴀ, Mast, Toby Speight
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does and in what context it's used. \$\endgroup\$ – Mast Jan 27 at 4:58
1
\$\begingroup\$

In both cases (!hasDraft and not !hasDraft) you check whether the enteredText is empty. You can hoist that into a boolean variable outside the if statement.

Also, you are testing if (!hasDraft) but then also doing an else on that condition, which means else (if not)not hasDraft. That can be confusing, so unless some other part of your code flow requires it, I suggest you make your if statement be if (hasDraft) {} else {} so that the implied not from the else matches the !hasDraft case.

boolean noEnteredText = TextUtils.isEmpty(enteredText);

if (hasDraft) {
    if (noEnteredText) {
        removePreviousDraft();
    else {
        overwritePreviousDraft();
    }
} else {
    if (noEnteredText) {
        sendMessage();
    } else {
        saveDraftAndRefreshList();
    }
}

This does not read well, IMO. Again, though, it might be fine in the context of your other code. But I wonder if reversing the sense of noEnteredText to be hasEnteredText would make it better?

boolean hasEnteredText = !TextUtils.isEmpty(enteredText);

if (hasDraft) {
    if (hasEnteredText) {
        overwritePreviousDraft();
    else {
        removePreviousDraft();
    }
} else {
    if (hasEnteredText) {
        saveDraftAndRefreshList();
    } else {
        sendMessage();
    }
}

This still seems off. But I suspect that's probably because I don't know your app. Is it possible you're trying to do too much, and the two decisions should be at two different levels of method call?

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.