Skip to content

London| 26-ITP-May | Martin Mwaka | Sprint 3 | Practice tdd#1431

Open
Temceo wants to merge 9 commits into
CodeYourFuture:mainfrom
Temceo:practice-tdd
Open

London| 26-ITP-May | Martin Mwaka | Sprint 3 | Practice tdd#1431
Temceo wants to merge 9 commits into
CodeYourFuture:mainfrom
Temceo:practice-tdd

Conversation

@Temceo

@Temceo Temceo commented Jun 30, 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

Completed Test Driven Development tasks

@github-actions

This comment has been minimized.

Sign in to view
@Temceo Temceo added 🏕 Priority Mandatory This work is expected Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Structuring-And-Testing-Data The name of the module. labels Jun 30, 2026
LonMcGregor
LonMcGregor reviewed Jul 6, 2026
View reviewed changes

@LonMcGregor LonMcGregor 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.

Good work, I see one place you might be able to clean up the count implementation

Comment thread Sprint-3/2-practice-tdd/count.js Outdated
@@ -1,5 +1,10 @@
function countChar(stringOfCharacters, findCharacter) {
return 5
if (stringOfCharacters.length === 0 || findCharacter.length === 0) return 0;

@LonMcGregor LonMcGregor Jul 6, 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.

What does this if condition do in this function?

@Temceo Temceo Jul 6, 2026

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.

This condition returns 0 if either the string or the character are empty. I have updated the code to make it more readable

@Temceo Temceo Jul 7, 2026 •
edited
Loading

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.

The first line is not needed as count is initialised to zero later in the function. If count is not incremented it will return zero. I have now removed it.

@LonMcGregor 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 Jul 6, 2026
@Temceo Temceo 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 6, 2026
LonMcGregor
LonMcGregor reviewed Jul 7, 2026
View reviewed changes

@LonMcGregor LonMcGregor 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.

This is a bit more readable. I am still wondering if this if conditional needs to be there. Have you tried running the function without the if check?

@LonMcGregor 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 Jul 7, 2026
@Temceo Temceo 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 7, 2026
LonMcGregor
LonMcGregor reviewed Jul 8, 2026
View reviewed changes

@LonMcGregor LonMcGregor 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.

Good fix - it's always good to look out for possible "dead code" to simplify things

@LonMcGregor LonMcGregor added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LonMcGregor LonMcGregor LonMcGregor left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Module-Structuring-And-Testing-Data The name of the module. 🏕 Priority Mandatory This work is expected

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Temceo @LonMcGregor