Refactor 2d_primitives.js

Increasing Access

It would make the code shorter and more readable

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build Process
• Unit Testing
• Internalization
• Friendly Errors
• Other (specify if possible)

Feature enhancement details

I think that 2d_primitives.js could be refactored a bit.
These are some suggestions:

_normalizeArcAngles signature

Problems:

• function has too many arguments
• function has boolean argument, which requires, when reading the code, to see the function signature

From this:

p5.prototype._normalizeArcAngles = (
  start,
  stop,
  width,
  height,
  correctForScaling
)

To this:

p5.prototype._normalizeArcAngles = (
  size, //(width, height),
  drawRegion, //(startAngle, stopAngle)
  angleBehaviourOnScale //(angleBehaviourOnScale.Stretch, angleBehaviourOnScale.KeepRelative)
)

_normalizeArcAngles angle constraint

Code:

  // Constrain both start and stop to [0,TWO_PI).
  start = start - constants.TWO_PI * Math.floor(start / constants.TWO_PI);
  stop = stop - constants.TWO_PI * Math.floor(stop / constants.TWO_PI);

Problems:

• contains many levels of abstractions,
• code should expain itself

Proposal:

  drawRegion = ConstrainDrawRegionToOneCircumference(drawRegion);

_normalizeArcAngles abs

Code:

separation = Math.min(
    Math.abs(start - stop),
    constants.TWO_PI - Math.abs(start - stop)
  );

Problem:

• contains many levels of abstractions
• makes the function too long

Proposal:

arcAngle = getArchAngle(drawRegion);

_normalizeArcAngles angle scaling

Code:

// Optionally adjust the angles to counter linear scaling.
 if (correctForScaling) {
   if (start <= constants.HALF_PI) {
     start = Math.atan(width / height * Math.tan(start));
   } else if (start > constants.HALF_PI && start <= 3 * constants.HALF_PI) {
     start = Math.atan(width / height * Math.tan(start)) + constants.PI;
   } else {
     start = Math.atan(width / height * Math.tan(start)) + constants.TWO_PI;
   }
   if (stop <= constants.HALF_PI) {
     stop = Math.atan(width / height * Math.tan(stop));
   } else if (stop > constants.HALF_PI && stop <= 3 * constants.HALF_PI) {
     stop = Math.atan(width / height * Math.tan(stop)) + constants.PI;
   } else {
     stop = Math.atan(width / height * Math.tan(stop)) + constants.TWO_PI;
   }
 }

Problems:

• contains repeated code,
• contains numeric literals,
• contains a redundant comment,
• contains many levels of abstractions,
• is too long

Proposal:

  if(angleBehaviourOnScale === angleBehaviourOnScale.KeepRelative)
    makeDrawRegionRelative(drawRegion);
   
makeDrawRegionRelative(region){
  region.start = makeAngleRelative(region.start);
  region.stop = makeAngleRelative(region.stop);
  return region;
}

//I don't understand the math behind completely so I'm not able to simplify it further, 
// this method is actually too long but it's still an improvement
//TODO: simplify this method
makeAngleRelative(angle){
  const THREE_PI = 3 * constants.HALF_PI;
  let offsetAngle = 0;
  
  if(angle < constants.PI)
    offsetAngle = 0;
  else if (angle <= THREE_PI)
    offsetAngle = constants.PI;
  else 
    offsetAngle = constants.TWO_PI;

  return Math.atan(width / height * Math.tan(angle)) + offsetAngle;
}

_normalizeArcAngles angle last check

Code:

// Ensure that start <= stop < start + TWO_PI.
if (start > stop) {
  stop += constants.TWO_PI;
}

Problem:

• function contains many levels of abstractions
  Proposal:

drawRegion = makeStopGreaterThanStart(drawRegion);

But I would leave the comment inside of the function because it's not obvious

arc function signature

Problem:

• function has too many arguments but it's a public API so it's better to just group them and call another function

Proposal:

p5.prototype.arc = function(x, y, w, h, start, stop, mode, detail) {
  p5._validateParameters('arc', arguments);
  position: {x:x,y:y}
  size: {width:w, height: h}
  drawRegion: {start: start, stop: stop}
  return arc(position, size, drawRegion, mode, detail);
}

arc function premature return

Code:

// if the current stroke and fill settings wouldn't result in something
// visible, exit immediately
if (!this._renderer._doStroke && !this._renderer._doFill) {
  return this;
}
if (start === stop) {
  return this;
}

Problem:

• Code should expain itself so the comment should not be necessary

Proposal:

if(isArcNotVisible(this, drawRegion))
  return this;

arc function size abs

Code:

// p5 supports negative width and heights for ellipses
w = Math.abs(w);
h = Math.abs(h);

Problem:

• a function should only contain one level of abstraction

Proposal:

size = flipNegativeDimensions(size);

circle inline documentation

Problem:

• I don't think it is necessary to explain what a circle is.

 /* Draws a circle to the canvas. A circle is a round shape. Every point on the
 * edge of a circle is the same distance from its center. By default, the first
 * two parameters set the location of the center of the circle. The third
 * parameter sets the shape's width and height (diameter).
 */

circle function

Code:

  p5.prototype.circle = function() {
    p5._validateParameters('circle', arguments);
    const args = Array.prototype.slice.call(arguments, 0, 2);
    args.push(arguments[2]);
    args.push(arguments[2]);
    return this._renderEllipse(...args);
  }

Problem:

• code could be simplified

Proposal:

  p5.prototype.circle = function(x, y, diameter) {
   p5._validateParameters('circle', arguments);   
   return this._renderEllipse(x, y, diameter, diameter, diameter);
  }

_renderEllipse signature

Code:

p5.prototype._renderEllipse = function(x, y, w, h, detailX){
}

Problem:

• too many arguments

Proposal:

p5.prototype._renderEllipse = function(position, size, detailX){
}

_renderEllipse code

Code:

// p5 supports negative width and heights for rects
  if (w < 0) {
    w = Math.abs(w);
  }

  if (typeof h === 'undefined') {
    // Duplicate 3rd argument if only 3 given.
    h = w;
  } else if (h < 0) {
    h = Math.abs(h);
  }

##Opinion:

• I don't think this function should be so elastic to be able to accept different counts of arguments. It's only used internally after all.

Proposal:

size = flipNegativeDimensions(size);

I noticed that in many functions x and y, width and height are separate arguments but I think it's preferable to use position and size instead.

I would like to be assigned to this issue.

Start by reading 2d_primitives.js, especially _normalizeArcAngles, arc, circle, and _renderEllipse, and compare the proposed signature and abstraction changes with their current callers. Before changing code, establish which refactors are in scope and verify that existing rendering behavior and public APIs remain intact when the work is complete.