Node refactor if/else mess

Question

I'd love some pointers on how to refactor this

async function validate_request_args(req, request_type) {
    let problem_id;
    const errors = [];
    switch (request_type) {
        case "post":
            if (req.body.id == null) {
                errors.push({
                    msg: "Missing id of the problem",
                    param: "id"
                });
            } else {
                problem_id = req.body.id;
            }
            break;

        case "get":
            if (req.query.id == null) {
                errors.push({
                    msg: "Missing id of the problem",
                    param: "id"
                })
            } else {
                problem_id = req.query.id;
            }
            break;
    }
    if (errors.length === 0) {
        const problem = await models.Problem.findByPk(problem_id);
        if (problem == null) {
            errors.push({
                msg: `Problem with id ${problem_id} does not exist `,
                param: "id"
            });
        }
        return monet.Validation.success(problem)
    }
    return monet.Validation.fail(errors);
}

Answer 1

To start with, I see what's likely to be a logic error. At the bottom, you do:

if (problem == null) {
    errors.push({
        msg: `Problem with id ${problem_id} does not exist `,
        param: "id"
    });
}
return monet.Validation.success(problem)

If the problem isn't found, you push an error object to the errors array, but then you return .success anyway and don't use the errors array again. In such a case, I think you want to call monet.Validation.fail with the error object instead, and not call .success.

Once that's done, note that you only ever have at most one element in the errors array, which .fail gets called with. It would be less repetitive to immediately call .fail and return when an error is encountered. Also, since the fail calls are all similar (monet.Validation.fail([{ msg: SOME_MESSAGE, param: 'id' }])), you can make the code DRY by putting that into a helper function:

const fail = msg => monet.Validation.fail([{ msg, param: 'id' }]);

Also, switch is almost never the right tool for the job IMO - it's quite verbose and can be error-prone when one forgets to break (which is surprisingly common). If the request type will be either post or get, use the conditional operator to figure out the property to look up on req instead:

const { id } = req[requestType === 'post' ? 'body' : 'query'];

I'd highly recommend against using loose equality ==. It requires an expectation that any reader of the code understands its strange rules. Better to use strict equality. Or, if the id or problem, if it exists, will be truthy (which sounds likely), it would look even nicer to use a truthy test:

if (!id) {
    return fail('Missing id of the problem');
}

If you actually need the logic you're implementing with == to check that the expressions aren't null and aren't undefined (but could be anything else, including other falsey values), then best to test both of those explicitly:

if (id === null || id === undefined) {
    return fail('Missing id of the problem');
}

I think it would also be useful to follow the standard Javascript naming conventions and use camelCase for variable names in most cases.

All together:

const fail = msg => monet.Validation.fail([{ msg, param: 'id' }]);
async function validateRequestArgs(req, requestType) {
    const { id } = req[requestType === 'post' ? 'body' : 'query'];
    if (!id) {
        return fail('Missing id of the problem');
    }
    const problem = await models.Problem.findByPk(id);
    if (!problem) {
        return fail(`Problem with id ${id} does not exist`);
    }
    return monet.Validation.success(problem);
}