Skip to content
This repository was archived by the owner on Apr 18, 2025. It is now read-only.

NW-6 | Zeliha Pala | JS1| [TECH ED] Complete week 4 exercises | WEEK-4 #188

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

Conversation

zelihapala
Copy link

@zelihapala zelihapala commented Dec 8, 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

@RbAvci RbAvci left a comment

Choose a reason for hiding this comment

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

Good work Zeliha! Ive just left some comments for a few sections, you can have a look.

for (let i = 0; i < str.length; i++) {
if (str[i] === char) {
count++;
}
Copy link

Choose a reason for hiding this comment

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

A good solution for the count. Also its better to keep clean code removing comment out lines. 👍

return count;
}
//const stringInput = "Harry Potter and the Philosopher's Stone"; stringInput gives a redeclaration error in here. I cleared it and still working
test("countChar - character 'e'", () => {
Copy link

Choose a reason for hiding this comment

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

test description part needs to be more specific explanation such as "works for multiple char occurrences" or "works for no char occurrences"

Copy link
Author

Choose a reason for hiding this comment

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

@RbAvci Thank you for your review. You are right. I'll provide the explanations as you suggested.


function getOrdinalNumber() {
return "1st";
}
Copy link

Choose a reason for hiding this comment

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

The getOrdinalNumber function always returns "1st" regardless of the input. If you want the function to generate ordinal numbers based on the input dynamically, you must modify the function.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @RbAvci that was so helpful I worked on this exercise and fixed. Now test cases are working

return "1st";
}

test("converts 1 to an ordinal number", function () {
Copy link

Choose a reason for hiding this comment

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

Also, you can write tests for numbers "2nd" and "3rd". In that way, you can fill in the logic inside the getOrdinalNumber function.

}

return true;
}
Copy link

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@pseudopilot pseudopilot left a comment

Choose a reason for hiding this comment

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

It's awesome job, @zelihapala !
I only left one comment with the hint for the possible optimisation. But you really did a great job here! 🥇

Comment on lines +30 to +36
} else if (count === 0) {
return "";
} else if (count === 1) {
return str;
} else {
return str.repeat(count);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to check 0 and 1 values? What would happen if we called str.repeat(0) or str.repeat(1)?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants