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\$
  • \$\begingroup\$ Depending on what the API returns you might be able to go with var error = err || res.error || res.body.error; \$\endgroup\$ – Johan Dec 11 '14 at 23:17
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\$
  • \$\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 Dec 11 '14 at 23:23
  • \$\begingroup\$ Also i think in a similar solution, but I have the same problem, in few lines. \$\endgroup\$ – vmariano Dec 11 '14 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\$ – Elf Sternberg Dec 11 '14 at 23:28
  • \$\begingroup\$ Sorry, I miss the begining of () \$\endgroup\$ – vmariano Dec 12 '14 at 13:50

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.