-
Notifications
You must be signed in to change notification settings - Fork 19
Baraah A. #19
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
base: main
Are you sure you want to change the base?
Baraah A. #19
Conversation
mnvacym
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job on your assignment. Some improvements are needed. I have left some comments. Please let me know if anything is unclear.
| // Step 1: prompt the user to enter a year | ||
| let input = prompt('Enter a year: '); | ||
| // Step 2: convert the user input to a number so we can perform calculations | ||
| let year = Number(input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you used variables with clear names. But since these are constant variables, meaning that they are not re-assigned a value, it is a good practice to use const instead of let. So that when a future developer reads this code, it is clear that these variables are constant and don't change.
| else { | ||
|
|
||
| if (year % 400 === 0){ | ||
| console.log('yes,' + year + ' is a leap year'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details matter. The output should be Yes, <YEAR> is a leap year. With a capital Y for yes, and a space after the comma.
| console.log('yes,' + year + ' is a leap year'); | ||
| } | ||
| else if(year % 100 === 0){ | ||
| console.log('No ,'+ year + ' is not a leap year'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a space before the comma. It needs to be after the comma.
| console.log('Yes,'+ year + ' is a leap year'); | ||
| } | ||
| else { | ||
| console.log('No ,'+ year + ' is not a leap year'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you met all the required conditions. However, one improvement would be to follow the DRY principle. In programming, there is a principle called DRY (Don't Repeat Yourself). So, in this case, for example, you could create a variable called isLeapYear and reuse it wherever needed. Also, this way you could avoid repeating the same console statement, and the code becomes more readable for the future. Like, for example:
const isLeapYear = (year % 4 === 0) && (year % 100 !== 0 || year % 400 === 0);
if (isLeapYear) {
console.log(`Yes, ${year} is a leap year`);
} else {
console.log(`No, ${year} is not a leap year`);
}
| else { | ||
| console.log('No ,'+ year + ' is not a leap year'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Styling of the code matters. The code would be more readable if you have consistent indentations.
| } | ||
| // Use incorrectAttempts to track the number of failed attempts | ||
| else { | ||
| incorrectAttempts++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, you need to check if incorrectAttempts has reached 4. If it is 4, you need to show the error message.
if (incorrectAttempts === 4) {
errorMessage('Login blocked: Too many incorrect attempts');
return;
}
| console.log("1: Convert EUR to USD"); | ||
| console.log("2: Convert USD to EUR"); | ||
| const menuSelection = prompt("Select your option [1 or 2]: "); | ||
| console.log("Display the current exchange rate"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output should be 3: Display the current exchange rate
| const eurAmountInput = prompt("Enter amount in EUR: "); | ||
| const eurAmountNum = Number(eurAmountInput); | ||
| if (Number.isNaN(eurAmountNum) || eurAmountNum > 0) { | ||
| if (Number.isNaN(eurAmountNum) || eurAmountNum <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice catch! The input indeed should be a positive number (excluding 0).
| console.log("the current exchange rate is 1 EUR = "+ EUR_USD_RATE + "USD.") | ||
|
|
||
| } else { | ||
| console.log("Invalid selection. Please choose either 1 or 2."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output should be updated to reflect that you now have 3 options.
console.log("Invalid selection. Please choose either 1, 2 or 3.");
| console.log(usdAmountNum.toFixed(2) + ' USD is equal to ' + eurAmount.toFixed(2) + ' EUR.'); | ||
| } | ||
| } else if (menuSelection==="3"){ | ||
| console.log("the current exchange rate is 1 EUR = "+ EUR_USD_RATE + "USD.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details matter. The output message should be case sensitive with correct spaces in place. It might be easier to use template literals:
console.log(`The current exchange rate is 1 EUR = ${EUR_USD_RATE} USD.`)
No description provided.