WESTMIDLANDS | ITP-MAY-25 | Ahmad Ehsas | Sprint 2| DATA GROUPS #756

ahmadehsas · 2025-08-09T13:35:25Z

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 | FirstName LastName | Sprint | Assignment Title
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Changelist:

Completed all the mandatory tasks.

Questions

Ask any questions you have for your reviewer.

…l the tets.

… tests cases.

…rray and passed the tests of given arrays.

…en tests.

…seNumber property.

…by join(\n) method.

LonMcGregor · 2025-08-11T09:00:29Z

It looks like there are a lot of files included here - from multiple sprints. Can I just confirm which ones you wanted reviewed?

ahmadehsas · 2025-08-11T12:13:07Z

Subject: Request for Review of Sprinter-2 Folder I hope this message finds you well. I would like to request a review of the Sprinter-2 folder, as it is pertinent to my branch, Sprint-2. Thank you for your attention to this matter. *LonMcGregor* left a comment (CodeYourFuture/Module-Data-Groups#756)

…

<#756 (comment)> It looks like there are a lot of files included here - from multiple sprints. Can I just confirm which ones you wanted reviewed? — Reply to this email directly, view it on GitHub <#756 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BOYJJVAGCTH5O66MQ2LMQBL3NBLUFAVCNFSM6AAAAACDP5QJCWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNZTHAZTMMBYHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

LonMcGregor

Hi, I've reviewed sprint 2 now. If the sprint 1 files are not meant to be there, can you remove them from the pull request please? Also, can you fix the package-lock.json file - if you didn't intentionally change that file, it should not have any changes comitted.

For sprint 2, you have made a good start, but I have some follow up questions for you to answer

LonMcGregor · 2025-08-12T08:34:41Z

Sprint-2/debug/author.js

+// This means that we need to check if the property belongs to the object using hasownproperty whenever we loop through an object with the `for ... in` loop.
+
+for (const key in author) {
+  if (author.hasOwnProperty(key)) {


What are you using hasOwnProperty to do here?

The Sprints 1, 2 and 3 files are in the same folder, and my Sprint-1 has already been reviewed. Is it necessary to remove it?

hasOwnproperty is being used to check whether the key belongs directly to the author object itself, not to something it inherited from its prototype chain.

Thanks for answering about hasownproperty. For the files in the other sprints, if they are not relevant to this pull request, they should be removed from the branch.

(hint: the files should be restored to original state note deleted entirely)

LonMcGregor · 2025-08-12T08:35:51Z

Sprint-2/debug/recipe.js

-${recipe}`);
+// Here we add `join("\n") to the ingredients array to log each ingredient in a new line.
+console.log(
+  `${recipe.title} serves ${


Is there a reason you wrote the recipe.serves variable over multiple lines?

Breaking recipes into multiple lines just makes the final string output easier to read when you look at the code.

I meant the code itself on line 16 - is there a reason you wrote the variable recipe.serves in the template expression over multiple lines instead of one line?

I think there is no functional reason for splitting recipe. serves over multiple lines. If we write in one line or multiple lines, it doesn't change the output.

LonMcGregor · 2025-08-12T08:36:54Z

Sprint-2/implement/contains.test.js

 // When passed to contains
 // Then it should return false or throw an error
+test("contains on an array returns false", () => {
+  expect(contains([1, 2, 3], 4)).toBe(false);


What happens if you test for something like contains([1,2,3,4], "length")?

The test returns true because 'length' is a property of the array object

LonMcGregor · 2025-08-12T08:42:11Z

Sprint-2/interpret/invert.js

+// it needed because it converts the object into an array of [key, value] pairs which makes it easy to loop over both key and value at the same time.

 // d) Explain why the current return value is different from the target output
+// because it returns the [key, value] of an object in reverse order [value,key]


I think this question was asking why the original output of this program was not what was expected (before you fixed it) - can you explain why you made the change you did and what the mistake was?

Because it was required to fix the implementation and write tests to prove it is fixed. I followed the instructions and made the required changes. There were a lot of mistakes at first when I wanted to check the test, and there were errors.

LonMcGregor · 2025-08-13T12:36:42Z

Thanks for answering my questions. To review what still needs to be addressed here:

Can you please revert the changes from sprint 1 so this branch and PR only has sprint 2's files?
You are correct about recipe.serves not making a difference functionally, but it would be easier to read the code if it was all on one line. Can you fix this?
Can you address my most recent comment about making contains give a false or error response when given array as input?

github-actions · 2025-08-15T09:44:27Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (SPRINT-2) doesn't match expected format (example: 'Sprint 2', without quotes)

github-actions · 2025-08-15T09:44:33Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (SPRINT-2) doesn't match expected format (example: 'Sprint 2', without quotes)

github-actions · 2025-08-15T09:52:37Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (SPRINT-2 ) doesn't match expected format (example: 'Sprint 2', without quotes)

LonMcGregor · 2025-08-18T09:13:08Z

Hi,

This PR still has all your Sprint 1 files (the PR is titled Sprint 2, so should only have those files). You can see these if you visit the "Files Changed" tab on this page. If you were reviewing someone else's code and there were lots of unrelated files, can you see why that might be a problem?
When discussing the code being on one line, we were looking at recipe.js. If you imagine reading this file as someone else, can you see how having variables in templates over multiple lines might make it more difficult to read? What could you do to make this easier to read for someone who did not write this code?
For contains.test.js You have now got a more correct test - what happens when you run this test?

ahmadehsas added 13 commits July 28, 2025 15:16

Fixed the implementaton of the function CalculateMedian and passed al…

96e54cd

…l the tets.

Implemented the deduplicate function with given arrays and passed the…

d577680

… tests cases.

Implemented the function findMax and passed the tests for given arrays.

3ecbe9f

Implemented the function sum that sums the numerical elements of an a…

0467d03

…rray and passed the tests of given arrays.

Refactored the implementation of Includes function and passed the giv…

a9a5914

…en tests.

Fixed the code address object and used dot notation to access the hou…

d5ca4ab

…seNumber property.

Explained and fixed the problem in the mention code.

ce7733e

Fixed the mention code and printed out each ingredient in a new line …

73e8544

…by join(\n) method.

Implemented the function Contain, and passed tests for this function.

f512318

Implemented the createLookup function and passed the test.

22c4094

Fixed the implementation for this test and wrote tests.

1231d9c

Implemented the function called tally and passed tests.

aa42568

Fixed the implementation of invert function and wrote tests.

ef820ad

ahmadehsas added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 9, 2025

LonMcGregor added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 12, 2025

LonMcGregor reviewed Aug 12, 2025

View reviewed changes

LonMcGregor 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 12, 2025

passed the test for contains of an array of length returns true.

898abfc

ahmadehsas 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, 2025

LonMcGregor removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 13, 2025

LonMcGregor added the Reviewed Volunteer to add when completing a review with trainee action still to take. label Aug 13, 2025

ahmadehsas added 2 commits August 14, 2025 01:03

Fixed the code in one line with prettier-ignore.

2bc209b

fixed the test for given an array as input return false.

d0788b8

ahmadehsas 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 15, 2025

LonMcGregor added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 18, 2025

illicitonion closed this Sep 1, 2025

Uh oh!

WESTMIDLANDS | ITP-MAY-25 | Ahmad Ehsas | Sprint 2| DATA GROUPS #756

WESTMIDLANDS | ITP-MAY-25 | Ahmad Ehsas | Sprint 2| DATA GROUPS #756

Uh oh!

Conversation

ahmadehsas commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist:

Questions

Uh oh!

LonMcGregor commented Aug 11, 2025

Uh oh!

ahmadehsas commented Aug 11, 2025 via email

Uh oh!

LonMcGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LonMcGregor commented Aug 13, 2025

Uh oh!

github-actions bot commented Aug 15, 2025

Uh oh!

github-actions bot commented Aug 15, 2025

Uh oh!

github-actions bot commented Aug 15, 2025

Uh oh!

LonMcGregor commented Aug 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahmadehsas commented Aug 9, 2025 •

edited

Loading