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.
