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

NW-6 | AREEB-SATTAR | JS1| [TECH ED] Complete week 3 exercises | WEEK-3 #154

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

Conversation

areebsattar
Copy link
Contributor

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.
I have completed the exercises except stretch here yet

Questions

Ask any questions you have for your reviewer.
I am not sure if these are the best way to implement them or if they can be improved further

@@ -1,6 +1,10 @@
function formatAs12HourClock(time) {
if (Number(time.slice(0, 2)) > 12) {
return `${Number(time.slice(0, 2)) - 12}:00 pm`;
let timeFormat = (Number(time.slice(0, 2)) - 12)
if (timeFormat <10 )
Copy link
Member

Choose a reason for hiding this comment

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

Can you think of a way this can be done without an if statement? JavaScript's standard library has a great function for this use case 😉

Copy link
Member

Choose a reason for hiding this comment

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

Nice job writing if statements that are in a working order Areeb! Only thing I'd do is swap the statements on lines 27 and 30. That way all of the angles would be in ascending order (i.e., <90 would come before ===90).

Copy link
Member

Choose a reason for hiding this comment

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

Fantastic! Adding a test specifically the code being changed is a great way to make sure it still works after the refactoring!

// I already stored it in a variable in first problem because we are using same thing twice which can make code look
// confusing and by storing it in variable it makes things easier for someone unfamiliar with code to understand.
function formatAs12HourClock(time) {
if (Number(time.slice(0, 2)) > 12) {
Copy link
Member

@JayMayer JayMayer Dec 1, 2023

Choose a reason for hiding this comment

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

I think we can take this further. What if we moved the expression Number(time.slice(0, 2)) into a variable after line 9, then use that variable in the if statement and line 11. By also giving the variable a name such as hours, it would be easier to glance at the code and tell what is happening in the logic.

else if (a<=0 || b<=0 || c<=0){
return false;
}
else if(a+b>c && a+c>b && b+c>a){
Copy link
Member

Choose a reason for hiding this comment

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

This logic here is the opposite of the logic on line 43. For example, a + b <= c must be false for us to reach this line of code. Therefore a + b > c will always be true at line 49. The same goes for the other statements.

For that reason the checks here on line 49 aren't necessary

return false;
}
else if (b=0){
return ;
Copy link
Member

Choose a reason for hiding this comment

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

Is this missing a return value?

@@ -29,3 +29,46 @@
// Given a card with an invalid rank (neither a number nor a recognized face card),
// When the function is called with such a card,
// Then it should throw an error indicating "Invalid card rank."

function getCardValue(str) {
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 acceptance criteria on line 9. str might be a value such as A♠, which contains both the card suit and value

Copy link

@bunday bunday left a comment

Choose a reason for hiding this comment

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

Great work!

else if (angle>180 && angle<360){
return "Reflex Angle"
}
else
Copy link

Choose a reason for hiding this comment

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

an angle of 360 itself is not taken care of

@@ -33,3 +35,39 @@
// Explanation: The fraction 3/3 is not a proper fraction because the numerator is equal to the denominator. The function should return false.

// These acceptance criteria cover a range of scenarios to ensure that the isProperFraction function handles both proper and improper fractions correctly and handles potential errors such as a zero denominator.

function isProperFraction(a, b) {
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 think will happen when we pass 3/-5 into your code?

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