Skip to content

Manchester | ITP-May-2025 | Chukwuemeke Ajuebor | Module-Structuring-and-Testing-Data | Sprint 3 #674

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

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

Conversation

AjueborChukwuemeke
Copy link

@AjueborChukwuemeke AjueborChukwuemeke commented Jul 21, 2025

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 REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR

In this PR I worked on testing the code to ensure it behaves as is expected subjected to different scenarios or given different inputs.
Started first with console.assert and then using jest to carry out better code efficient ways of testing code.

Questions
None

@AjueborChukwuemeke AjueborChukwuemeke added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 21, 2025
@kfklein15
Copy link

Hi, the PR description is missing. Can you please update it accordingly to the template? Thanks

@CameronDowner CameronDowner added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jul 26, 2025
Copy link

@CameronDowner CameronDowner left a comment

Choose a reason for hiding this comment

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

This is looking really good!

I've left a few comments - some highlighting edge cases & some highlighting areas to improve readability and code clarity, rather than logic or correctness.

Overall very strong submission

Let me know if you have any questions

// Explanation: Even though this gives a zero value it still returns true
//so we expect it to be true
const zeroNumeratorFraction = isProperFraction(0, 3);
assertEquals(zeroNumeratorFraction, "input non-zero above");

Choose a reason for hiding this comment

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

Would we expect 0/3 to be an 'error' case?

// target output: false
// Explanation: This actually outputs false even though the operation is practically impossible.
const zeroZeroFraction = isProperFraction(0, 0);
assertEquals(zeroZeroFraction, "put numbers above and below");

Choose a reason for hiding this comment

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

Does the numerator need to be non-zero?

@@ -8,7 +8,14 @@
// write one test at a time, and make it pass, build your solution up methodically

function isProperFraction(numerator, denominator) {
if (numerator < denominator) return true;
if (numerator === 0 && denominator === 0) return "put numbers above and below";

Choose a reason for hiding this comment

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

Stretch: if we re-order these if statements, could we reduce the number of conditions we need to check? Each condition adds complexity to our code

For example, if I check numerator === 0 first, will I need to check numerator !== 0 later?

@@ -8,7 +8,15 @@
// write one test at a time, and make it pass, build your solution up methodically
// just make one change at a time -- don't rush -- programmers are deep and careful thinkers
function getCardValue(card) {
if (rank === "A") return 11;
rank = card[0]

Choose a reason for hiding this comment

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

This is missing a declaration - what do we need to put in front of this variable assignment?

else if (Number(rank) >= 2 && Number(rank) <= 10) return Number(rank);
//else return "Invalid card rank.";
/* else if (card.length > 3 && !(card[0] === "1" && card[1] === "0" && isNaN(card[2])))
return "Invalid card rank"; */
}

// You need to write assertions for your function to check it works in different cases

Choose a reason for hiding this comment

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

Could you add this test case & see what it returns?

const tenOfHearts = getCardValue("10♥");
assertEquals(tenOfHearts, 10);

// The while loop goes through the string, one character at a time
// gets the character at the current position and
// checks if it matches the character we're looking for
while (i < stringOfCharacters.length) {

Choose a reason for hiding this comment

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

This looks good - if there another type of loop we could use here? Potentially to remove the need for i?

Choose a reason for hiding this comment

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

Looks good - we could also use a for...of loop here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of#iterating_over_a_string

const iterable = "boo";

for (const value of iterable) {
  console.log(value);
}
// "b"
// "o"
// "o"

let lastDigit = numStr[numStr.length - 1]; // Get last digit as string
let lastTwoDigits = numStr.slice(-2); // Get last two digits as string

//We have three special cases for the numbers 11, 12, 13

Choose a reason for hiding this comment

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

Really nice use of comments here - super easy to read

@@ -1,5 +1,18 @@
function getOrdinalNumber(num) {
return "1st";
let numStr = num.toString(); // Convert number to string

Choose a reason for hiding this comment

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

Do these need to be let?

function repeat(str,count) {
if (count < 0)
return "negative counts invalid";
else if (count === 0) return "";

Choose a reason for hiding this comment

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

What happens if I call str.repeat(0) or str.repeat(1)?

Comment on lines +7 to +8
let repeated = str.repeat(count)
return repeated;

Choose a reason for hiding this comment

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

Do we need to assign this to a variable before returning?

@CameronDowner CameronDowner added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 26, 2025
@AjueborChukwuemeke AjueborChukwuemeke added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Jul 27, 2025
@CameronDowner CameronDowner added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 2, 2025
@CameronDowner
Copy link

This is looking good & you have addressed all issues / blocking comments 🙂

Happy to mark as complete, but there a couple comments remaining that would help improve your code further so worth having a look if you have time

@CameronDowner CameronDowner added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and all review comments have been addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants