Skip to content

London | 26-ITP-May | Jorvan White | Sprint 2 | Coursework - #1418

Open
JorvanW wants to merge 23 commits into
CodeYourFuture:mainfrom
JorvanW:coursework/sprint-2
Open

London | 26-ITP-May | Jorvan White | Sprint 2 | Coursework #1418
JorvanW wants to merge 23 commits into
CodeYourFuture:mainfrom
JorvanW:coursework/sprint-2

Conversation

@JorvanW

@JorvanW JorvanW commented Aug 10, 2026

Copy link
Copy Markdown

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

Created Pull Request from Sprint 2 Coursework and updated for Pull Request

@github-actions

This comment has been minimized.

@JorvanW JorvanW changed the title London | 26-ITP-May | Jorvan White | Sprint 2 - Coursework London | 26-ITP-May | Jorvan White | Sprint 2 | Coursework Aug 10, 2026
@github-actions

This comment has been minimized.

@JorvanW JorvanW added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 10, 2026
@github-actions

This comment has been minimized.

@github-actions github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 10, 2026
@JorvanW JorvanW added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 10, 2026
@github-actions

This comment has been minimized.

@github-actions github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 10, 2026
@JorvanW JorvanW added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 10, 2026
@Poonam-raj Poonam-raj 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 11, 2026
Comment thread Sprint-2/debug/recipe.js
Comment thread Sprint-2/implement/lookup.test.js Outdated
Comment on lines +3 to +5
test("creates a country currency code lookup for multiple codes",() => {
expect(createLookup(countryCurrencyPairs)).toEqual([[US, 'USD'], [CA, 'CAD'], [EN, 'GBP']]);
})

@Poonam-raj Poonam-raj Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could test one currency pair first, maybe something not included in this list, and then move into a bigger list. Having two tests with different inputs really helps prove that things aren't hardcoded in your solution.

Comment thread Sprint-2/implement/lookup.test.js Outdated
Comment thread Sprint-2/implement/querystring.js Outdated
Comment thread Sprint-2/implement/tally.test.js Outdated
Comment thread Sprint-2/implement/tally.js Outdated
Comment on lines +1 to +20
function tally(array) { //(array) is the input to the function

if (!Array.isArray(array)){ //checking if array is actually an array
throw new Error("Input must be an array");
}

if (array.length === 0){ //if array is empty return a empty array
return []
}


const result = {} //creates result as an empty object which will store the counts


for (const item of array){ //'for' loops through every item in the array
if (result[item]){ // checks if the item is already in the result{}
result[item]++} // If it is then '++' tells it to add 1 to the value

else {
result[item] = 1; // else is saying that if it doesn't exist then we give it a value of 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I generally appreciate the comments adding clarity for future ref and readers, but it's not needed and maybe makes the committed code messy. This solution is fairly easy to interpret without it and we want clean commits as much as possible

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this did not remove comments - please remove unnecesary comments

Comment thread Sprint-2/interpret/invert.test.js Outdated
Comment thread Sprint-2/interpret/invert.js Outdated
Comment thread Sprint-2/interpret/invert.js Outdated
Comment thread Sprint-2/interpret/invert.js

@Poonam-raj Poonam-raj left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some great work writing tests and implementing solutions - a few comments worth addressing, especially in the interpret section where testing is particularly lacking

@Poonam-raj Poonam-raj 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 Aug 11, 2026
@JorvanW JorvanW 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 Aug 13, 2026
@Poonam-raj Poonam-raj 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 17, 2026

@Poonam-raj Poonam-raj left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small remaining bits here - good work making corrections. I did leave some comments about test cases which you did not address in the implement section (particularly lookup), can you look at that and my other remainign comments - thanks!

@Poonam-raj Poonam-raj 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 Aug 17, 2026
@JorvanW

JorvanW commented Aug 17, 2026

Copy link
Copy Markdown
Author

I think github is down (at this time of writing) and there's some syncing issues but on my end the comments from the tally is gone and I currently can't load any of the comments you've made previously.
Screenshot From 2026-08-17 16-50-35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants