3
\$\begingroup\$

Code

if (source.length > 0) {
    if (
        source[0].mandatory === true &&
        this.allInputs.documentsType !== "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    } else if (
        source[0].mandatory === false &&
        this.allInputs.documentsType === "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    }
} else {
    if (
        target[0].mandatory === true &&
        this.allInputs.documentsType === "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    } else if (
        target[0].mandatory === false &&
        this.allInputs.documentsType !== "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    }
}

All is working, the problem I have is that this code is not very readable. Can someone please help me refactor this?

Thanks

\$\endgroup\$
2
  • 4
    \$\begingroup\$ I would leave it as is since there are already answers here, but in the future it would benefit reviewers to have a bit more information about the code in the description. From the help center page How to ask: "You will get more insightful reviews if you not only provide your code, but also give an explanation of what it does. The more detail, the better." \$\endgroup\$ May 19 at 15:39
  • 2
    \$\begingroup\$ We need to know what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task, rather than your concerns about the code. \$\endgroup\$ May 20 at 18:49
4
\$\begingroup\$

I don't know much javascript or typescript. But this seems to be a basic problem.

So, I will give it a try in phased manner:

Level 1: Clean up, by merging conditions

if (source.length > 0) {
    if ((source[0].mandatory === true &&
        this.allInputs.documentsType !== "mandatory") || 
        (source[0].mandatory === false &&
        this.allInputs.documentsType === "mandatory"))
    {
        item.mandatory = !item.mandatory;
    }
} else {
    if ((target[0].mandatory === true &&
        this.allInputs.documentsType === "mandatory") ||
        (target[0].mandatory === false &&
        this.allInputs.documentsType !== "mandatory"))
    {
        item.mandatory = !item.mandatory;
    }
}

Level 2: Merging more aggressively

if ((source.length > 0 && ((source[0].mandatory === true &&
    this.allInputs.documentsType !== "mandatory") || 
    (source[0].mandatory === false &&
    this.allInputs.documentsType === "mandatory"))) ||
    ((target[0].mandatory === true &&
    this.allInputs.documentsType === "mandatory") ||
    (target[0].mandatory === false &&
    this.allInputs.documentsType !== "mandatory"))) 
{
    item.mandatory = !item.mandatory;
}

Level 3: Using generic type variable, to clean up repeating conditions Edit: I would have liked to simply this more but this assumes both source and target to be same which are not.

var obj = null;
if (source.length > 0) {
    obj = source[0];
} else {
    obj = target[0];
}

if ((obj.mandatory === true &&
    this.allInputs.documentsType !== "mandatory") ||
    (obj.mandatory === false &&
    this.allInputs.documentsType === "mandatory"))
{
    item.mandatory = !item.mandatory;
}

I hope you understand what I am suggesting you to do.

Edit: As correctly suggested by Scott my last optimization is wrongly treating source and target as same, please pardon my mistake.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ I think there's a bug in your final revision. Looking closely, the o.p. doesn't treat source[0].mandatory exactly the same as target[0].mandatory. e.g. The o.p. checks source[0].mandatory === true && documentType !== 'mandatory', which is different from th other check target[0].mandatory === true && documentType === 'mandatory' \$\endgroup\$ May 19 at 13:54
  • \$\begingroup\$ Oh.. yes thanks for pointing that out. \$\endgroup\$
    – mangupt
    May 19 at 15:33
  • \$\begingroup\$ You should comment the actual code block to point out earlier that there's a bug, before readers have a chance to get hung up on it themselves. Don't just leave an "edit: previous stuff was wrong" at the bottom, that's not nearly as good. You could present that 3rd version as "you'd like your code to look something like this if possible, but this actually implements slightly different logic". \$\endgroup\$ May 19 at 17:22
  • 1
    \$\begingroup\$ this.allInputs.documentsType === "mandatory" and !== are inverses of each other, right? So you can presumably use obj.mandatory to invert the compare result or not, or check mandatory != (compare) or something. In fact that's what Scotty Jamison's answer is doing. \$\endgroup\$ May 19 at 17:25
  • \$\begingroup\$ Yes, I agree Scotty's answer is more refined. Also, I have added the suggested comment on top. \$\endgroup\$
    – mangupt
    May 21 at 7:53
11
\$\begingroup\$

To make it easier to follow the flow of the logic, I'm just going to pull out the longer parts of these conditions into helper variables. They may not stay like that in the final version, but it helps as I'm revising it.

const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';

if (source.length > 0) {
    const mandatory = source[0].mandatory;
    if (mandatory === true && !isDocumentTypeMandatory) {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && isDocumentTypeMandatory ) {
        item.mandatory = !item.mandatory;
    }
} else {
    const mandatory = target[0].mandatory;
    if (mandatory === true && isDocumentTypeMandatory) {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && !isDocumentTypeMandatory) {
        item.mandatory = !item.mandatory;
    }
}

The trick to simplifying these kinds of things is to figure out what is similar, figure out how to change the code to make those parts exactly the same, then DRY it up.

For example, I'm noticing that the two branches of the outer if-else are almost exactly the same. The only differences are where it gets the mandatory variable from, and the ! that is sometimes in front of isDocumentTypeMandatory.

Lets start by extracting out the diferences from the two branches:

const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;

if (source.length > 0) {
    if (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && isDocumentTypeMandatory === sourceHasContent) {
        item.mandatory = !item.mandatory;
    }
} else {
    if (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && isDocumentTypeMandatory === sourceHasContent) {
        item.mandatory = !item.mandatory;
    }
}

Now those the two branches of the outer-if are now exactly the same, which lets me just get rid of it.

const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;

if (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) {
    item.mandatory = !item.mandatory;
} else if (mandatory === false && isDocumentTypeMandatory === sourceHasContent) {
    item.mandatory = !item.mandatory;
}

At this point, we can just combine the two conditions into one. I personally prefer avoiding multiline if conditions and instead extract the condition into an external variable.

const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;

const flipMandatoryValue = (
    (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) ||
    (mandatory === false && isDocumentTypeMandatory === sourceHasContent)
);

if (flipMandatoryValue) {
    item.mandatory = !item.mandatory;
}

This is probably about as good as it'll get. Certainly, the variable naming can be a ton better (without much context, its hard to give them good names).


There is one other trick that can be applied to simplify things even further, but there's already so much going on here that it might just make the code even less readable, but I'll present it anyways.

This whole time I've been assuming that mandatory can take on other values besides "true" or "false", which is why you're explicitly checking if they're === true or === false. In the following revisions, I'll preserve this idea by doing a typeof check. I'll then modify the condition as follows:

const flipMandatoryValue = typeof mandatory === 'boolean' && (
    (mandatory && isDocumentTypeMandatory !== sourceHasContent) ||
    (!mandatory && isDocumentTypeMandatory === sourceHasContent)
);

// is the same as
const subCondition = isDocumentTypeMandatory !== sourceHasContent;
const flipMandatoryValue = typeof mandatory === 'boolean' && (
    (mandatory && subCondition) ||
    (!mandatory && !subCondition)
);

// is the same as
const subCondition = isDocumentTypeMandatory !== sourceHasContent;
const flipMandatoryValue = typeof mandatory === 'boolean' && (
    // Either both of these booleans should be true, or both should be false,
    // so we can just compare them to each other with ===.
    mandatory === subCondition
);

// is the same as
const subCondition = isDocumentTypeMandatory !== sourceHasContent;
// We can drop the typeof check - we're comparing with a boolean anyways
const flipMandatoryValue = mandatory === subCondition;

This gives us the final revision as follows:

const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;
const subCondition = isDocumentTypeMandatory !== sourceHasContent;

if (mandatory === subCondition) {
    item.mandatory = !item.mandatory;
}

As I said before, using === to compare booleans can be a useful trick to simplify complicated if-thens, but here I feel like it adds more complexity than value, unless you can find good ways to name everything and make it more intuitive.

\$\endgroup\$
0
5
\$\begingroup\$

Assuming that the values of source[0].mandatory and target[0].mandatory are booleans (true or false), you can make the code a lot shorter by comparing the truthiness of your expressions:

const docTypeMandatory = this.allInputs.documentsType === "mandatory";
if (source.length > 0) {
    if (source[0].mandatory === !docTypeMandatory) {
        item.mandatory = !item.mandatory;
    }
} else {
    if (target[0].mandatory === docTypeMandatory) {
        item.mandatory = !item.mandatory;
    }
}

This also helps highlight the flipping of logic between the two main cases, which was otherwise a bit buried.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ The conditions should be reversed: source[0].mandatory === !docTypeMandatory \$\endgroup\$
    – adiga
    May 20 at 6:57
4
\$\begingroup\$

Not a review

Just a point that expressions are preferable over statements. You can use a ternary and test a boolean source[0].mandatory or target[0].mandatory against a boolean result this.allInputs.documentsType === "mandatory".

const mandatory = this.allInputs.documentsType === "mandatory";
item.mandatory = source.length ?
    (source[0].mandatory !== mandatory ? !item.mandatory : item.mandatory) :
    (target[0].mandatory === mandatory ? !item.mandatory : item.mandatory);

Which can be written as statements

const mandatory = this.allInputs.documentsType === "mandatory";
if (source.length) {
    if (source[0].mandatory !== mandatory) {
        item.mandatory = !item.mandatory;
    }
} else if (target[0].mandatory === mandatory) {
    item.mandatory = !item.mandatory;
}

or

const mandatory = this.allInputs.documentsType === "mandatory";
if (source.length) {
    if (source[0].mandatory !== mandatory) { item.mandatory = !item.mandatory }
} else if (target[0].mandatory === mandatory) { item.mandatory = !item.mandatory }

There are even shorter ways to write this, but that will begin to obscure what is happening.

const mandatory = this.allInputs.documentsType === "mandatory";
item.mandatory =  (source[0]?.mandatory ?? !target[0].mandatory) !== mandatory ? 
    !item.mandatory : 
    item.mandatory;
\$\endgroup\$
5
  • \$\begingroup\$ I think that logic towards the end is incorrect. Whether the test item in question is source or target inverts the test in consideration. \$\endgroup\$ May 20 at 1:04
  • \$\begingroup\$ @SteveBennett That is why target[0].mandatory has a ! in front of it. \$\endgroup\$
    – Blindman67
    May 20 at 1:54
  • 1
    \$\begingroup\$ IMHO, ternaries should only be used in simple cases. Nested ternaries are generally more confusing than nested if/then. \$\endgroup\$
    – Barmar
    May 20 at 15:19
  • \$\begingroup\$ @Barmar Your comment IMHO sounds very like doctrine. I will not insult the intelligence of readers by avoiding standard language features (someone declared confusing). Surely you are not suggesting there are coders that are confused by code? I am more than sure you understand ternaries, you know they are easy to read so I ask why are they "...more confusing..."? To whom are they "...more confusing..." ? Rhetorical I have heard the less than water tight arguments way too many times. Lets treat readers as professionals, not as 1st year CS undergrads... :) \$\endgroup\$
    – Blindman67
    May 20 at 16:26
  • \$\begingroup\$ I was very clear to preface it as personal opinion. Ternaries are fine, but can be abused like any terse notation. \$\endgroup\$
    – Barmar
    May 20 at 16:33
4
\$\begingroup\$

This is based on @Steve Bennett's answer.

  • You can skip the source.length check using optional chaining (?.).
  • Since both the if conditions have the same block of code, you can club them together with an ||
const isDocTypeMandatory = this.allInputs.documentsType === "mandatory";

if (source[0]?.mandatory === !isDocTypeMandatory
    || target[0].mandatory === isDocTypeMandatory) 
{
  item.mandatory = !item.mandatory;
}
\$\endgroup\$
1
  • \$\begingroup\$ Yeah, this is starting to get a little bit hairy though, because it relies on the fact that source[0]?.mandatory will evaluate to undefined and won't match false. Which is starting to get less readable again. \$\endgroup\$ May 21 at 0:58
2
\$\begingroup\$

I've taken a slightly different approach from what seems to have been answered here, so I decided to add an answer to already good answers.

What you have is a boolean algebra problem, where you have four variables that express a result.

The variables are :

sourceIsNonZero = source.length > 0;
sourceIsMandatory = source[0].mandatory === true;
targetIsMandatory = target[0].mandatory === true;
docTypeIsMandatory = documentsType === "mandatory";

I've chosen to express the output as : Should the item's mandatory value be flipped?

Since there are four boolean variables, we have a possibility of \$2^4=16\$ combinations, which isn't so bad. To try and reduce this boolean, I've decided to use the good old Truth Table, which works well for small number of combinations.

Truth table

Then, we can observe the truth table to find out a boolean expression that summarizes the problem, in this case :

(sourceIsNonZero && (sourceIsMandatory !== docTypeIsMandatory)) || (targetIsMandatory === docTypeIsMandatory)

Considering this, this is the solution I would propose :

sourceIsNonZero = source.length > 0;
sourceIsMandatory = source[0].mandatory === true;
targetIsMandatory = target[0].mandatory === true;
docTypeIsMandatory = documentsType === "mandatory";

shouldMandatoryAttributeBeFlipped = (sourceIsNonZero && (sourceIsMandatory !== docTypeIsMandatory)) 
                                    || (targetIsMandatory === docTypeIsMandatory)

if (shouldMandatoryAttributeBeFlipped) {
    item.mandatory = !item.mandatory;
}

Edit

I first used && to signify "are the same", which is... dumb from my part. What we want to check isn't that they are both true, but that they are different in the case of the source check and the same in the case of the target check. I changed && to ===.

As pointed out in the comments, invoking source[0].mandatory if source < length == 0 will crash, in this case I would either recommend joining the two conditions : sourceIsNonZeroAndMandatory = source.length > 0 && source[0].mandatory or use the null propagation operator on the second condition source[0]?.mandatory.

end edit

While it's a little bit more "wordy" than other propositions, I think this solution is readable, presents in clear manner the important variables and the way the output is decided.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Most likely you cannot invoke source[0] if source.length is zero \$\endgroup\$ May 20 at 17:07
  • \$\begingroup\$ @მამუკაჯიბლაძე very true. Let me change that \$\endgroup\$
    – IEatBagels
    May 20 at 17:42
  • \$\begingroup\$ Nightmare once you want to debug.. \$\endgroup\$ May 20 at 18:39
  • \$\begingroup\$ @KorayTugay I can only disagree. With the quality of the debugging tools available and the fact that every condition is clearly named, you'd find very quickly where the problem lies. \$\endgroup\$
    – IEatBagels
    May 20 at 18:41
  • \$\begingroup\$ You final deduction is only solving half of the problem - when sourceIsManditory/targetIsManditory === true, but what about when they === false? Take the first row of your truth table for example - It's supposed to result in "true", but your boolean expression would give "false". \$\endgroup\$ May 21 at 13:37

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.