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 | Haythem Mohammed | Module-JS1 | Week4 #175

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

Conversation

haythem-f
Copy link

@haythem-f haythem-f commented Nov 29, 2023

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.

Copy link

@PERicci PERicci left a comment

Choose a reason for hiding this comment

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

Comment about nested 'else if' in password-validator.js and recommendation to use a 'switch statement' instead.

week-4/implement/is-prime.test.js Show resolved Hide resolved
week-4/implement/is-prime.test.js Show resolved Hide resolved
else {console.log("Valid!");}

}
passValidat("1a1*1F1");
Copy link

Choose a reason for hiding this comment

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

This code works fine. However this bit /[^\w\s']/g This includes special characters, which contradicts the requirement for the password to contain at least one symbol. It would be better if you update it using this: const regex = /[^\w\s\d]/g;

else {console.log(str);}
}
}
rePeat("My name is Haythem. I am a software developer in CYF", 3);
Copy link

Choose a reason for hiding this comment

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

I think the issue here is your code (if (count < 0)) is inside the loop. try putting it outside the loop, you might want to use <= instead of < to include the case where count is 0.for example: function rePeat(str, count) {
if (count <= 0) {
console.log("Error: Your input is non-positive");
} else {
for (let i = 0; i < count; i++) {
console.log(str);
}
}
}

rePeat("My name is Haythem. I am a software developer in CYF", 3);

week-4/investigate/find.js Show resolved Hide resolved
Copy link
Member

@JayMayer JayMayer Dec 13, 2023

Choose a reason for hiding this comment

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

Be careful with the requirements in this task. In a few places, the cases mention "return a new string", so we need the function to give something back rather than logging out the result.

Copy link
Member

Choose a reason for hiding this comment

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

Be careful of the requirement that says the function "should return a boolean"



test ("Get Ordinals", function () {
expect(getOrdinal(41)).toBe('st')
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 a lot of different branches that the code can take in the function. It would be really good to get a few more test cases to make sure there's good coverage!

@@ -15,3 +15,19 @@
// 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 countChar(str, char) {
Copy link
Member

Choose a reason for hiding this comment

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

Excellent work Haythem!

let cardNumber = '1009000100000108';
cardNumber = cardNumber.replace(/\D/g, ''); // to remove any non digits.
function creditCardLength(cardNumber) { // the function is to check card number length.
let theLength = (Math.log(Math.abs(cardNumber)+1) * 0.43429448190325176 | 0) + 1;
Copy link
Member

Choose a reason for hiding this comment

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

This seems like complicated logic to get the length of the credit card number. Can you think of a simpler way to get the length of the string?

@JayMayer
Copy link
Member

Great work @haythem-f. Just be careful of formatting (all code should be formatted using Prettier), and your branches. There's work coming from other weeks into your pull requests.

I would recommend checking out the main branch (git checkout main) before creating your new branch. That way, you'll be starting from a clean slate, without the work from previous weeks on your week 4 branch.

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.

4 participants