patternjavascriptMinor
Algorithm simulator (bubble and selection sorts)
Viewed 0 times
selectionalgorithmbubblesortsandsimulator
Problem
I am concerned where it can be improved.
What it does:
Information:
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
- 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
-
-
You can take advantage of
-
-
You have a ton of repetition. In particular, your parameters for
-
When concatenating a variable to a string, it is implicitly converted. You don't need to explicitly call
-
Continuing from the above, it would make more sense to put the swapping animation in a separate callable function.
-
In
-
In
-
In
HTML
-
You used `
-
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.)
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.