Feedback homework week 3

remarcmij commented 6 years ago

Hi Neveen, here is my feedback on your homework for week 3.

I think you have done pretty well. I particularly like how you have attempted to make your functions pure and without side effects (but see assignment 7).

Please review on how to use the ternary operator.

1-more.js

Your program doesn't tell the truth:
```
Sum of 1, 2, 5 is: 6
```

2-more.js

There is a semicolon missing in line 5 as flagged by ESLint. In general, you should follow up on all issues identified by ESLint. That's the whole point of ESLint: to give you warnings and errors where your code can (and should) be improved.

3-more.js

Excellent! But I would remove blank lines 4 & 8. And add a blank line before and after a function definition.

4-more.js

Excellent!

5-more.js

It is considered a bad practice to use more than one statement on a single line.
While you can use a ternary operator like you do here, I would prefer to use it in such a way that you use the resulting value. And of course, console.log doesn't return a value. You could do this instead:
```
'use strict';
const test = 3;
console.log(test === 3 ? 'yes' : 'no');
```
I like the way you avoided an ESLint complaint by introducing the test variable.

6-more.js

Excellent! But be consistent in how you use quotes. Either all single quotes (my preference) or all double quotes.

7-more.js

Use camelCase: Vehicles -> vehicles.
I would move the vehicles variable inside the function body, to make the function pure.
You should review the documentation for the ternary operator. It returns a conditional value. This is much cleaner than what you did:
```
const condition = age > 1 ? 'used' : 'new';
```
Also, try and avoid to reassign a parameters. It is considered a bad practice. The age parameter denotes a numeric age. The string constants 'used' and 'new' do not represent an age, but rather the condition of the car.

You don't need a for loop. You can just test the value of type to check whether it is in the range of your array and, if true, use it directly as an index. The whole code with proper formatting and use of blank lines could look like this:

'use strict';

function vehicleType(color, type, age) {
  const vehicles = ['car', 'motorbike', 'caravan', 'bike'];
  const condition = age > 1 ? 'used' : 'new';
  if (type > 0 && type <= vehicles.length) {
    return 'a ' + color + ' ' + condition + ' ' + vehicles[type - 1];
  }
  return 'unknown vehicle';
}

console.log(vehicleType('green', 3, 1));
console.log(vehicleType('red', 5, 1));

8-more.js, 9-more.js

You are almost there: there should be no comma before the word 'and':

Amazing Joe's Garage, we service cars, motorbikes, caravans, and bikes.
//                                                         ^no comma here

10-more.js

I like the way you have avoided hard-coded array indices. In JS2 week 2 you will learn other ways of doing this that do not involve using a for loop.

11-more.js

A blank line here and there would help to make the code more readable.
You produce a lot of output. I would prefer just a single line which tells whether the arrays are equal or not. In your case I have to wade through the output to draw my conclusion.
You can terminate your for loop with a break statement as soon as you have detected that they arrays are not equal. In that case, any further work in the loop is a waste of time and energy.
If I change your arrays to:
```
const x = [1, 2, 3];
const y = [1, 2, 5];
```
I get this output (only last the two lines shown), which doesn't make sense:
```
1,2,3 is not equal to 1,2,5
1,2,3 equals 1,2,5 
```
Using parseInt() on an array is not meaningful. If you look up the documentation for parseInt() you will find that it expects a string argument. So what will actually happen in your case is that, for instance, the array [1, 2, 3] is first converted to the string '1,2,3' and then parsed to the value of 1, because parseInt() stops as soon as it finds something other than a digit (in this case a comma).

12-more.js

Correct!

13-more.js

Correct! But the variable result is unneeded and can be removed.

neveenatik commented 6 years ago

Hi Jim, Thanks for your feedback I wonder why in 7 is it better not to use for loop and better to use only condition. Does it take more time maybe? and for 13 that was included in the assignment I guess it is by mistake because in the original homework there was no result variable. 🤣

What will be the value of result? (And why?) Add your explanation in a JavaScript comment.

const bar = 42; const result = typeof typeof bar;

Thank you very much for all the time you put in giving me your feedback I highly appreciate it <3

remarcmij commented 6 years ago

Hi Neveen,

In 7, the execution time required for the loop is proportional to the number of elements in the array, whereas the execution time when using an index is constant. These two cases are in computer science often denoted using the big O notation: O(N) versus O(1). See this link which I have borrowed from the HYF DataStructures repo: A beginner's guide to Big O notation. For the small array that is used here, it will not make much of a difference, but once your array grow in size this becomes very important.

As for your array comparison function, it is best to make this a pure function again, without side effects. If you follow the principle that two arrays are equal until proven otherwise, you could use a simple function like this.

function compareArrays(array1, array2) {
  if (array1.length !== array2.length) {
    return false;
  }
  for (let i = 0; i < array1.length; i++) {
    if (array1[i] !== array2[i]) {
      return false;
    }
  }
  return true;
}

Good luck!

neveenatik / hyf-javascript1