Skip to content

Conversation

@vil02
Copy link
Member

Open in Gitpodknow more

Describe your change:

This PR reduced the code duplication and adds missing tests.

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new JavaScript files are placed inside an existing directory.
  • All filenames should use the UpperCamelCase (PascalCase) style. There should be no spaces in filenames.
    Example:UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not
  • All new algorithms have a URL in their comments that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@vil02vil02 marked this pull request as ready for review March 25, 2024 19:47
@vil02vil02 mentioned this pull request Mar 25, 2024
8 tasks
appgurueu
appgurueu previously approved these changes Mar 26, 2024
Copy link
Collaborator

@appgurueuappgurueu left a comment

Choose a reason for hiding this comment

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

You could also consider making checkLocation return a boolean - it could return whether the location is valid.

Then you'd be duplicating the two throws (but this is actually not that bad, because such side effects being "hidden" in a function may be a bit surprising).

You would then be able to deduplicate the x >= 0 && x < rgbData.length && y >= 0 && y < rgbData[0].length bounds check as well.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.08%. Comparing base (bd34e9f) to head (1be1853).

Additional details and impacted files
@@ Coverage Diff @@## master #1645 +/- ## ========================================== + Coverage 84.07% 84.08% +0.01%  ========================================== Files 375 375 Lines 19688 19685 -3 Branches 2913 2917 +4 ========================================== + Hits 16552 16553 +1 + Misses 3136 3132 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@appgurueuappgurueuforce-pushed the reduce_code_duplication_in_flood_fill branch from f405dde to 1be1853CompareMarch 27, 2024 10:13
@vil02
Copy link
MemberAuthor

@appgurueu thanks for 1be1853.

@vil02vil02 changed the title refactor: reduce code duplication by adding checkLocation to FloodFillrefactor: reduce code duplication in FloodFillMar 27, 2024
Copy link
Collaborator

@appgurueuappgurueu left a comment

Choose a reason for hiding this comment

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

Ah, I forgot to approve, didn't I...

@raklaptudirmraklaptudirm merged commit d920e7f into TheAlgorithms:masterApr 3, 2024
@vil02vil02 deleted the reduce_code_duplication_in_flood_fill branch April 3, 2024 16:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@vil02@codecov-commenter@appgurueu@raklaptudirm