5
\$\begingroup\$

I am building an API where the client can send a url and some comma-delimited tags:

{
   "url": "https://www.youtube.com/watch?v=pquxHIBx8ks",
   "tags": "JavaScript, Sequelize"
} 

Before processing this data, I want to validate the input. To do that, I wrote a middleware function called validateSubmitScreencastReq:

import config from 'config'
import youtubeUrl from 'youtube-url'
import db from 'sequelize-connect'
import * as youtubeClient from '../../source/util/youtubeClient'

async function validateUrl (url) {
  if (!url) {
    return 'url cannot be undefined'
  }

  if (!youtubeUrl.valid(url)) {
    return 'url must be a valid YouTube URL'
  }

  const client = youtubeClient.create(config.youtubeApiKey)
  if (!await client.videoExists(url)) {
    return 'url must link to an existent, public YouTube video'
  }

  const foundScreencast = await db.models.screencast.findOne({
    where: {
      url: url
    }
  })
  if (foundScreencast !== null) {
    return 'url has already been submitted'
  }

  return undefined
}

function validateTags (tags) {
  if (!tags) {
    return 'tags cannot be undefined'
  }

  if (typeof tags !== 'string') {
    return 'tags must be a string'
  }

  return undefined
}

export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const urlError = await validateUrl(req.body.url)
    const tagError = validateTags(req.body.tags)
    const errors = []
    if (urlError) {
      errors.push({
        field: 'url',
        message: urlError
      })
    }
    if (tagError) {
      errors.push({
        field: 'tags',
        message: tagError
      })
    }
    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

I'm not too happy with this code.

The function essentially aggregates error messages, which makes me think of using reduce but I can't see how to make it work elegantly.

What's more, I don't think I'm expressing that the function aggregates errors very clearly.

How can I improve the readability the above code, preferably using functional concepts?

(If it's at all useful, here is the validateSubmitScreencastReq function on GitHub and here are the corresponding unit tests.)

\$\endgroup\$
  • \$\begingroup\$ Hold on, comma-delimited tags? In JSON? Why not just make it a list? (Sorry, did not look at the code, but please: use lists!) \$\endgroup\$ – Sjoerd Job Postmus Jun 4 '16 at 11:36
  • \$\begingroup\$ @SjoerdJobPostmus It's user input. I need to convert it into an array somewhere. I prefer to do it on the server. \$\endgroup\$ – Alex Booker Jun 4 '16 at 11:51
  • \$\begingroup\$ Check for object properties initially if you need to ... if (req.body.hasOwnProperty('url')) { // object has 'url' prop ... ... If it has the property you don't have to worry about the function getting undefined variable passed. ... if (typeof tags !== 'string') { will pass with an empty string (''), why not check for tags.length ... I'm not sure about the framework but the try/catch doesn't have any throws in there. I think it's probably better to just send the first error encountered back and break, but that could depend on what you're trying to do. \$\endgroup\$ – Tomanow Jun 6 '16 at 1:51
1
+50
\$\begingroup\$
export async function validateSubmitScreencastReq (req, res, next) {
  try {
    const errors = [
        {field: 'url', message: await validateUrl(req.body.url)},
        {field: 'tags', message: validateTags(req.body.tags)}
    ].filter((obj) => obj.message !== undefined);

    if (errors.length !== 0) {
      res.status(400).json({errors})
    } else {
      next()
    }
  } catch (error) {
    next(error)
  }
}

This removes any non-errors from the originally constructed array and should give the same end-result as your code.

You can also improve your validate url/valide tag functions:

const assert = require("assert");//common nodejs package

async function validateUrl (url) {
  try {
    assert(url, "url can not be undefined");
    assert(youtubeUrl.valid(url), 'url must be a valid YouTube URL');

    const client = youtubeClient.create(config.youtubeApiKey)

    assert(await client.videoExists(url), 'url must link to an existent, public YouTube video');    
    assert(!(await db.models.screencast.findOne({where: {url: url}})), 'url has already been submitted');
  } catch (error) {
    /* remove the if, if you want to also return whatever those functions may
       have thrown as an error */
    if (error instanceof AssertionError) {
      return error.message;
    }
  }
  //No return needed if you want to return undefined
};
\$\endgroup\$
-1
\$\begingroup\$

Avoiding the '!' and nesting your conditions would be good. They would then throw relevant errors.

if (url) {
    if (youtubeUrl.valid(url)) {
        const client = youtubeClient.create(config.youtubeApiKey)
        if (await client.videoExists(url)) {
            const foundScreencast = await db.models.screencast.findOne({
                where: {
                    url: url
                }
            });
            if (foundScreencast === null) {
                return;
            } else {
                return 'url has already been submitted'
            }
        } else {
            return 'url must link to an existent, public YouTube video'
        }
    } else {
        return 'url must be a valid YouTube URL'
    }
} else {
    return 'url cannot be undefined'
}

No errors would be aggregated then, and only a single error will be thrown. That would be relevant enough. You errors array would be very clean as well.

\$\endgroup\$
  • \$\begingroup\$ A single error is already thrown (or returned)... Nesting like that just makes code hard to read. \$\endgroup\$ – coyotte508 Jun 9 '16 at 17:47

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.