Nested Conditional Operators

Cover image by Talles Alves on Unsplash

Internet wisdom says that nested conditional operators are evil. And I firmly believed internet wisdom until today.

Today I found myself refactoring some old Javascript code and it had one of those functions that validate that a heckload of properties exist and have the right format in an object. Then I went crazy and wrote something similar to this.

const validateInput = ({ field1, field2, field3 }) =>
    (!field1
        ? Promise.reject('Missing field1')
        : !Array.isArray(field2)
        ? Promise.reject('field2 is not an array')
        : !isValidType(field3)
        ? Promise.reject('field3 is invalid')
        : Promise.resolve()
    )

Don't mind the thing with promises, this function is used inside a promise chain.

The thing is, it makes me cringe a bit because, you know, nested conditional operators are evil, but I actually find it readable, and I'd say it might even flow better than the chain of ifs.

I would like to read your thoughts and opinions in the topic. Are nested conditional operators inherently evil and unreadable or are they alright, they have just been used in a messy manner for too long?

Comments

[Raphael Amponsah]

WeirdScript:

[Mihail Malo]

Let me tell you the story of how once upon a time I refactored someone's switch cases to a functional expression with nice declarative map lookups:

Old code:

function typeOfSchemaOld(schema: JSONSchema): SCHEMA_TYPE {
  if (schema.allOf) return 'ALL_OF'
  if (schema.anyOf) return 'ANY_OF'
  if (schema.oneOf) return 'ONE_OF'
  if (schema.items) return 'TYPED_ARRAY'
  if (schema.enum && schema.tsEnumNames) return 'NAMED_ENUM'
  if (schema.enum) return 'UNNAMED_ENUM'
  if (schema.$ref) return 'REFERENCE'
  if (Array.isArray(schema.type)) return 'UNION'
  switch (schema.type) {
    case 'string': return 'STRING'
    case 'number': return 'NUMBER'
    case 'integer': return 'NUMBER'
    case 'boolean': return 'BOOLEAN'
    case 'object':
      if (!schema.properties && !isPlainObject(schema)) {
        return 'OBJECT'
      }
      break
    case 'array': return 'UNTYPED_ARRAY'
    case 'null': return 'NULL'
    case 'any': return 'ANY'
  }

  switch (typeof schema.default) {
    case 'boolean': return 'BOOLEAN'
    case 'number': return 'NUMBER'
    case 'string': return 'STRING'
  }
  if (schema.id) return 'NAMED_SCHEMA'
  if (isPlainObject(schema) && Object.keys(schema).length) return 'UNNAMED_SCHEMA'
  return 'ANY'
}

New code

type KnownKeys<T> = {
  [K in keyof T]: string extends K ? never : number extends K ? never : K
} extends { [_ in keyof T]: infer U }
  ? U
  : never
type Strings<T> = Exclude<T, Exclude<T, string>>

type SchemaIdentifier = (schema: JSONSchema) => SCHEMA_TYPE

const propertyMap: [KnownKeys<JSONSchema>, SCHEMA_TYPE | SchemaIdentifier][] = [
  ['allOf', 'ALL_OF'],
  ['anyOf', 'ANY_OF'],
  ['oneOf', 'ONE_OF'],
  ['items', 'TYPED_ARRAY'],
  [
    'enum',
    schema =>
      schema.hasOwnProperty('tsEnumNames') ? 'NAMED_ENUM' : 'UNNAMED_ENUM'
  ],
  ['$ref', 'REFERENCE']
]

const defaultMap: Record<string, SCHEMA_TYPE | undefined> = {
  boolean: 'BOOLEAN',
  number: 'NUMBER',
  string: 'STRING'
}

const anyMoreProps: SchemaIdentifier = schema =>
  !schema.properties && !isPlainObject(schema)
    ? 'OBJECT'
    : defaultMap[typeof schema.default] ||
      (schema.id
        ? 'NAMED_SCHEMA'
        : isPlainObject(schema) && Object.keys(schema).length
          ? 'UNNAMED_SCHEMA'
          : 'ANY')

const typeLookup: Record<
  Strings<JSONSchema['type']>,
  SCHEMA_TYPE | SchemaIdentifier
> = {
  string: 'STRING',
  number: 'NUMBER',
  integer: 'NUMBER',
  boolean: 'BOOLEAN',
  object: anyMoreProps,
  array: 'UNTYPED_ARRAY',
  null: 'NULL',
  any: 'ANY'
}

const typeOfSchemaNew = (schema: JSONSchema): SCHEMA_TYPE => {
  const firstPropertyMatched = propertyMap.find(([key]) =>
    schema.hasOwnProperty(key)
  )
  const val = firstPropertyMatched
    ? firstPropertyMatched[1]
    : schema.hasOwnProperty('type') && schema.type
      ? Array.isArray(schema.type)
        ? 'UNION'
        : typeLookup[schema.type]
      : anyMoreProps
  return typeof val === 'string' ? val : val(schema)
}

👍 👍 👍 Too bad it was rejected 👍 👍 👍

[Mihail Malo]

I would format it like this:

const validateInput = ({ field1, field2, field3 }) =>
  !field1
    ? Promise.reject('Missing field1')
    : !Array.isArray(field2)
      ? Promise.reject('field2 is not an array')
      : !isValidType(field3)
        ? Promise.reject('field3 is invalid')
        : Promise.resolve()

To my disappointment, Prettier does exactly that when used to format selection but flattens it when using format document. What gives?!
Disclaimer: it originally didn't get rid of the parentheses, but after I removed them it didn't reinsert them.

However, you are messing around with Promises every single return here, so maybe the real solution here is the an async function?

const validateInput = async ({ field1, field2, field3 }) => {
  if (!field1) throw "Missing field1"
  if (!Array.isArray(field2)) throw "field2 is not an array"
  if (!isValidType(field3)) throw "field3 is invalid"
}

Wow, this is amazing :O And shorter! And will produce less diffs on refactoring!

However, do we actually need to return promises? Surely not!

const validateInput = ({ field1, field2, field3 }) => {
  if (!field1) return "Missing field1"
  if (!Array.isArray(field2)) return "field2 is not an array"
  if (!isValidType(field3)) return "field3 is invalid"
}

// we can get a promise later if we really need to
const asyncValidateInput1 = async obj => {
  const errStr = validateInput(obj)
  if (errStr) throw errStr
}
// and your conditional operators approach
const asyncValidateInput2 = obj => {
  const errStr = validateInput(obj)
  return errStr ? Promise.reject(errStr) : Promise.resolve()
}

But do we ever? Nah.

const assertInput = obj => {
  const errStr = validateInput(obj)
  if (errStr) throw errStr
  return obj
}

getObj.then(assertInput).then(/* safely use `obj` here */)

If someone actually reads this long-azz comment, yes, you should usually wrap what you're rejecting with into a new Error(str), for instance in the assertInput func, but this might be an acceptable exception.

[bruno da silva]

I don't know for js but in PHP, the evaluation is non trivial (left to right):

stackoverflow.com/a/6224398

[Fred Ross]

I think that if I were to run across this while digging through code, I would be much happier to just see

const validateInput = ({ field1, field2, field3 }) => {
    if (!field1) {
        return Promise.reject('Missing field1');
    }
    if (!Array.isArray(field2)) {
        return Promise.reject('field2 is not an array');
    }
    if (!isValidType(field3)) {
        return Promise.reject('field3 is invalid');
    }
    return Promise.resolve();
}

If definitely doesn't win any code golf or cleverness competitions, but it imposes an absolute minimum of cognitive load on someone passing through the code.

[Marcel Cremer]

In my opinion the main problem in this particular case is not the tenary operator - it's the ugly code that results from the violation of the DRY principle at the "error throwing".

Let's break this down a bit:

• You have several "I validate a field"-things going on.
• each of them will reject the "overall"-Promise, when the validation is not valid.
• if every field validation is successfull, the Promise is resolved.

Why don't you just tell your program exactly this idea?

"I validate a field" becomes a function, that rejects a Promise on error, or resolves it if everything is fine:

// async can be Replaced with new Promise(bla)
validateField = async (condition, message) => {
    if(!condition)
        throw new Error(message);
}

Now we can set a condition and get a resolved / rejected Promise back - we just need to execute all of them (with Promise.all) and use the overall success as indicator, that all validation was successful:

const validateForm = ({ field1, field2, field3 }) => Promise.all([
    validateField(!!field1, 'Missing field1'),
    validateField(Array.isArray(field2), 'field 2 is not an Array'),
    validateField(!isValidType(field3), 'field3 is invalid')
]);

I think that this increases readability by several levels and tells us on the first glance, what it's doing. You might even add other fancy stuff in the "validateField"-Function, which applies to all validations, or you can use the "validateField"-part without calling the validateForm-Function.

I think if I found your piece of code, I would've refactored it like that and I can imagine, other places where "multiple" tenarys would be needed can be refactored like that as well.

What do you think about this attempt?

[Avalander]

Using Promise.all is a very interesting approach, I like it.

[Nested Software]

validateField seems reasonable, but I am not sure why it is asynchronous. All of the field information seems to be available as-is, so it looks as though validateField and validateInput could be simplified:

const main = () => {
    const obj = {
        field2: [1,2,3],
        field3: 'joe'
    }

    result = validateInput(obj)

    result
        .then(success=>console.log('validation was successful'))
        .catch(error=>console.log(`${error}`))
}

const validateInput = ({field1, field2, field3}) => {
    try {
        validateField(exists(field1), 'Missing field1')
        validateField(Array.isArray(field2), 'field 2 is not an Array')
        validateField(isValidType(field3), 'field3 is invalid')

        return Promise.resolve()
    } catch (error) {
        return Promise.reject(error)
    }
}

const validateField = (condition, message) => {
    if (!condition)
        throw new Error(message)
}

//implemented as a stub
const isValidType = field => true

//not sure if there needs to be a distinction between boolean
//fields that are set to false, fields that are set to null or empty string, 
//and fields that simply don't exist in the object
const exists = field => field != undefined

main() //Error: Missing field1

Also, I'm not sure why there are negations in !!field1 and !isValidType(field3).

[Marcel Cremer]

I just made it async, because I was too lazy to write new Promise bla (async always returns a Promise). Also I used Promise.all because I guessed that Avalander had some fancy async validation stuff going on.

!!field1 casts a value to a boolean (a bit hacky, I know 😉 Just wanted to point out, that someone should provide a boolean value).

!isValidType is taken from the original post - of course non-negated conditions should be preferred.

[Avalander]

Yeah, let's not forget that the code I posted is a simplification of the real problem, but we can ignore the asynchronous part, I just wanted to discuss the chained conditional operators.

[Nested Software]

Just wondering, how come this code only checks the first invalid input, if they are all independent? Does that mean in this domain it’s not useful to find all of the invalid inputs at once?

[Avalander]

Let's keep in mind that I was refactoring old code, I wouldn't design a validation system like this to start with. I kept it like this however because I didn't want to change any functionality during the refactor, and this code is in a request handler in a web service and the client can't handle a response with multiple errors anyway.

[Nested Software]

No worries. In that case I think I’d prefer just (pseudocode):

if (invalid1)
    return error1
if (invalid2)
    return error2
etc...

I think it’s more straightforward.

Update:

The use of the ternary operator for this seems possible, but in my opinion it's leaning in the direction of being cute for cute's sake rather than really being useful. At the very least I would argue it's not idiomatic js, and if there is a benefit to it at all, I don't think it's significant enough. In general I like the idea of using the idiomatic approach for a given language unless there is really quite a strong reason not to.

All that being said, if you and your team are comfortable with this code, I don't think using it is the end of the world.

[Satvik Sharma]

If you want to write this style of code, I'd prefer using cond from lodash which reimplements cond from Lisp.

const validateInput = _.cond([
    [({ field1 }) => !field1, Promise.reject('Missing field 1')],
    [({ field2 }) => !Array.isArray(field2), Promise.reject('field2 is not an array')],
    [({ field3 }) => !isValidType(field3), Promise.reject('field3 is invalid')],
    [_.stubTrue, Promise.resolve()]
]);

Edit: didn't realise it was an object, not an array.

Could even do something like this:

const isMissing = key => obj => !obj[key];
const notArray = key => obj => !Array.isArray(obj[key]);
const notType = (type, key) => obj => typeof obj[key] !== type;

const validateInput = _.cond([
    [isMissing('field1'), Promise.reject('Missing field 1')],
    [notArray('field2'), Promise.reject('field2 is not an array')],
    [notType('string', 'field3'), Promise.reject('field3 is invalid')],
    [_.stubTrue, Promise.resolve()]
]);

[Avalander]

This is a great suggestion, I actually ended up doing something similar with Ramda's cond

[Dustin King]

Indentation that follows the level of nesting would help me understand what is happening:

const validateInput = ({ field1, field2, field3 }) =>
    (!field1
        ? Promise.reject('Missing field1')
        : !Array.isArray(field2)
            ? Promise.reject('field2 is not an array')
            : !isValidType(field3)
                ? Promise.reject('field3 is invalid')
                : Promise.resolve()
    )

I'm not sure I got the evaluation order right, and I'd be suspicious about it in somebody else's code. Usually I'd use parentheses to make sure, but that might make it less readable with a lot of nesting.

[loctv]

In general, I will never use nested conditional operator, simply because it looks ugly (to me) and it’s hard to debug. Flatten out your code is the best way for readable and maintainable code.