Skip to main content
fixed method call, a couple other minor grammar changes but all I really care about is the method call.
Source Link

Let me know what I can improve on or what I'm doing poorly.

If you are going to do animations with those lines, you will want to keep track of them,. I am assuming that is why you have an array of LINES. However, in createLine you create the SVG element and just return it, you do not keep a reference to it. So it will be hard to animate those lines, you would have to query the DOM to find them back.

I would suggest you have an object that creates SVG elements, a factory so to speak. And then another object that tracks lines you have created so far, mostly to do animations and other fun stuff.

The factory object could be something like

SVG = {
  createCanvas : function( width, height, containerId ){
    var container = document.getElementById( containerId );
    var canvas = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
    canvas.setAttribute('width', width);
    canvas.setAttribute('height', height);
    container.appendChild( canvas );    
    return canvas;
  },
  createLine : function (x1, y1, x2, y2, color, w) {
    var aLine = document.createElementNS('http://www.w3.org/2000/svg', 'line');
    aLine.setAttribute('x1', x1);
    aLine.setAttribute('y1', y1);
    aLine.setAttribute('x2', x2);
    aLine.setAttribute('y2', y2);
    aLine.setAttribute('stroke', color);
    aLine.setAttribute('stroke-width', w);
    return aLine;
  }
}

Initially the lines object could be as simple as

var lines = [];
lines.addLine = function( line ){
 this.push( line );
 return line;
}

Your start function would then be something like

function start() {
  var canvas = SVG.createCanvas( 1000 , 400 , 'container' ),
      lineElement, i, x1;

  for (i = 1; i < 11; i += 1) {
    x1 = Math.floor(Math.random() * 500 / 2),
    lineElement = lines.addLine( SVG.createlinecreateLine(x1, 0, 200, 300, 'rgb(0,0,' + x1 + ')', i) );
    canvas.appendChild( lineElement );
  }
}

Let me know what I can improve on or what I'm doing poorly.

If you are going to do animations with those lines, you will want to keep track of them, I am assuming that is why you have an array of LINES. However, in createLine you create the SVG element and just return it, you do not keep a reference to it. So it will be hard to animate those lines, you would have to query the DOM to find them back.

I would suggest you have an object that creates SVG elements, a factory so to speak. And then another object that tracks lines you have created so far, mostly to do animations and other fun stuff.

The factory object could be something like

SVG = {
  createCanvas : function( width, height, containerId ){
    var container = document.getElementById( containerId );
    var canvas = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
    canvas.setAttribute('width', width);
    canvas.setAttribute('height', height);
    container.appendChild( canvas );    
    return canvas;
  },
  createLine : function (x1, y1, x2, y2, color, w) {
    var aLine = document.createElementNS('http://www.w3.org/2000/svg', 'line');
    aLine.setAttribute('x1', x1);
    aLine.setAttribute('y1', y1);
    aLine.setAttribute('x2', x2);
    aLine.setAttribute('y2', y2);
    aLine.setAttribute('stroke', color);
    aLine.setAttribute('stroke-width', w);
    return aLine;
  }
}

Initially the lines object could be as simple as

var lines = [];
lines.addLine = function( line ){
 this.push( line );
 return line;
}

Your start function would then be something like

function start() {
  var canvas = SVG.createCanvas( 1000 , 400 , 'container' ),
      lineElement, i, x1;

  for (i = 1; i < 11; i += 1) {
    x1 = Math.floor(Math.random() * 500 / 2),
    lineElement = lines.addLine( SVG.createline(x1, 0, 200, 300, 'rgb(0,0,' + x1 + ')', i) );
    canvas.appendChild( lineElement );
  }
}

Let me know what I can improve on or what I'm doing poorly.

If you are going to do animations with those lines, you will want to keep track of them. I am assuming that is why you have an array of LINES. However, in createLine you create the SVG element and just return it, you do not keep a reference to it. So it will be hard to animate those lines, you would have to query the DOM to find them back.

I suggest you have an object that creates SVG elements, a factory so to speak. And then another object that tracks lines you have created so far, mostly to do animations and other fun stuff.

The factory object could be something like

SVG = {
  createCanvas : function( width, height, containerId ){
    var container = document.getElementById( containerId );
    var canvas = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
    canvas.setAttribute('width', width);
    canvas.setAttribute('height', height);
    container.appendChild( canvas );    
    return canvas;
  },
  createLine : function (x1, y1, x2, y2, color, w) {
    var aLine = document.createElementNS('http://www.w3.org/2000/svg', 'line');
    aLine.setAttribute('x1', x1);
    aLine.setAttribute('y1', y1);
    aLine.setAttribute('x2', x2);
    aLine.setAttribute('y2', y2);
    aLine.setAttribute('stroke', color);
    aLine.setAttribute('stroke-width', w);
    return aLine;
  }
}

Initially the lines object could be as simple as

var lines = [];
lines.addLine = function( line ){
 this.push( line );
 return line;
}

Your start function would then be something like

function start() {
  var canvas = SVG.createCanvas( 1000 , 400 , 'container' ),
      lineElement, i, x1;

  for (i = 1; i < 11; i += 1) {
    x1 = Math.floor(Math.random() * 500 / 2),
    lineElement = lines.addLine( SVG.createLine(x1, 0, 200, 300, 'rgb(0,0,' + x1 + ')', i) );
    canvas.appendChild( lineElement );
  }
}
Source Link
konijn
  • 34.3k
  • 5
  • 70
  • 267

Let me know what I can improve on or what I'm doing poorly.

If you are going to do animations with those lines, you will want to keep track of them, I am assuming that is why you have an array of LINES. However, in createLine you create the SVG element and just return it, you do not keep a reference to it. So it will be hard to animate those lines, you would have to query the DOM to find them back.

I would suggest you have an object that creates SVG elements, a factory so to speak. And then another object that tracks lines you have created so far, mostly to do animations and other fun stuff.

The factory object could be something like

SVG = {
  createCanvas : function( width, height, containerId ){
    var container = document.getElementById( containerId );
    var canvas = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
    canvas.setAttribute('width', width);
    canvas.setAttribute('height', height);
    container.appendChild( canvas );    
    return canvas;
  },
  createLine : function (x1, y1, x2, y2, color, w) {
    var aLine = document.createElementNS('http://www.w3.org/2000/svg', 'line');
    aLine.setAttribute('x1', x1);
    aLine.setAttribute('y1', y1);
    aLine.setAttribute('x2', x2);
    aLine.setAttribute('y2', y2);
    aLine.setAttribute('stroke', color);
    aLine.setAttribute('stroke-width', w);
    return aLine;
  }
}

Initially the lines object could be as simple as

var lines = [];
lines.addLine = function( line ){
 this.push( line );
 return line;
}

Your start function would then be something like

function start() {
  var canvas = SVG.createCanvas( 1000 , 400 , 'container' ),
      lineElement, i, x1;

  for (i = 1; i < 11; i += 1) {
    x1 = Math.floor(Math.random() * 500 / 2),
    lineElement = lines.addLine( SVG.createline(x1, 0, 200, 300, 'rgb(0,0,' + x1 + ')', i) );
    canvas.appendChild( lineElement );
  }
}