6
\$\begingroup\$

Be as harsh as you can! I'm trying to improve my coding style and would like to know if there is anything I could improve.

EDIT: To make this fair, The person who provides the most helpful tips (both in quantity and quality) will get marked as the answer

The code below is the JavaScript file for a CMS application I'm creating:

When the application is complete it will be Open Source for anyone to use.. I'll keep you guys updated if you're interested in trying it out

$(function () {

    loadPage(htmlLandingPage, currentSiteName) //added on by previous page
    $("#dashBody").css('top', $("#topDash").outerHeight());
    $("#dashBody").hide()




    $('#dashBody').load(function () {
        iFrameContents.find("head").append($("<link/>",
            {
                rel: "stylesheet",
                href: "/cmsCSS/loadedPageCSS.css"
            }
            ));
        $(window).on("scroll", function () {

            iFrameContents.find("HTML,body").scrollTop($(window).scrollTop());
        });



        iFrameContents.on("click", ".cmsEdit", function (e) {
            e.stopPropagation();
            selectedElem = $(this)
            if (e.ctrlKey) {
                e.preventDefault();
                if (!$(e.target).is('.inEdit *') && !$(e.target).hasClass('inEdit')) {

                    clearEditSection()
                }
                if (!$(this).hasClass("inEdit")) {
                    $.when(checkOutStatus(selectedElem.prop("id"))).done(function (result) {
                        if (!result.d) {
                            checkOutSection(selectedElem.prop("id"))
                            selectedElem.addClass("inEdit")

                            selectedElem.prepend($("<div class='cmsEditBox'/>"));
                            for (i = 0; i < editBoxButtons.length; i++) {
                                $("<button/>", {
                                    html: editBoxButtons[i],
                                    addClass: "editButtons",
                                    click: function () { editButtonAction(this, selectedElem) }
                                }).appendTo(selectedElem.children(".cmsEditBox"))
                            }
                            selectedElem.children(".cmsEditBox").css('top', (selectedElem.children(".cmsEditBox").height() * -1) - 10)
                            selectedElem.children(".cmsEditBox").css('left', ((selectedElem.children(".cmsEditBox").width() / 8) * -1))
                            selectedElem.children(".cmsEditBox").prop('contenteditable', false)
                            selectedElem.prop('contenteditable', true)
                            currentSectionHTML = selectedElem.clone().children(".cmsEditBox").remove().end().html();
                        } else {
                            $.when($(".loadProgress").fadeOut(100)).done(function () {
                                alert("Section Currently Checked Out")
                            });

                        }
                    });
                    }
                 }



        });


        iFrameContents.find('*').on("click", function (e) {

            if ($(".inEdit").length > 0) {
                e.stopPropagation();
            }
            if (!e.ctrlKey) {
                    if (!$(e.target).is('.inEdit *') && !$(e.target).hasClass('inEdit')) {
                        clearEditSection()
                    }
                }



        });

        currentPageID = iFrameContents.find("body").prop("id")
        if (!currentPageID) {
            $("<div/>", {
                addClass: "loadProgress",
                html: $("<span/>", { addClass: "loadingText", html: "Missing<br/>Body<br/>ID" })
            }).appendTo("#topDash").hide().fadeIn();
            return false
        }
        checkDBforPage(currentPageID, currentSiteName)


    });



});


function editButtonAction(btnText, selectedElem) { // Add Button Functions Here
    buttonText = $(btnText).text()
    if (buttonText == "SAVE") {
        saveSection(selectedElem.prop("id"), selectedElem.clone().children(".cmsEditBox").remove().end().html())
    }
    if (buttonText == "CANCEL CHANGE") {
        selectedElem.html(currentSectionHTML)
    }
    if (buttonText == "REVERT TO ORIGINAL") {
        revertSection(selectedElem,selectedElem.prop("id"), selectedElem.clone().children(".cmsEditBox").remove().end().html())
    }
    if (buttonText == "SUBMIT CHANGE") {
        submitSection(selectedElem, selectedElem.prop("id"), selectedElem.clone().children(".cmsEditBox").remove().end().html())
    }
}



function checkPageSectionsInDB(pageID) {

    $.ajax({
        type: "POST",
        url: "default.aspx/checkPageSections",
        data: "{'pageID': " + JSON.stringify(pageID) + ",'sitePageID': " + JSON.stringify(sitePageID) + ",'siteContent': " + JSON.stringify(editableSections) + "}",
        contentType: "application/json; charset=utf-8",
        beforeSend: function () {
            $(".loadingText").html("Analyzing <br/> Sections<br/> In DB")

        },
        dataType: "json",
        success: function (data) {
            $(".loadingText").html("Done!")
            $(".loadProgress").fadeOut();         
        },
        error: function (request, status, errorThrown) {
            $(".loadingText").html("Error <br/>  Checking<br/> Sections")
        }

    });
}



function saveSection(sectionID,sectionContent) {
    $.ajax({
        type: "POST",
        url: "default.aspx/saveSection",
        data: "{'sectionID': " + JSON.stringify(sectionID) + ",'personID': " + JSON.stringify(personID) + ",'sitePageID': " + JSON.stringify(sitePageID) + ",'sectionContent': " + JSON.stringify(sectionContent) + "}",
        contentType: "application/json; charset=utf-8",
        beforeSend: function () {
            $(".loadProgress").fadeIn();
            $(".loadingText").html("Saving..")

        },
        dataType: "json",
        success: function (data) {
            $(".loadProgress").fadeOut()
        },
        error: function (request, status, errorThrown) {
            $(".loadingText").html("Error<br/>Saving")
            return false
        }

    });
}

function revertSection(sectionSelected,sectionID, sectionContent) {
    $.ajax({
        type: "POST",
        url: "default.aspx/revertSection",
        data: "{'sectionID': " + JSON.stringify(sectionID) + ",'personID': " + JSON.stringify(personID) + ",'sitePageID': " + JSON.stringify(sitePageID) + ",'sectionContent': " + JSON.stringify(sectionContent) + "}",
        contentType: "application/json; charset=utf-8",
        beforeSend: function () {
            $(".loadProgress").fadeIn();
            $(".loadingText").html("Reverting..")

        },
        dataType: "json",
        success: function (data) {
            sectionSelected.removeClass("inEdit")
            sectionSelected.prop("contentEditable",false)
            sectionSelected.html(data.d)
            $(".loadProgress").fadeOut();
        },
        error: function (request, status, errorThrown) {
            $(".loadingText").html("Error<br/>reverting")
            return false
        }

    });
}
\$\endgroup\$
  • 1
    \$\begingroup\$ At first glance I'd cache $("#dashBody") and $("#topDash"), rather than keep searching the DOM every time you want to reference them. \$\endgroup\$ – Archer Oct 3 '13 at 14:54
  • \$\begingroup\$ Wonderful!! this is exactly the kind of critque I'm looking for. Any more suggestions? \$\endgroup\$ – calcazar Oct 3 '13 at 14:59
4
\$\begingroup\$

I'd cache the following jQuery objects...

var $dashBody = $("#dashBody");
var $topDash = $("#topDash");
var $loadProgress = $(".loadProgress");
var $loadingText = $(".loadingText");

In function clearEditSection() you do a second search for elements that can be cut down to 1...

iFrameContents.find('.inEdit')
    .prop('contenteditable', false)
    .removeClass("inEdit");

Also, a lot of the functions that perform ajax calls can be merged into 1 function, and you also don't need to do as much work with the data object...

function ajaxPost(sectionID, url, beforeText, errorText) {
    $.ajax({
        type: "POST",
        url: url
        data: {
            sectionID: sectionID,
            personID: personID,
            sitePageID: sitePageID
        },
        contentType: "application/json; charset=utf-8",
        beforeSend: function () {
            $(".loadProgress").fadeIn();
            $(".loadingText").html(beforeText)
        },
        dataType: "json",
        success: function (data) {
            $(".loadProgress").fadeOut()
        },
        error: function (request, status, errorThrown) {
            $(".loadingText").html(errorText)
            return false
        }
    });
}

function checkOutStatus(sectionID) could then be called like this...

ajaxPost(sectionID, "default.aspx/checkOutStatus",
    "Verifying <br/> Check Out<br/> Status",
    "Verifying <br/> Check Out<br/>Error");

There's other places where you duplicate DOM lookups so just be aware that repeated jQuery selections can drastically slow the page, depending on the size of the DOM and what browser you are using.

One other thing that I'd strongly advise is that you get into the habit of ending every single "line" of code with a semi-colon. Most of the time you'll get away with it, but there are instances where it can make your code mean something different. Terminate your statements correctly and you won't go wrong :)

\$\endgroup\$
  • \$\begingroup\$ Good idea, Making the Ajax functions in to 1 would drastically reduce the size of the script file. I will be implementing this later on today. \$\endgroup\$ – calcazar Oct 3 '13 at 17:19
4
\$\begingroup\$

you can change

 sectionSelected.removeClass("inEdit")    
 sectionSelected.prop("contentEditable", false)   
 sectionSelected.html(data.d)

to

sectionSelected.removeClass("inEdit").prop("contentEditable", false).html(data.d);

and

selectedElem.children(".cmsEditBox").css('top', (selectedElem.children(".cmsEditBox").height() * -1) - 10)
selectedElem.children(".cmsEditBox").css('left', ((selectedElem.children(".cmsEditBox").width() / 8) * -1))
selectedElem.children(".cmsEditBox").prop('contenteditable', false)   

for this

selectedElem.children(".cmsEditBox").css({
    'top': (selectedElem.children(".cmsEditBox").height() * -1) - 10),
    'left': ((selectedElem.children(".cmsEditBox").width() / 8) * -1)}).prop('contenteditable', false);
\$\endgroup\$
  • \$\begingroup\$ Very good critiques! I went a head and added the above suggesstions on my application. Thank you for your time! \$\endgroup\$ – calcazar Oct 3 '13 at 17:19

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.