HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

JavaScript file for a CMS application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filecmsapplicationjavascriptfor

Problem

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($("",
{
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($(""));
for (i = 0; i ", {
html: editBoxButtons[i],
addClass: "editButtons",
click: function () { editButtonAction(this, selectedElem) }

Solution

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  Check Out Status",
    "Verifying  Check OutError");


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 :)

Code Snippets

var $dashBody = $("#dashBody");
var $topDash = $("#topDash");
var $loadProgress = $(".loadProgress");
var $loadingText = $(".loadingText");
iFrameContents.find('.inEdit')
    .prop('contenteditable', false)
    .removeClass("inEdit");
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
        }
    });
}
ajaxPost(sectionID, "default.aspx/checkOutStatus",
    "Verifying <br/> Check Out<br/> Status",
    "Verifying <br/> Check Out<br/>Error");

Context

StackExchange Code Review Q#32184, answer score: 4

Revisions (0)

No revisions yet.