1
\$\begingroup\$

I have this little piece of front-end code, and I had a little code smell about the handling of errors.

  request
   .post(('/api/comment/:id/upvote').replace(':id', this.comment.id))
      .end(function (err, res) {
      if (err) {
        log('Fetch error: %s', err);
        return classes(targetElement).remove('selected');
      }

      if (res.status == 401) {
        return errorContainer.innerHTML = t(res.body.error);
      }

      if (!res.ok) {
        log('Fetch error: %s', res.error);
        return classes(targetElement).remove('selected');
      }

      if (res.body && res.body.error) {
        log('Fetch response error: %s', res.body.error);
        return classes(targetElement).remove('selected');
      }

      log('successfull upvote %s', self.comment.id);
      });

In all error conditions, I should "rollback" an status of html element.

classes(targetElement).remove('selected');

I can chain all conditions in one if in order to handle errors, but there's still a code smell.

Does someone have some ideas on how to wrap this in a better error handling?

\$\endgroup\$
1
  • \$\begingroup\$ Depending on what the API returns you might be able to go with var error = err || res.error || res.body.error; \$\endgroup\$
    – Johan
    Commented Dec 11, 2014 at 23:17

1 Answer 1

2
\$\begingroup\$

It looks as if you have two things going on: detecting the error, and reporting the error. You have a lot of different error handling; is there a way to abstract this down into a message? Maybe a pair of functions, one of which abstract all of these errors down to a single, page-side error code, and the other of which presents a textual version of that. Or something that returns a tuple.

In the meantime, I've broken it down into "something went wrong" and "what kind of wrongness happened" blocks.

The one thing that stands out at me is you have two different return types: whatever is returned from the function classes(), and the undefined at the end of the function on success. By putting the error handler's return on its own line, I've made sure that js-hint and js-lint won't complain about inconsistent return types.

(function (err, res) {
    if ((err) || (res.status == 401) || (!res.ok) || (res.body && res.body.error)) {
        classes(targetElement).remove('selected');
        if (err || (res.body && res.body.error) || (!res.ok)) {
            log("Fetch error: ", (err || res.error || (res.body && res.body.error)));
        }
        if (res.status == 401) {
            errorContainer.innerHTML = t(res.body.error);
        }
        return;
    }
    log('successfull upvote %s', self.comment.id);
});
\$\endgroup\$
4
  • \$\begingroup\$ Maybe I miss something, but the line with log("Fetch error: ", (err || res.error || (res.body && res.body.error))); returns Fetch error: true/false \$\endgroup\$
    – vmariano
    Commented Dec 11, 2014 at 23:23
  • \$\begingroup\$ Also i think in a similar solution, but I have the same problem, in few lines. \$\endgroup\$
    – vmariano
    Commented Dec 11, 2014 at 23:25
  • \$\begingroup\$ The || function in this context will return the content of the first value that does not evaluate to falsy (false, 0, null, undefined, ""). So if err is undefined but res.error has text, the text will be the first 'or' clause that evaluates to non-false, the text will be the value of the total expression. \$\endgroup\$ Commented Dec 11, 2014 at 23:28
  • \$\begingroup\$ Sorry, I miss the begining of () \$\endgroup\$
    – vmariano
    Commented Dec 12, 2014 at 13:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.