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);
}
request_type
always bepost
orget
, or could it be something else likeput
? \$\endgroup\$