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

Need a feedback on my Javascript code and app implementation idea

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

Problem

I'm new into Javascript, and I'm not sure that is a good approach. The code works, and does what I need it to do but I'm sure I didn't do the things the right way.

Can you give me feedback about the implementation idea and code?

So, let's say that in index.html I have the following code into body section.



The content of the main.js file is the following:

document.write('');

function loaded() {
}

loaded(); 
document.write('');


I know that using document.write it is not a smart thing at all. I'm sure that other things are faulty, but I'm newbie and I'm looking for your feedback.

Solution

From a quick glance there are a couple of points where your making simple mistakes.

// Document.write is bad
document.write('');

// use css definitions instead
counter_div.setAttribute('style', 'width: 310px; height: 50px; font-family:lucida,tahoma,helvetica,arial,sans-serif; display: block; overflow:hide;');

// dont set inner html. This is bad. use DOM manipulation instead
title_span.innerHTML = meter_title;

// uses eval here. pass a function rather then a string
setInterval("increment()", interval);

// forgetting to declare addCommas with `var`. This is implecetly global.
addCommas = function(nStr) {


I shall have a look at how to redesign this.

Why do you need to append the div after the script tag. It's bad by design to use the script tag in the DOM for positioning.

it would be better to create a `` to fill with the timer/counter.

Refactored partly here. This includes fixing the issues above plus making the dom manipulation a bit nicer.

I'm not too certain how to do refactoring on your timer code.

Code Snippets

// Document.write is bad
document.write('<script language="javascript" type="text/javascript" >');

// use css definitions instead
counter_div.setAttribute('style', 'width: 310px; height: 50px; font-family:lucida,tahoma,helvetica,arial,sans-serif; display: block; overflow:hide;');

// dont set inner html. This is bad. use DOM manipulation instead
title_span.innerHTML = meter_title;

// uses eval here. pass a function rather then a string
setInterval("increment()", interval);

// forgetting to declare addCommas with `var`. This is implecetly global.
addCommas = function(nStr) {

Context

StackExchange Code Review Q#466, answer score: 3

Revisions (0)

No revisions yet.