NotificationsYou must be signed in to change notification settings
Fork5.7k
Star33.2k

refactor: reduce code duplication in`FloodFill`#1645

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

raklaptudirm merged 4 commits intoTheAlgorithms:masterfromvil02:reduce_code_duplication_in_flood_fill

Apr 3, 2024

Merged

refactor: reduce code duplication in`FloodFill`#1645

raklaptudirm merged 4 commits intoTheAlgorithms:masterfromvil02:reduce_code_duplication_in_flood_fill

Apr 3, 2024

Conversation

Copy link

Member

vil02 commentedMar 25, 2024

know more

Describe your change:

This PR reduced thecode duplication and adds missing tests.

Checklist:

I have readCONTRIBUTING.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 butuserprofile.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 containsFixes: #{$ISSUE_NO}.

vil02 added2 commits

March 25, 2024 19:31

tests: add tests checking if floodFill funtions throw when location i…

5f143d9

…s outside

refactor: reduce code duplication by addingcheckLocation to `Flood…

0e42fe0

…Fill`

vil02 marked this pull request as ready for review

March 25, 2024 19:47

vil02 requested review fromraklaptudirm andappgurueu ascode owners

March 25, 2024 19:47

vil02 mentioned this pull request

Mar 25, 2024

style: improve test names ofGetEuclidGCD#1646

Merged

8 tasks

appgurueu previously approved these changes

Mar 26, 2024

View reviewed changes

Copy link

Collaborator

appgurueu 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 makingcheckLocation return a boolean - it could return whether the location is valid.

Then you'd be duplicating the twothrows (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 thex >= 0 && x < rgbData.length && y >= 0 && y < rgbData[0].length bounds check as well.

refactor: add and useisInside

cf34946

Co-authored-by: appgurueu <34514239+appgurueu@users.noreply.github.com>

vil02 dismissedappgurueu’sstale review viacf34946

March 26, 2024 22:18

vil02 requested a review fromappgurueu

March 26, 2024 22:31

Copy link

codecov-commenter commentedMar 27, 2024•
edited
Loading

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.