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

Algorithm simulator (bubble and selection sorts)

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

Problem

I am concerned where it can be improved.

  • Main concerns are coding style.



  • Any other improvements are welcome too.



  • Priority is on JavaScript, review for HTML/CSS are welcome too.



What it does:

  • Simulate Bubble Sort graphically with animations.



  • Simulate Selection Sort graphically with animations.



  • There are some visual kinks in Selection Sort simulation but it sorts correctly and works to the best of my knowledge.



Information:

  • Contains Single HTML Page,Single CSS file and Single JavaScript file



  • Prototype.js => v1.7



  • script.aculo.us => v1.9.0



  • Screen clipping, just a part of it



JavaScript: base.js

```
/**
* Bhathiya Perera IT110057-04
*
*/

/**
* Draws an array with indexes in box structure
* @param {Object} arr Array to be drawn
*/
function drawarray(arr) {
var boxset = $("boxset");
var boxsetnum = $("boxsetnum");
boxset.innerHTML = "";
boxsetnum.innerHTML = "";

boxset.hide();
boxsetnum.hide();

var boxsetStr = "";
var boxsetnumStr = "";

for (var i = 0; i " + arr[i] + ""
boxsetnumStr = boxsetnumStr + "" + (i + 1) + ""
}

boxset.innerHTML = boxsetStr;
boxsetnum.innerHTML = boxsetnumStr;

boxset.show();
boxsetnum.show();

}

/**
* Simulate Bubble Sort (Ascending only)
* @param {Object} a Array (Must be an integer array)
*/
function bubbleSort(a) {
var swapped;
do {
swapped = false;
for (var i = 0; i a[i + 1]) {
new Effect.Move("arrbox" + i.toString(), {
x : 32,
y : 0,
queue : 'end'
});
new Effect.Move("arrbox" + (i + 1).toString(), {
x : -32,
y : 0,
queue : 'end'
});
new Effect.Move("arrbox" + i.toString(), {
x : 0,
y : 32,
queue : 'end'
});
new Effect.Mo

Solution

Disclaimer: I am familiar with neither Prototype nor Scriptaculous.

JS

-
drawarray() should be named drawArray().

-
You can take advantage of += instead of boxsetStr = boxsetStr + ....

-
/ is not a special character in strings and does not need to be escaped. "" is clutter; it's harder to read than "".

-
You have a ton of repetition. In particular, your parameters for Effect.Move are repeated. You should store them in a constant.

-
When concatenating a variable to a string, it is implicitly converted. You don't need to explicitly call i.toString(). Combining this with the previous point, we get:

var MOVE_LEFT = { x: -32, y: 0, queue: "end" };
...
new Effect.move("arrbox" + i, MOVE_LEFT);


-
Continuing from the above, it would make more sense to put the swapping animation in a separate callable function.

-
In bubble_simulate(), you use parseInt without explicitly stating the base. In general, you should explicitly state the base with parseInt(toBeSorted[q], 10). This is because in older browsers which do not comply with ECMAScript 5, strings leading with a 0 are treated as octal numbers, so parseInt("010") => 8. In practice, this is not the current behaviour of any of the major browsers.

-
In selectionSort, you declare your loop variables at the top, while in bubbleSort, you declared i in your loop. This would be fine if you actually reused your loop variables in selectionSort, but you don't; the resulting inconsistency strikes me as odd.

-
In show_selection_sort(), it seems like you would benefit greatly from templating. It certainly looks extremely busy.

HTML

-
You used ` for layout. No. Bad. s are for data, not for layout. (Exception: HTML for email.) You should be using s.

-
There's some unnecessary whitespace where you included
algo/base.js. There's some more after and . I say unnecessary because these do not make your structure any easier to read.

-
For some reason you have
style attributes on things where some improved CSS could easily apply.

CSS

-
You're using
cursor: pointer; cursor: hand;. This is no longer necessary; the last browser to support cursor: hand but not cursor: pointer was IE 5.5, which was dropped from support almost ten years ago.

-
Vendor prefixes for
border-radius` have not been required for quite a while. This article on css-tricks.com estimates less than 1% of 2012 traffic to support prefixed versions but not the bare version.

-
In general, people like their CSS to be ordered in some fashion, whether grouped by category or alphabetically. I can't seem to detect an ordering for your CSS. (Of all the points made, this one matters the least.)

Code Snippets

var MOVE_LEFT = { x: -32, y: 0, queue: "end" };
...
new Effect.move("arrbox" + i, MOVE_LEFT);

Context

StackExchange Code Review Q#59233, answer score: 4

Revisions (0)

No revisions yet.