Turtle graphics

[David-Ko]

Hi Max,

This is the pull request for my week-2 (Turtle Graphics) homework.

Thanks,
David

[maxwellgordon]

Since you are initializing this.coordinates here, you can set it's initial value to [this.x, this.y] as opposed to doing in two steps, like you did on lines 8 and 9

this.coordinates = [this.x, this.y];

[David-Ko]

THanks Max. Changes made.

[maxwellgordon]

try to avoid extra whitespace within your functions

[David-Ko]

Thanks Max. Changes made.

[maxwellgordon]

when defining a function, please be consistent with your spacing. I notice here you use the following:

functionName (param1, param2){

while on line 13 you do

functionName(){

and on 20 you do

functionName() {

I prefer the following spacing:

function functionName(param1, param2) {

(the function keyword can be omitted when the method is defined within a class)

This consistency is important for potential employers looking at your code. It shows attention to detail. Similarly, so does consistently with using (or not using) ;. To be inconsistent with either is akin to having typos in your resume. It immediately sends the wrong first impression.

Again, I prefer to always use semi-colons after every line that needs them.

[David-Ko]

Thanks Max. Changes made.

[maxwellgordon]

It is helpful to give your variables names that are descriptive.
Here x is confusing. With a name like x, given the context of the problem, it is easy for one to assume it has something to do with an x-coordinate, especially since we are using findMax to the find the maximum x- and y-coordinates within the array allCoordinates.
A better, more descriptive name for the parameter would have been coordinateArray or something like that

[maxwellgordon]

Similarly, findMax could have been named a little better to something like findMaxXY or findMaxXYCoordinates which one would assume would return a value like

return [biggestX, biggestY];

And then you would use it as follows on line 90:

const maxCoords = findMaxXYCoordinates(allPoints);

and not you could use maxCoords within your print and get rid of your maxCoordinates array of length 1.

[maxwellgordon]

when submitting your homework, same with when you would (later in your career) submit any PR, please remove any commented out lines of code.
You can always leave comments and they are encouraged (like you did on line 112. But please remove any commented out code (like on lines 108 and 109)

[David-Ko]

Thanks Max. Changes made.

[maxwellgordon]

Very good work around. I know you were unable to get the following to work

new Turtle(0, 0)
  .forward(5)
  .right()
  .forward(5)
  .right()
  .forward(5)
  .right()
  .forward(5)
  .print()

using chaining.

The reason this wasn't working was because you could only call right or forward (or any other method belonging to the turtle class) on an instance of a turtle.
I can see in your work around, that's exactly what you did. You realized that chaining wasn't working so you used your turtle instance (Sonic) to call the methods one at a time.

In order to get chaining to work, you need to make each of those methods described above (right, left, forward) have a return value.
Currently, since there is no explicit return statement, each method is implicitly returning undefined.
Because of this implicit return of undefined, when you chain a right onto a call to forward like below:

new Turtle (0,0)
    .forward(5)
    .right()

JS/Node thinks you are trying to invoke the right method on this implicitly returned undefined.
It can be thought of like this:

const turtle = new Turtle(0,0);
const turtle1 = turtle.forward(5); // turtle1 becomes undefined
turtle1.right()

So, what we would like is for each of those methods to return the turtle.
To do this, we need each method to return this at the end of it.
Like this:

 forward(steps) {
    // ...
     return this;
  }

 right() {
    // ...
     return this;
  }

 left() {
    // ...
     return this;
  }

And now chaining will work!

[David-Ko]

Thanks Max. Changes made. I can't believe i didn't think about that!

[maxwellgordon]

this would be an excellent place to use a switch statement!

left() {
    switch(this.face) {
        case 'east':
            this.face = 'north';
            break;
        case 'north':
            this.face = 'west';
            break;
        case 'west':
            this.face = 'south';
            break;
        case 'south':
            this.face = 'east';
            break;
    }
}