Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have the following ajax call and if it errors, I have a default error message stored in a constant which I'll use if the response doesn't contain a custom message. When I look at this, I keep thinking it could be done better. Can anyone suggest how I can do it better?

function requestService(){
            $.ajax({
                dataType: 'json',
                type: 'POST',
                url: '/myurl',
                contentType: CONTENT_TYPE,
                timeout: AJAX_TIMEOUT,
                success: function(data, textStatus, jqXHR){
                    populateData(JSON.parse(jqXHR.responseText))
                },
                error: function(err){
                    var errorMessage = AJAX_ERROR;
                    try {
                        errorMessage = JSON.parse(err.responseText).messages.messages[0].description;
                    }
                    catch(error) {
                        //There was an problem finding an error message from the server. The default message will be used.
                    }
                    displayError(errorMessage)
                },
                complete: function(){
                    console.log("complete");
                }
            });
        };
share|improve this question
    
Welcome to Code Review! I have changed your title to better say what the code does, rather than that you'd like to improve it (that's the whole intent of the site, after all). I hope you get some great answers! – Phrancis May 7 '15 at 0:11
up vote 2 down vote accepted

I'd say it's pretty much by-the-book already, but you could use the catch block to set the default:

error: function(err){
  var errorMessage;
  try {
    errorMessage = JSON.parse(err.responseText).messages.messages[0].description;
  } catch(exception) {
    errorMessage = AJAX_ERROR;
  }
  displayError(errorMessage);
}

Or, you can use the full try...catch...finally formulation, if you want:

error: function(err){
  var errorMessage;
  try {
    errorMessage = JSON.parse(err.responseText).messages.messages[0].description;
  } catch(exception) {
    errorMessage = AJAX_ERROR;
  } finally {
    displayError(errorMessage);
  }
}

Or you can try getting the custom error message, and only defaulting at the last moment:

error: function(err){
  var errorMessage;
  try {
    errorMessage = JSON.parse(err.responseText).messages.messages[0].description;
  } catch(exception) {
    // ignored
  }
  displayError(errorMessage || AJAX_ERROR);
}

The possible advantage of this, is that if the server for any reason sends a blank (false'y) error message the above will still default to the generic error message, even though no exceptions were thrown.

In any event the overall approach seems OK to me. There's not really a cleaner way of getting the server's error message, since you have to parse the JSON and dig deep into the structure. Either thing may throw exceptions, so try...catch is the simplest way of handling that. At minimum you need the try..catch for JSON.parse anyway, so you might as well use it for both things.

share|improve this answer
    
Thanks, these are some helpful suggestions and I feel more confident having had another pair of eyes on it too! – Leo Farmer May 7 '15 at 1:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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