3
\$\begingroup\$

I wrote a web files browser. I want to refactor my JavaScript code, but I'm new to JavaScript.

I have some thoughts:

  1. Split the file into several files:

    • FoldersManager.js
    • FilesManager.js
    • Paginator.js
    • GlobalVars.js

    Do you have any other ideas?

  2. Naming conventions: How should functions\vars names be written?

  3. I wrote my code flat. How can I refactor it to objects easily?

var folders_tree;
var data_per_folder = [];
var current_folder = {};
//    folder_id: "",
//    folder_name: ""
//};

var current_page = 1;

var file_types_lookup = [];


$(function () {

    fillFileTypesLookup();

    // set topbar_links onClick to alter div content
    $(".nav li a").attr({
        "onclick": "topbarItem_OnClick(this.href)"
    });

    // set pagination onClick to alter div file_content
    $(".pagination li a").attr({
        "onclick": "paginationItem_OnClick(this)"
    });


    $("#add_folder").click(function () {
        $('#new_folder_modal').modal('show');       

    });

    $("#remove_folder").click(function () {
       sendDeleteFolderRequestAndUpdateFilesTree();
    });


    $("#edit_folder").click(function () {        
        $('#edit_folder_name').val(current_folder.attr('name'));
        $('#edit_folder_color').val(current_folder.attr('color'));
        $('#edit_folder_description').val(current_folder.attr('description'));
        $('#edit_folder_modal').modal('show');
    });


    //otherwise will create modal with default values.
    $('#extra_details_modal').modal({
        backdrop: true,
        keyboard: true
    });

        //otherwise will create modal with default values.
    $('#new_folder_modal').modal({
        backdrop: true,
        keyboard: true
    });

    //set jsTree
    $("#jstree").jstree({
        "json_data": {

            //initial - demo only, usually takes json from the controller
            //static data, or function(node, mappingBeforeRequestToserver)
            //"data": data, 
            //combines with data above
            "ajax": {

                //data: mappingBeforeRequestToserver-function(node about to be open or -1).  `this` will be the tree instance) 
                //url: function(node about to be open as a paramater or -1) retuns the ajax URL (like /get_children/node_2).
                //The error and success functions- modifiying the results from the server before populating the tree  
                type: "GET",
                async: true,
                url: function (node) {
                    if (node == -1) {
                        return 'Manager/GetLocations';
                    }

                    //                     else {
                    //                        return 'Manager/GetAttachments';
                    //                    }
                },

                data: function (node) {
                    if (node == -1) {
                        return {
                            'userId': 'a358ab9d-d481-4bdd-8cb2-18ddc8898c78'
                        };
                    }

                    //                    else {
                    //                        return {
                    //                            'userId': 'a358ab9d-d481-4bdd-8cb2-18ddc8898c78',
                    //                            'count': '5',
                    //                            'locationId': node[0].id,
                    //                            'startIndex': '0'
                    //                        };
                    //                    }
                },

                contentType: "application/json; charset=utf-8",
                dataType: "json",
                cache: true,
                success: function (msg) {
                    // alert("succeeded" + msg);
                    folders_tree = msg;
                    return msg;
                },
                error: function (msg) {
                    // alert(msg);
                }
            },
            "xsl": "flat",
            "override_ui": "true",
            "real_checkboxes": "true",
        },
        "plugins": ["themes", "json_data", "ui", "crrm"]
    }).bind("select_node.jstree", onSelectFolder);
});


function topbarItem_OnClick(href) {
    $(".pill-content>div").hide();
    var indexOfHash = href.indexOf("#");
    var selected = href.substring(indexOfHash);
    $(selected).show();
};


function paginationItem_OnClick(paginationItem) {
    //we are already in a page. 
    //current_folder.attr('id') and current_folder.attr('name') are not undifined
    //paginationItem.attr({class : "active"});
    var pageNum = paginationItem.text;
    current_page = pageNum;
    $(".pagination li.active").removeAttr("class");
    $("#pagination_" + pageNum).attr("class", "active");

    //console.debug(pageNum);
    if (data_per_folder[current_folder.attr('id')].data_per_page[pageNum] == undefined) // no data for pageNum
    {
        fillAttachmentsPerPage(pageNum);

    } else {
        populatePage(pageNum);
    }
};

function onSelectFolder(event, data) {

    topbarItem_OnClick('#files');

     current_folder = data.rslt.obj;

    //current_folder.attr('id') = data.rslt.obj.attr('id');
    //current_folder.attr('name') = data.rslt.obj.attr('name');

    if (data_per_folder[current_folder.attr('id')] == undefined) //no data for this folder
    {
        data_per_folder[current_folder.attr('id')] = {
            data_per_page: []
        };
    }

    if (data_per_folder[current_folder.attr('id')].data_per_page[1] == undefined) // no data for first page
    {
        fillAttachmentsPerPage(1);
    } else {
        populatePage(1);
    }
};

function populatePage(pageNum) {
    $("#files_gallery")[0].innerHTML = data_per_folder[current_folder.attr('id')].data_per_page[pageNum].attachments_markup;
    $("#files #files_header")[0].innerHTML = '<li><a href="/user/messages"><span class="file_item"> attachments for folder (name: ' + current_folder.attr('name') + ' id : ' + current_folder.attr('id') + 
    ' parentId: ' + current_folder.attr('parentId') + ' description : ' +current_folder.attr('description')+')</span></a></li>';

}

function fillFileTypesLookup() {
    if (file_types_lookup.length == 0) // no files types in lookup
    {

        createAjaxRequest("Manager/GetTopAttachmentTypes", null).done(function (res) {

            for (var i = 0; i < res.length; i++) {

                file_types_lookup[res[i].TypeId] = res[i];
            }
        });
    }
}

function fillAttachmentsPerPage(pageNum) {
    createAjaxRequest("Manager/GetAttachments", {
        'userId': 'a358ab9d-d481-4bdd-8cb2-18ddc8898c78',
        //'3141a957-05e5-4678-9f5d-4cc9f41d1a00',
        'count': '16',
        //attachments per page
        'locationId': current_folder.attr('id'),
        'startIndex': (pageNum - 1) * 16
    }).done(function (res) {

        //res =  IEnumerable<Attachment> Attachments, long TotalCount, long LastIndex
        var attachments_divs = "";

        for (var i = 0; i < res.Attachments.length; i++) {
            //attachment_img = '<a><img src="/Images/Browser/file_types/'+res.Attachments[i].AttachmentTypeId +'.png" title= "name: '+ res.Attachments[i].Name+'"/></a>';
            attachment_img = '<img src="' + file_types_lookup[res.Attachments[i].AttachmentTypeId].IconUrl + '" title= "name: ' + res.Attachments[i].Name + '" onclick="Attachment_onClick(\'' + res.Attachments[i].AttachmentId + '\')" />';
            attachments_divs = attachments_divs + ' <div> ' + attachment_img + ' </div>';
        }


        data_per_folder[current_folder.attr('id')].data_per_page[pageNum] = {
            first_index: 0,
            last_index: res.TotalCount,
            count: res.LastIndex,
            attachments: res.Attachments,
            attachments_markup: attachments_divs,
            attachment_extraDetails: []
        };

        populatePage(pageNum);
    });
}

function Attachment_onClick(attachmentId) {
    if (data_per_folder[current_folder.attr('id')].data_per_page[current_page].attachment_extraDetails[attachmentId] == undefined) {

        createAjaxRequest("Manager/GetAttachmentExtraDetails", {
            'attachmentId': attachmentId
        }).done(function (res) {

            //res =   AttachmentExtraDetails:   Guid AttachmentId, string MessageLink, string AttachmentLink, 
            //        DateTime SentTime, DateTime ArrivalTime, int Size, Guid SenderFriendId, string SenderEmail

            var sentTimeStr = new Date(parseInt(res.SentTime.substring(6,19),10)*1000).toString();
            var arrivalTimeStr = new Date(parseInt(res.ArrivalTime.substring(6,19),10)*1000).toString();
            var sizeStr;
            if (res.Size > 1000) 
            sizeStr = res.Size / 1000+'.'+ (res.Size % 1000) / 100 +' MB';
            else 
            sizeStr = res.Size +' KB';

            var attachments_paragraphs = '<p>Attachment Id: ' + res.AttachmentId + '</p> <p>Message Link: ' + res.MessageLink + '</p>'+
            '<p>Attachment Link: ' + res.AttachmentLink +'</p> <p>Sent Time: ' + sentTimeStr + '</p> <p>Arrival Time: ' + arrivalTimeStr + '</p>'+ 
            '<p>Size: '+sizeStr+' </p> <p>Sender Friend Id: '+res.SenderFriendId+'</p> <p>Sender Email: '+res.SenderEmail+' </p>';


        data_per_folder[current_folder.attr('id')].data_per_page[current_page].attachment_extraDetails[attachmentId] = {attachments_markup : attachments_paragraphs};
        fillExtraDetailsModalAndShow(attachmentId);
        });
    }

     if (data_per_folder[current_folder.attr('id')].data_per_page[current_page].attachment_extraDetails[attachmentId] != undefined) { // not the first time
         fillExtraDetailsModalAndShow(attachmentId);
        }
    };


    function fillExtraDetailsModalAndShow(attachmentId){
        $(".modal-body").html(data_per_folder[current_folder.attr('id')].data_per_page[current_page].attachment_extraDetails[attachmentId].attachments_markup);
        $('#extra_details_modal').modal('show');
    };

    function sendCreateNewFolderRequestAndUpdateFilesTree()
    {
        if (validateCreateNewFolderDialog())
        {
        var new_folder_descriptor = {};
        new_folder_descriptor.name = $('#new_folder_name').val();
        new_folder_descriptor.color = $('#new_folder_color').val();
        new_folder_descriptor.description = $('#new_folder_description').val();
        new_folder_descriptor.userId = 'a358ab9d-d481-4bdd-8cb2-18ddc8898c78';
        new_folder_descriptor.parentId = current_folder.attr('id');

        createAjaxRequest("Manager/CreateNewLocation", 
        {
            'name': new_folder_descriptor.name,
            'color' : new_folder_descriptor.color,
            'userId' :  new_folder_descriptor.userId,
            'parentId' : new_folder_descriptor.parentId,
            'description' : new_folder_descriptor.description
        }).done(function (res) {

           if (res.IsSucceeded)
           {
                new_folder_descriptor.locationId = res.LocationId;
                createNewFolderInFilesTree(new_folder_descriptor);
                $('#new_folder_modal').modal('hide');
           }

          //TODO: benda  else: error

        });
        }

    };

    function validateCreateNewFolderDialog()
    {
        var isValid = false;

        if ($('#new_folder_name').val() != "")
        {
           $('#new_folder_name_div').attr("class", "clearfix");
            isValid = true;
        }
        else
        {      
           //TODO: benda fix css
           //$('#new_folder_name').attr("color", "#b94a48");
           // $('#folder_name_div').attr("class", "clearfix error");
        }
        return isValid;        
    };

    function createNewFolderInFilesTree(new_folder_descriptor)
    {
        $("#jstree").jstree("create", null, "last", {
            "data": new_folder_descriptor.name,
            "attr": {
                "id": new_folder_descriptor.locationId,
                "color": new_folder_descriptor.color,
                "description": new_folder_descriptor.description,
                "parentId" : new_folder_descriptor.parentId,
                "name": new_folder_descriptor.name
            },
            "state": "opened"
        }, null, //null instead of delegate like: function(data) {this.attr('id')= this.data},
        true); //"skip_rename"        
    };    


    function sendDeleteFolderRequestAndUpdateFilesTree()
    {
        var new_folder_descriptor = {};
        new_folder_descriptor.userId = 'a358ab9d-d481-4bdd-8cb2-18ddc8898c78';
        new_folder_descriptor.locationId = current_folder.attr('id');

        createAjaxRequest("Manager/DeleteLocation", 
        {
            'locationId' : new_folder_descriptor.locationId
        }).done(function (isSucceeded) {

           if (isSucceeded)
           {
                deleteFolderInFilesTree();
                //current_folder = {};
           }

          //TODO: benda  else: error

        });
    };


    function deleteFolderInFilesTree()
    {
        $("#jstree").jstree("remove", null);
    };


    function sendEditFolderRequestAndUpdateFilesTree()
    {

        var edit_folder_descriptor = {};
        edit_folder_descriptor.userId = 'a358ab9d-d481-4bdd-8cb2-18ddc8898c78';
        edit_folder_descriptor.locationId = current_folder.attr('id');

        edit_folder_descriptor.name = $('#edit_folder_name').val();
        edit_folder_descriptor.color = $('#edit_folder_color').val();
        edit_folder_descriptor.description = $('#edit_folder_description').val();;
        edit_folder_descriptor.parentId = current_folder.attr('parentId');


        createAjaxRequest("Manager/EditLocation", 
        {
            'locationId' : edit_folder_descriptor.locationId,
            'name': edit_folder_descriptor.name,
            'color' : edit_folder_descriptor.color,
            'userId' :  edit_folder_descriptor.userId,
            'parentId' : edit_folder_descriptor.parentId,
            'description' : edit_folder_descriptor.description
        }).done(function (isSucceeded) {

           if (isSucceeded)
           {
                current_folder = {};
                editFolderInFilesTree(edit_folder_descriptor);
                $('#edit_folder_modal').modal('hide');
           }

          //TODO: benda  else: error

        });
    };


    function editFolderInFilesTree(edit_folder_descriptor)
    {

        var editedNode = $("#"+edit_folder_descriptor.locationId);
        editedNode.attr('name',edit_folder_descriptor.name);
        editedNode.attr('color',edit_folder_descriptor.color);
        editedNode.attr('description',edit_folder_descriptor.description);
        editedNode.attr('parentId',edit_folder_descriptor.paretnId);
        editedNode.attr('id',edit_folder_descriptor.locationId);

        $("#jstree").jstree("refresh", null);
    };
\$\endgroup\$

migrated from codereview.meta.stackexchange.com Jan 7 '12 at 16:44

This question came from our discussion, support, and feature requests site for peer programmer code reviews.

  • \$\begingroup\$ naming conventions: Use a styleGuide of your choice. \$\endgroup\$ – Raynos Jan 7 '12 at 16:56
2
\$\begingroup\$

A couple notes on design:

  1. Split the code up into different objects that handle specific tasks. You don't need to split your code up into different files, because it's not that long (yet), but you can split the functionality into objects, like this:

    FoldersManager = {
    
      init: function() {
        $("#add_folder").click(function () {
          $('#new_folder_modal').modal('show');       
        });
    
        ...
      }
    
    };
    
  2. Put code that manipulates the DOM into a $.ready() block, which guarantees that the DOM will be ready when your code executes:

    $.ready(function() {
      FoldersManager.init();
      FilesManager.init();
      Paginator.init();
    });
    
  3. As ericW mentioned, don't use global variables. They can conflict with global variables from another file which could result in hard-to-find bugs. You can stick them inside the new objects using the following pattern:

    $.ready(function() {
      FoldersManager.init();
      FilesManager.init();
      Paginator.init();
    });
    
    FoldersManager = (function() {
    
      var folders_tree;
      var data_per_folder = [];
      var current_folder = {};
    
      return {
    
        init: function() {
          $("#add_folder").click(FoldersManager.openAddFolderDialog);
    
          ...
        },
    
        openAddFolderDialog: function() {
          $('#new_folder_modal').modal('show');
        }
    
      };
    
    });
    

And some minor things:

  1. Why are you doing things like this:

    $(".nav li a").attr({
        "onclick": "topbarItem_OnClick(this.href)"
    });
    

    You can use the same $.click() function you were using for the anonymous functions:

    $(".nav li a").click(function() {
      // or change the function to use the href attribute directly
      // instead of taking it as a param
      topbarItem_OnClick($(this).attr('href'));
    });
    
  2. This:

    if (data_per_folder[current_folder.attr('id')] == undefined)
    

    can be simplified to:

    if (!data_per_folder[current_folder.attr('id')])
    

    See http://james.padolsey.com/javascript/truthy-falsey/ for more info.

\$\endgroup\$
1
\$\begingroup\$

Your thinking is much better. One thing need to attention: don't left global vars. The best practice is to use global objects, and put the global vars into them.

When you make the object's responsibility clear, make sure every function is in the right object. The code will be much easier to read.

\$\endgroup\$
  • \$\begingroup\$ can you help me with refactoring one of the objects? I want to see the right form \$\endgroup\$ – Elad Benda Jan 10 '12 at 17:40
0
\$\begingroup\$

People will probably throw things at me for this answer, but here goes.

This advice goes mostly for languages like JS, but has some portability.

In a function, you are typically dealing with an object of primary concern, sometimes it is 'this' -- but other times it can be a concern global to the function, or a concern local to a function part....

To make the code more readable, name such objects generically, like o, or it. Make a comment to what that symbol really means/is at its declaration if you have to. Even if you are dealing with a global var like G.some_long_name, create a local shortname for it:

var o=G.some_long_name;

There are trade-offs to this method though - finding refs is not as easy with your editor. But compressed code like this is much easier to read, and makes copy/paste code duplication easier.

Next, do something to your parameter names to make them distinguishable from locals. I put a tail on them, and make them generic too when I can, using o_ for object, and n_ for name, i_ for index.

Use unix_style for private functions, and camelCase for public, unless you are extending a core type prototype, then you might want to use String.prototype.Has, instead of String.prototype.has -- depending on if you want to accentuate a source where something might have gone wrong versus having your extension blend in.

Name your class member vars like _member.

Repetitive code like this should be declared as an object, unless it is going to be changing dynamically:

data_per_folder[current_folder.attr('id')]

Use

var f_data=data_per_folder[current_folder.attr('id')];
f_data.something();

And here is a cool trick, instead of

$(".pagination li.active").each(...

How about

var items=".pagination li.active".$();
items.each(...
items.ect(..

using

String.prototype.$=function (){return $(''+this);}

You can even make a builder to dynamically create/manage the selectors you are using, a type of meta-selector:

String.prototype.$$=function (){
  var me=''+this;
  var s='';
  if (me=='coolbeans'){
     s+=condition?' #something':'#somethingelse';
     // more...
  }
  // magic
  return s.$();// as per above
}
// later
var dynamicthings='coolbeans'.$$();
\$\endgroup\$

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.