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

northwest |Rahwa Zeslus |module-structure-and-Testing-Data | WEEK3 #224

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

Conversation

RahwaZeslusHaile
Copy link

@RahwaZeslusHaile RahwaZeslusHaile commented Dec 9, 2024

northwest |Rahwa Zeslus |module-structure-and-Testing-Data | WEEK3

@codrex codrex self-requested a review December 10, 2024 19:33
@@ -1,8 +1,9 @@
// Predict and explain first...
// the num is already declared by the constant keyword ,which is not going to change in every logging.
Copy link

Choose a reason for hiding this comment

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

While your implementation is correct, your explanation does not clearly explain the reason for the bug. Please give your code a second look and update your explanation.

@@ -13,3 +13,10 @@
// Given someone's weight in kg and height in metres
// Then when we call this function with the weight and height
// It should return their Body Mass Index to 1 decimal place
function BMICalculation(height,adultWeight){
const heightSquare= Math.sqrt(height);
Copy link

Choose a reason for hiding this comment

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

Your BMI implementation is wrong. You are doing something incorrectly with your height. Please look up the correct formula for BMI calculation.

@@ -13,3 +13,8 @@

// You will need to come up with an appropriate name for the function
// Use the string documentation to help you find a solution
function changeToUpper(words){
Copy link

Choose a reason for hiding this comment

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

Can you come up with a more descriptive name for this function? changeToUpper does not clearly describe what the function is doing

}

console.log(toPounds("90P"))

Copy link

Choose a reason for hiding this comment

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

It would be best if you also handled edge cases like empty strings, missing "P", or invalid input.

@@ -8,3 +8,8 @@
// Given a number,
// When I call this function with a number
// it returns the new price with VAT added on
function priceWithoutVAT(price){
priceWithVAT=price+ (price*0.2);
Copy link

Choose a reason for hiding this comment

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

Can you make sure priceWithVAT is properly declared

@RahwaZeslusHaile
Copy link
Author

thank you for your constructive reviews, I updated with the corrected one .

@RahwaZeslusHaile
Copy link
Author

Please take a look when you have the chance and let me know if there are any improvements or issues you see. Thanks in advance

@RahwaZeslusHaile RahwaZeslusHaile added the Needs Review Participant to add when requesting review label Dec 17, 2024
@cjyuan
Copy link

cjyuan commented Dec 19, 2024

@RahwaZeslusHaile I think you can tag the reviewer (like what I did here with your ID) to let the reviewer know you would like a follow-up review from the reviewer.

Copy link

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

The code is quite solid.
card-validator.md has a bug that needs fixing.

Comment on lines 26 to 45
// When the angle is greater than 180 degrees and less than 360 degrees,
// Then the function should return "Reflex angle"
function getAngleType(angleMeasure){

if (angleMeasure === 90){
return "Right_Angle";
}
else if (angleMeasure < 90) {
return "Acute angle";
}
else if (angleMeasure > 90 && angleMeasure < 180) {
return "Obtuse angle";
}
else if (angleMeasure===180) {
return "Straight angle";
}
else if (angleMeasure > 180) {
return "Reflex angle";
}
}
Copy link

Choose a reason for hiding this comment

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

What do you expect from the following function calls?

getAngleType(360)
getAngleType(1000)
getAngleType(0)
getAngleType(-1000)

If the spec is not clear about how to classify 0 or negative angles, you can lookup the definition of "Acute angle".

Copy link
Author

Choose a reason for hiding this comment

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

Yes I understand ,I didn't see that way. So what I have done in this new code is try to change the degrees to to an equivalent angle within the standard range of
0 degree to 360 degree

Comment on lines 46 to 50
console.log(getAngleType(90))
console.log(getAngleType(110))
console.log(getAngleType(10))
console.log(getAngleType(180))
console.log(getAngleType(200))
Copy link

Choose a reason for hiding this comment

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

Do you notice any inconsistency among the output produced by these statements?

Comment on lines +39 to +41
if (!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 10) {
return Number(rank);
}
Copy link

Choose a reason for hiding this comment

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

What do you expect from the following function calls?
Does your function return the value you expected?

getCardValue("010♠");
getCardValue("02♠");
getCardValue("0x02♠");
getCardValue("2.1♠")

Comment on lines +51 to +53
if(a <= 0 || b <= 0 || c <= 0){
return false;
}
Copy link

Choose a reason for hiding this comment

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

If any of a, b, and c is less than or equal to zero, then the condition at line 43 will always be false.
Is there any need to further check if a, b, and c is less than or equal to zero at line 51?

I will not go into details why in some programming languages (but not JavaScript) we need also to ensure a, b, c are positives.

if (char >= "a" && char <= "z") {
// Calculate new character with wraparound
const charCode = char.charCodeAt(0);
const rotatedCode = ((charCode - 97 + shift) % 26) + 97; // 'a' starts at ASCII 97
Copy link

Choose a reason for hiding this comment

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

This approach is good.
If shift were allowed to be negative so that charCode - 97 + shift can become a negative number, how would you modify your code to deal with negative shift?

Comment on lines +38 to +65
function validateCreditCard(cardNumber) {
if (cardNumber.length !== 16) {
return false;
}

const digitSet = new Set(cardNumber);
if (digitSet.size < 2) {
return false;
}


const lastDigit = parseInt(cardNumber.charAt(cardNumber.length - 1), 10);
if (lastDigit % 2 !== 0) {
return false;
}


let sum = 0;
for (let i = 0; i < cardNumber.length; i++) {
sum += parseInt(cardNumber.charAt(i), 10);
}

if (sum <= 16) {
return false;
}

return true;
}
Copy link

Choose a reason for hiding this comment

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

Have you fully tested this function? It missed checking something in the given cardNumber.


function isPrimeNumber(number) {
if (number <= 1) {
throw new Error('There is no negative prime number or zero as a prime number.');
Copy link

Choose a reason for hiding this comment

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

How are these non-prime numbers (e.g., -100, 1, 0) any different from other non-prime numbers (e.g., 10, 15, 100)?

return false;
}

for (let index = 3; index <= Math.sqrt(number); index += 2) {
Copy link

Choose a reason for hiding this comment

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

You can possibly improve the performance of the code in the following manner:

  • Avoid calling Math.sqrt(num) repeatedly by assigning the value of Math.sqrt(num) to a variable once, and then refer to the variable in the condition of the loop.
    • Note: The condition is checked at the start of every iteration.

const previousPasswords = ['Password1!', 'Admin123#'];

it('should return false for passwords with less than 5 characters', () => {
expect(isValidPassword('P1!', previousPasswords)).toBe(false);
Copy link

Choose a reason for hiding this comment

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

The function could have also returned false because P1! does not contain any lowercase letter. How can we be 100% sure the function can correctly check passwords shorter than 5 characters?

Comment on lines +5 to +11
if (count === 0) {
return '';
}
if (count === 1) {
return str;
}
return str.repeat(count);
Copy link

Choose a reason for hiding this comment

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

These statements could be greatly shorten if you know how String.repeat() works.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants