5
\$\begingroup\$

Is there a way I can write this function with less code?

jQuery

function displayError(display, string, xhr) {
    var args = arguments;
    var $error = $("div#error");
    var visible = $error.is(":visible");

    if(args.length == 1 && typeof display === "boolean" && display == false) {
        if(visible) {
            $error.slideUp();
        }
    } else if(args.length == 2 && typeof display === "boolean" && display == true && typeof string == "string") {      
        if(visible) {
            $error.fadeOut(function () { 
                $(this).html('<b style="color: #ce1919;">(!)</b> '+string).fadeIn();
            });
        } else {
            $error.html('<b style="color: #ce1919;">(!)</b> '+string).slideDown();
        }
    } else if(args.length == 3 && typeof display === "boolean" && display == true && typeof string == "string" && typeof xhr === "object") {
        if(visible) {
            $error.fadeOut(function () { 
                $(this).html('<b style="color: #ce1919;">('+xhr.status+")</b> "+string).fadeIn();
            });
        } else {
            $error.html('<b style="color: #ce1919;">('+xhr.status+")</b> "+string).slideDown();
        }
    }
}

Based on Marco's answer I came up with this:

function displayError(display, string, xhr) {
    var $error = $("div#error");
    var errVisible = $error.is(":visible");
    var errStatus = typeof xhr === "object" ? xhr.status : "!";
    var errMsg = '<b style="color: #ce1919;">(' + errStatus + ")</b> " + string;

    if(display) {
        if(errVisible) {
            $error.fadeOut(function() {
                $(this).html(errMsg).fadeIn();
            });
        } else {
            $error.html(errMsg).slideDown();
        }
    } else {
        $error.slideUp();
    }
}
\$\endgroup\$
  • \$\begingroup\$ You should remove the b element and its hard-coded color and move the styling to the CSS. \$\endgroup\$ – RoToRa Mar 11 '14 at 12:52
3
\$\begingroup\$

I don't use Javascript, but i could write it in this way:

function displayError(display, string, xhr) 
{
    var args = arguments;
    var $error = $("div#error");
    var visible = $error.is(":visible");

    if (!display)
    {
        $error.slideUp();
    }
    else
    {

        if (visible) 
        {
            switch (args.length) 
            {
                case 1:
                case 2:
                    $error.fadeOut(function () 
                    {
                        $(this).html('<b style="color: #ce1919;">(!)</b> ' + string).fadeIn();
                    });
                    break;
                case 3:
                    $error.fadeOut(function () 
                    {
                        $(this).html('<b style="color: #ce1919;">(' + xhr.status + ")</b> " + string).fadeIn();
                    });
                    break;
            }
        } 
        else 
        {
            $error.html('<b style="color: #ce1919;">(' + xhr.status + ")</b> " + string).slideDown();
        }
    }
}

But why you use args.length == 1/2/3 if you don't do anything with it? You could just write:

function displayError(display, string, xhr) {
    var $error = $("div#error");
    var visible = $error.is(":visible");
    var status = typeof xhr === "object" ? xhr.status : "!";

    if (display) {
        if (visible) {
            $error.fadeOut(function () {
                $(this).html('<b style="color: #ce1919;">(' + status + ')</b> ' + string).fadeIn();
            });

        } else {
            $error.html('<b style="color: #ce1919;">(' + status + ")</b> " + string).slideDown();
        }
    } else {
        $error.slideUp();
    }
}

I used var status = typeof xhr === "object" ? xhr.status : "!"; so you will get the status if xhr is an object, or ! if not.

I removed all typeof parts since i found it useless, you will always get a string so why you worry about it

\$\endgroup\$
  • \$\begingroup\$ Very nice answer for someone who doesn't use JavaScript, +1 \$\endgroup\$ – konijn Mar 11 '14 at 12:27

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.