5
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ For what purpose are you using document.write? I doubt that you need to insert an internal script in this way. \$\endgroup\$ Jan 31 '11 at 13:11
  • \$\begingroup\$ 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? \$\endgroup\$ Mar 27 '13 at 19:28
3
\$\begingroup\$

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.

\$\endgroup\$
0
1
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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