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

NW6| Sabella -Fisseha| JS1 | Week4/Exercise #180

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Sabella-8
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.


for (let i = 2; i < num; i++) {
if (num % i === 0) {
return `${num} is not prime`;
Copy link
Member

Choose a reason for hiding this comment

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

Great use of short-circuiting here @Sabella-8! If we know a number is definitely prime, we can stop the loop and return, without any more computation.

const currentOutput = getOrdinalNumber(input);
const targetOutput = "1st";

expect(currentOutput).toBe(targetOutput);
Copy link
Member

Choose a reason for hiding this comment

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

There's quite a few branches in your implementation. Don't be afraid to write lots of test cases that check different possibilities.

@@ -15,3 +15,23 @@
// And a character char that does not exist within the case-sensitive str,
// When the function is called with these inputs,
// Then it should return 0, indicating that no occurrences of the char were found in the case-sensitive str.
function countOccurrences(input, char) {
const num = input.split(char).length - 1
if(num => 1)
Copy link
Member

Choose a reason for hiding this comment

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

Check your syntax here @Sabella-8. Remember that => means something different to >=

const num = input.split(char).length - 1
if(num => 1)
return `${char} occurs ${num} times`;
else{`${char} occurs 0 times`;
Copy link
Member

Choose a reason for hiding this comment

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

Make sure that Prettier is installed and working in Visual Studio Code. This would help to keep formatting consistent, especially around if blocks

Comment on lines +11 to +13
for (let i = 0; i < num.length - 1; i += 2) {
add += num[i] + num[i + 1];
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this loop is slightly more complicated than it needs to be. If we go from i == 0 to i < num.length, we will already visit each number once. Then we only need to add the current element, instead of the current element and the one after that

@@ -0,0 +1,36 @@
// Number must be 16 digits, all of them must be numbers.
// - You must have at least two different digits represented (all of the digits cannot be the same).
// - The final digit must be even.
Copy link
Member

Choose a reason for hiding this comment

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

Is there anywhere that the final digit gets checked for odd-/even-ness?

Copy link
Contributor

@areebsattar areebsattar left a comment

Choose a reason for hiding this comment

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

It looks like you were branching from the branches themselves which resulted in a lot of file changes here! Is that the case? Apart from that I really like how you write the code as it's easy to read and understand

const currentOutput = checkPassword(str);
const targetOutput = "password is valid";
expect(currentOutput).toBe(targetOutput);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the implementation of code here it;s easy to understand

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.

3 participants