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|Bakhat Begum|Module-JS1 |Bakha/week-3| Sprint-3|Week-3 #162

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

Conversation

BakhatBegum
Copy link

@BakhatBegum BakhatBegum commented Nov 23, 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

Some parts were complicated. Blackjack cards took new horses to understand and write the code. Over all day by day, I am getting better.

Questions

I would be very happy to get feedback.

@BakhatBegum BakhatBegum changed the title NW6Bakha/week 3 NW6|Bakhat Begum|Module-JS1 |Bakha/week-3| Sprint-3 Nov 23, 2023
@BakhatBegum BakhatBegum changed the title NW6|Bakhat Begum|Module-JS1 |Bakha/week-3| Sprint-3 NW6|Bakhat Begum|Module-JS1 |Bakha/week-3| Sprint-3|Week-3 Nov 23, 2023
Copy link

@Sabella-8 Sabella-8 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!

Choose a reason for hiding this comment

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

LGTM

Choose a reason for hiding this comment

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

Very good job!!
Since you have already identified angle === 90 as a right angle in line 29, there's no need to restate angle != 90 in line 29. The first condition (angle === 90) has been satisfied, and the code does not proceed to the second line.

Choose a reason for hiding this comment

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

In my view, users will input not only numbers but also cards with unique suits, such as 'J♥'. In this scenario, your code should be designed to convert these inputs into their corresponding numerical values.

Choose a reason for hiding this comment

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

Well done!

Choose a reason for hiding this comment

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

Good job! Additionally, ensure that 'a', 'b', and 'c' cannot be zero in your code. Since it can not be a triangle if either of the sides is 0.

Choose a reason for hiding this comment

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

LGTM

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.

a very good job you have going on here

function getAngleType(angle){
if (angle === 90){
return "Right angle";
} else if (angle != 90 && angle < 90){
Copy link

Choose a reason for hiding this comment

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

this can easily be refactored into angle < 90`. you didnt take care of negative numbers, 360 and numbers greater than 360

function isProperFraction(numerator, denominator) {
if (numerator < denominator){
return true;
}else if (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 test for 0 first

//ans : Store this expression in a variable revised code considers both the hours and minutes in the input time, correctly determines whether it's morning or afternoon.


function formatAs12HourClock(time) {
Copy link

Choose a reason for hiding this comment

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

an elegant solution.

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