Sorting Each Entry (code review + optimization)

Question

I have a requirement where there are 3 selection criteria user selects countries from a map displayed as lets say in 3 categories , i am maintaining 3 arraylist for the same.Each time he selects a country i have to display list of country names displayed in Alphabetical order in a bottom bar with font color distinguishing each category .I have written a code which will add all the values of arraylist in a different arraylist and sort it and then based on category it will create font for each and display them in the bottom so each time user selects a country this method is called.Please review and let me know if i can improve the performance.Below is the code

Lets say issuingOffice,selectedCountries and deSelectedCountries are arraylist declared in global scope and have values.

function updateBottomBar(){
    var initalContent="Countires: <label>";
    require(["dojox/collections/ArrayList"],function(arrayList){
    var bottomBar = new arrayList();
    var it = issuingOffice.getIterator();
    while (!it.atEnd()) {
        var country = it.get();
        bottomBar.add(country);
    }
    var scit = selectedCountries.getIterator();
    while (!scit.atEnd()) {
        var country = scit.get();
        bottomBar.add(country);
    }
    var descit = deSelectedCountries.getIterator();
    while (!descit.atEnd()) {
        var country = descit.get();
        bottomBar.add(country);
    }
    bottomBar.sort();
    var bbit = bottomBar.getIterator();
    while (!bbit.atEnd()) {
        var country = bbit.get();
        if(selectedCountries.contains(country)){
            initalContent!="<font>"+country+"</font>";
        }else if(selectedCountries.contains(country)){
            initalContent!="<font>"+country+"</font>";
        } else{
            initalContent!="<font>"+country+"</font>";
        }
    }
    initalContent+="</label>";
    dojo.byId("displayCountryName")=initalContent;
    });
}

Should that be a += in here: initalContent!="<font>"+country+"</font>";? — ChrisWue
– ChrisWue, Commented Oct 11, 2013 at 0:24
@ChrisWue Thanks for reminding me, I've seen it but totally forgot to include it in the answer. Done. — plalx
– plalx, Commented Oct 11, 2013 at 3:49

plalx · Accepted Answer · 2013-10-11 23:38:03Z

There are many improvements that can be done, but here are a few things I identified:

I wouldn't have the require call within the function itself. I would rather create a top-level module with all listed dependencies. The updateBottomBar function would be defined in this module:

require(["dojox/collections/ArrayList"/*, other deps...*/], function(ArrayList) {
    function updateBottomBar() {
        //...
    }
});

Have a look at https://dojotoolkit.org/documentation/tutorials/1.8/modules/

It's very inefficient to merge the multiple arrays every-time. You should rather maintain an additional array that stays in sync with the others. When a new item is pushed in one of the other arrays, you reflect the changes in that additional array directly (same logic with removal). However, if you can, I would rather have a single collection to maintain and use a discriminator such as a category property to categorize items, so instead of holding strings, the collection would contain { country: String, category: String } objects.
With your current implementation, you also throw away some useful information that you need to recompute later. For example if(selectedCountries.contains(country)) shouldn't be necessary since you had that knowledge when you pushed the items in the bottomBar list. Instead of pushing a simple string, push an object like proposed in point #2. Then you will be able to check if the items belongs to the selected category in O(1) time by doing something like:
```
switch (country.category) {
    'selected': //do something break;
}
```
Concatenating multiple strings with the + operator isin't the most efficient way. Instead you should push your strings in an array and then use theArray.join('') to perform the concatenation.

Also note that you can create DOM elements dynamically with document.createElement and make the process efficient by using document fragments (document.createDocumentFragment).
You have a typo -> var initalContent="Countires: <label>";
initalContent != ... Did you mean += ?

Keeping data in sync in different data structures is not a good idea, so I'd definitely go with the single source option. — ChrisWue
– ChrisWue, Commented Oct 11, 2013 at 0:07
@ChrisWue Well that might not be possible for the OP, that's why I've made that suggestion. However, why do you think it's such a bad idea? — plalx
– plalx, Commented Oct 11, 2013 at 3:00
Keeping data in sync requires more code and more decisions to make, more places to check, more things you can forget when extending the code more bugs to be put in. It will get you in trouble sooner or later. — ChrisWue
– ChrisWue, Commented Oct 11, 2013 at 6:47

Stack Exchange Network

Sorting Each Entry (code review + optimization)

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Sorting Each Entry (code review + optimization)

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions