Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

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.

<script type="text/javascript" src="js/main.js" language="javascript"></script>

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

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

function loaded() {
}

loaded(); 
document.write('</script>');

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.

share|improve this question
 
For what purpose are you using document.write? I doubt that you need to insert an internal script in this way. –  Eric Bréchemier Jan 31 '11 at 13:11
 
With the original question code deleted, most of the answers are incomprehensible or confusing (not the answers fault). Can you restore the original code, and then make an UPDATE: section with changes you have taken on advice? –  Michael Paulukonis Mar 27 at 19:28

2 Answers

up vote 2 down vote accepted

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

// 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) {

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 <div> 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.

share|improve this answer
 
ty for your valuable feedback and refactoring. –  dole Jan 30 '11 at 20:52
up vote 1 down vote

I think you want to take out the document.writes and add

<body onload="loaded()">

(or whatever function you need to run first) to index.html

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.