Skip to content

Conversation

@habibalftrh
Copy link

I have completed challenges 1 and 2, please review my code. Thank you

Copy link
Contributor

@ianriizky ianriizky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check all of my reviews and don't hesitate for asking me if you have any questions. Thank you very much.

* @param {number} sequence
*/
function generateFibonacciUsingLoop(sequence) {
// write your code here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this unnecessary comment.

Comment on lines +11 to +12
fibonacci.pop();
return fibonacci;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set a line break between each part of your code. For example:

fibonacci.pop();

return fibonacci;

So the other team will easily read your code.

Comment on lines +8 to +15
let fibonacci = [0, 1];

if (sequence == 1) {
fibonacci.pop();
return fibonacci;
} else if (sequence == 2) {
return fibonacci;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have else if code with return statement like this, you can make it more readable by writing it this way.

let fibonacci = [0, 1];

if (sequence == 2) {
  return fibonacci;
}

if (sequence == 1) {
  fibonacci.pop();

  return fibonacci;
}

* @param {number} sequence
*/
function generateFibonacciUsingRecursive(sequence) {
// write your code here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my review at #11 (comment).

Comment on lines +42 to +43
}
return array;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my review at #11 (comment).

Comment on lines +21 to +29
value = value.replace(/[^a-zA-Z]/g, "").toLowerCase();
let valueLen = value.length;

for (let index = 0; index < Math.floor(valueLen / 2); index++) {
if (value[index] !== value[valueLen - index - 1]) {
return false;
}
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my review at #11 (comment).

Comment on lines +33 to +39
value = value.replace(/[^a-zA-Z]/g, "").toLowerCase();
if (index >= Math.floor(value.length / 2)) {
return true;
}
if (value[index] !== value[value.length - 1 - index]) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my review at #11 (comment).

Comment on lines +54 to +61
const resultObj = new Fibonacci(sequence);
if (method === "loop") {
result = resultObj.generateFibonacciUsingLoop(sequence);
} else if (method === "recursive") {
result = resultObj.generateFibonacciUsingRecursive(sequence);
} else {
throw new Error("Method must be loop or recursive.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep our main code clear. So I think this "method" checking can be organized into one method on the Fibonacci class.

Comment on lines +43 to +53
const resultObj = new Factorial(n);

let result = 0;

if (method == "loop") {
result = resultObj.calculateFactorialUsingLoop();
} else if (method == "recursive") {
result = resultObj.calculateFactorialUsingRecursive();
} else {
throw new Error("Method must be loop or recursive.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my review at #11 (comment).

Comment on lines +52 to +59
let result;
if (method === "reverse") {
result = new Palindrome(word).isPalindromeUsingReverse(word);
} else if (method === "loop") {
result = new Palindrome(word).isPalindromeUsingLoop(word);
} else if (method === "recursive") {
result = new Palindrome(word).isPalindromeUsingRecursive(word);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my review at #11 (comment).

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