2
\$\begingroup\$

I've developed a function to calculate the exit angle of a projectile after it collides with a boundary/edge inside a rectangular space, based on the entry angle and the axis it is colliding with.

Given those two factors, the actual boundary is implied, since a projectile travelling at an angle of 315° (where 0°/360° sit at 3 o'clock) and colliding with a horizontal boundary (the x axis) must be colliding with the top boundary.

The current implementation came about through iterative refactoring of a much more verbose and redundant version, however I can't vocalise exactly why some parts are necessary, but they appear to satisfy the requirements and I'm confident it works based on a number of tests.

function reflectAngle(angle, axis) {

    if (angle % 90 === 0)
        return (angle + 180) % 360;

    let reflected;
    let segment = (angle % 90) * 2;

    if (axis === 'X')
        reflected = angle + (180 - segment);

    else if (axis === 'Y')
        reflected = angle - segment;

    if (angle < 90)
        reflected += 180;

    return reflected % 360;
}

Can this function be simplified or improved for readability?

\$\endgroup\$
1
  • \$\begingroup\$ Could you include example cases that you tested? I'm not convinced that all of these calculations make sense. \$\endgroup\$ Feb 14 '18 at 3:32
3
\$\begingroup\$

Review

A little ambiguous, and overly complex. You need to familiarize your self with radians and forget about degrees

Style

Block-less blocks

A block is a section of code delimited by {...} many C like syntax languages allow you to skip the block delimiters for single line blocks after conditional statements.

This can be a major source of life long frustration, and there is not a programmer alive that does not know the insidious nature of the bugs that can result when you make changes to the code and forget the {...}

// worst
if (foo === bar)
       foo = poo

// bad
if (foo === bar)
       foo = poo;

// better but be consistent never mix this with above
if (foo === bar) foo = poo;

// best 
if (foo === bar) { foo = poo } // note that ; and } denote the end of 
                                // an expression, you don't need to use both

// or
if (foo === bar) { 
   foo = poo;
}

Constants

So many magic numbers. If you find your self adding the same numbers over and over it is a good sign that a defined constant is better. You never know when things may change. I.E. switch units from degrees to radians.

const A90  = 90;
const A180 = 180;
const A360 = 360;

Then if you want to change to radians

const A90  = Math.PI / 2;
const A180 = Math.PI;
const A360 = Math.PI * 2;

or clock angles

const A90  = 3;
const A180 = 6;
const A360 = 12;

No need to find each number in the code base.


Logic

  1. The axis argument is ambiguous. Axis? is it the axis of reflection or the axis to reflex from. Also you use a string and to humans "x", "X" and "y", "Y" have the same meaning, but your code sees them differently, this is never a good thing. It would pay to check both (upper and lower) or convert to lowercase, or best use a defined constant,

  2. Why are you using degrees, NO Math functions uses degrees, the only time you need to use degrees is for output, which generally is never needed. Learn to work in radians, it makes a lot of angle related maths so much easier.

  3. The code is just too complex for something so simple. The problem is just one of negating the scalar associated with the direction of reflection. I.E. a vector has two scalars x,y if you reflect along X you negate x, if you reflex along Y you negate y.

    Messing around with the cyclic angle is not needed. I have a general rule of thumb that if I ever find my self having to normalize a cyclic value E.G. (angle % 360) I am doing it wrong and there is a better way.


Rewrite

This is how I would write the function. I would never use degrees, that is only ever needed for display only. That makes the function much simplier.

// defined axis names
const AXIS = {x : 0, y : 1}

// function computes the reflected angle
// rad Incoming angle in radians
// dir direction of reflection. Any of AXIS, defaults to AXIS.y
function reflectAngle(rad, dir) {
    return dir === AXIS.x ? Math.acos(-Math.cos(rad)) : Math.asin(-Math.sin(rad));
}

var reflected = reflectAngle(1, AXIS.x);

Update:

Sorry totally did not add the correct example!

As explained in point 3 (above) mirror x or y depending on which axis you are reflecting from.

function reflectAngle(rad, dir) {
    const c = Math.cos(rad), s = Math.sin(rad);
    return Math.atan2(...(dir === "X" ? [s, -c] : [-s, c]));
}
\$\endgroup\$
4
  • \$\begingroup\$ this function does not actually achieve its purpose. it appears to bounce the object back in the opposite direction from where it came (so 180°); you could do the same with (rad+Math.PI) \$\endgroup\$
    – Jason FB
    Jan 29 '20 at 21:36
  • \$\begingroup\$ you solution does this streamable.com/u8b29 \$\endgroup\$
    – Jason FB
    Jan 29 '20 at 21:51
  • \$\begingroup\$ note: my orig comments from Jan 29 appear to have been before your updated section above. \$\endgroup\$
    – Jason FB
    Feb 7 '20 at 15:54
  • \$\begingroup\$ I have not tried the code below "Update:", however, I still can't figure out why it is needed to use sign/cosign/tangent here at all, because there aren't any triangles being calculated, I think maybe it is unneeded use of trig functions \$\endgroup\$
    – Jason FB
    Feb 7 '20 at 15:56
1
\$\begingroup\$

I'm not certain I'm completely happy with this yet but it seems to do the job for this question

angleOfReflection(angle_in_radians, edges) {
    // angle_in_radians is the angle the thing is currently traveling (in radians)
    // edges should be an array of strings should be strings
    // none or any of 'right', 'top', 'left', 'bottom'
    let new_angle = Number(angle_in_radians)

    edges.map((edge) => {
      if(edge === 'left' || edge === 'right') {
        new_angle = Math.PI - angle_in_radians
      }

      if(edge === 'top' || edge === 'bottom') {
        new_angle = Math.PI*2 - angle_in_radians
      }
    })

    if (new_angle < 0) new_angle = (Math.PI*2)+new_angle
    return new_angle
  }

see video

https://streamable.com/8fegw

\$\endgroup\$
4
  • 1
    \$\begingroup\$ To win at the game of war is not to play. \$\endgroup\$
    – greybeard
    Jan 30 '20 at 6:37
  • \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ Jan 30 '20 at 17:11
  • \$\begingroup\$ I think it is mathematically and fundamentally the same as the OP's code but it is simplified, improved (As the author asked for), and uses radians instead of degrees. \$\endgroup\$
    – Jason FB
    Feb 7 '20 at 15:54
  • \$\begingroup\$ @TobySpeight-- I would suggest perhaps this question is not appropriate to only be here on Code review and would be more appropriate in fact for the main stackexchange forum. IN particular, you will note that searches for "angle of reflection in javascript" do not return many or any code results other than this page, so it seems to me this page is asking a larger question than what is presented. \$\endgroup\$
    – Jason FB
    Feb 7 '20 at 19:20

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.