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 | Nohe-Tekelmariyam | JS1-Module | Week-3 #185

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nohetekelmariyam
Copy link

@nohetekelmariyam nohetekelmariyam commented Dec 3, 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

@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.

Well done, there area couple of places to fix, and this will be good for a review again.

@@ -21,3 +21,18 @@
// Identify Reflex Angles:
// When the angle is greater than 180 degrees and less than 360 degrees,
// Then the function should return "Reflex angle"
function getAngleType(Angles) {
Copy link

Choose a reason for hiding this comment

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

start your variable name with a small letter, i.e angles instead of Angles

return "Acute angle";
} else if (Angles == 180) {
return "Straight angle";
} else if (Angles > 180 && AngleS < 360) {
Copy link

Choose a reason for hiding this comment

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

your variable name is not AnglesS change that, this code wont run

return "it's not angle";
}
}
var AngleS = 350;
Copy link

Choose a reason for hiding this comment

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

small letter for starting variable name. Also use const to declare a variable if the value will never change

@@ -33,3 +33,16 @@
// 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(numerator, denominator) {
if (numerator >= denominator && denominator != 0) {
Copy link

Choose a reason for hiding this comment

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

you should take care of error first, check if the denomiator is 0 then return an error, then go to do the next check.

if (numerator >= denominator && denominator != 0) {
return "false";
} else if (numerator < denominator) {
return "True";
Copy link

Choose a reason for hiding this comment

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

a boolean true does not start with a capital letter

) {
return "false";
} else {
return "turn";
Copy link

Choose a reason for hiding this comment

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

you are returning turn here

side1 + side2 <= side3 ||
side1 + side3 <= side2 ||
(side2 + side3 <= side1 && side1 <= 0) ||
side2 <= 0 ||
Copy link

Choose a reason for hiding this comment

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

why is there no standalone check for side1 <= 0 ?

function formatAs12HourClock(time) {
const clockHourIn12 = Number(time.slice(0, 2));
if (clockHourIn12 > 12) {
return `${clockHourIn12 - 12}:00 pm`;
Copy link

Choose a reason for hiding this comment

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

if a value like 18:46 is send in here, the result according to your code will be 6:00pm which is wrong. Also is the time send is 09:00 the result will be 09:00 without the am appended. You need to take care of these cases

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.

2 participants