Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code review #25

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

andrcatros
Copy link

What you did well:

  • you made a good attempt at solving all the problems on the track with your own solutions
  • you set up testing correctly
  • files are well organised and "tidy" with no unnecessary comments or console.logs

What you can improve:

  • four of your string.js functions - sayHello, uppercase, lowercase and firstCharacters - only work for the strings used as examples in the tests! I love the ingenuity of reverse engineering the tests this way, but it's not how these problems should be solved. You should write functions that work correctly with any string input, not just the 3 examples used in the tests. If you're stuck and don't know how to write these functions, please ask in the Slack channel or in class. There are a number of in-built string methods in JavaScript and it's important to learn these.

  • there's quite a lot of unnecessary if/else statements in your booleans.js file. If/else work by evaluating the truthiness/falsiness of the expressions after if/else - but you can skip the if/else and just return the Boolean value the expression evaluates to instead. The ! (NOT) JavaScript operator is very helpful here.

For example, you negate function is:

const negate = a => {
  if (a === true) {
    return false;
  } else {
    return true
  };
};

You can re-write this as:

const negate = a => {
  return !(a === true);
};

Or even const negate = a => a === false;
(You don't need to use return for single line arrow functions.)

Have a go at refactoring all your Boolean functions in this way.

  • there's also a typo on line 22 of arrays.js - you use array2 (an undefined variable) instead of array (variable declared as function parameter):
const addToArray2 = (element, array) => {
  return (array2 = array.concat(element));
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants