var message = ""
if (XMLHttpRequest.responseText.length > 0)
message = jQuery.parseJSON(XMLHttpRequest.responseText).error
if (!message)
message = t('messages.oh_no_error')
Refactorings
No refactoring yet !
Adam
August 23, 2010, August 23, 2010 19:26, permalink
var message;
if (XMLHttpRequest.responseText.length > 0) {
message = jQuery.parseJSON(XMLHttpRequest.responseText).error;
} else {
message = t('messages.oh_no_error');
}
Ants
August 23, 2010, August 23, 2010 21:14, permalink
@Adam: Your refactoring changes the logic. If the result of jQuery.parseJSON(XMLHttpRequest.responseText).error is an empty or null string, the original code sets the message to t('messages.oh_no_error'), but your code now leaves message as empty or null.
Adam
August 24, 2010, August 24, 2010 16:01, permalink
Good point. Like I have said in the past, all refactorings posted should be required to supply working unit tests. ;)
Robert Dougan
August 26, 2010, August 26, 2010 00:22, permalink
I would do something like this
var message = "",
responseText = XMLHttpRequest.responseText;
if (responseText.length > 0) message = jQuery.parseJSON(responseText).error;
else if (!message) message = t('messages.oh_no_error');
Ants
August 26, 2010, August 26, 2010 18:23, permalink
@Robert: Good job factoring out the responseText, but your code changes the logic the same way @Adam did. If the result of jQuery.parseJSON(responseText).error is an empty or null string, the original code sets the message to t('messages.oh_no_error'), but your code now leaves message as empty or null.
Miguel
August 26, 2010, August 26, 2010 20:24, permalink
I think it works this way
var message = XMLHttpRequest.responseText;
message = (message.length > 0) ? (jQuery.parseJSON(message).error || t('messages.oh_no_error') : "";
aaronhancock.myopenid.com
April 14, 2011, April 14, 2011 19:49, permalink
You don't need to check the length of XMLHttpRequest.responseText for a specific value in this case. You just want to make sure it has something in there.
var message = new String();
if (XMLHttpRequest.responseText.length){
message = jQuery.parseJSON(XMLHttpRequest.responseText).error;
}
(!message) ? message = t('messages.oh_no_error');
aaronhancock.myopenid.com
April 14, 2011, April 14, 2011 19:56, permalink
You don't need to check the length of XMLHttpRequest.responseText for a specific value in this case. You just want to make sure it has something in there.
var message = new String();
if (XMLHttpRequest.responseText.length){
message = jQuery.parseJSON(XMLHttpRequest.responseText).error;
}
(!message) ? message = t('messages.oh_no_error');
Dan Manastireanu
May 6, 2011, May 06, 2011 12:36, permalink
var message = XMLHttpRequest.responseText.length ? jQuery.parseJSON(XMLHttpRequest.responseText).error : '';
message = message || t('messages.oh_no_error');
//As a 1 liner
var message = XMLHttpRequest.responseText.length ? jQuery.parseJSON(XMLHttpRequest.responseText).error || t('messages.oh_no_error'): t('messages.oh_no_error');
How to make this code looks better?