Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surround completed region newest #673

Merged
merged 6 commits into from
Nov 3, 2023

Conversation

cadogs
Copy link
Collaborator

@cadogs cadogs commented Oct 20, 2023

Description

Nurikabe Surround Region #553
Altered surround region rule so that it only works with regions of the exact right size as the number within the region. Still need to change the rule name as requested in the issue from "Surround Region" to "Surround Completed Region. Most
relevant information related to this issue can be viewed in the [BUG] Nurikabe Surround Region #553.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Enhancement (improvement to an already existing feature)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Replicated all cases in [BUG] Nurikabe Surround Region #553.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@Corppet Corppet self-requested a review October 27, 2023 20:58
Copy link
Collaborator

@Corppet Corppet left a comment

Choose a reason for hiding this comment

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

Please make sure you also fill out the checklist when opening a pull request. This not only ensures you went through the steps to submitting quality code, but helps us understand what has been done and what still needs work. It's ok to not have everything done, we just want to know the progress that has been made especially if someone else wants to work on this in the future.

DisjointSets<NurikabeCell> regions = NurikabeUtilities.getNurikabeRegions(destBoardState);
Set<NurikabeCell> adj = new HashSet<>(); //set to hold adjacent cells
Point loc = cell.getLocation(); //position of placed cell
NurikabeCell cellLeft = destBoardState.getCell(loc.x - 1, loc.y);
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this is logically correct, this code block uses duplicate code for each direction. I recommend iteration through each direction in a for loop rather than copy pasting the code four times.

Some pseudocode that might help:

import java.awt.Point;

List<Point> directions = { new Point(-1, 0), new Point(1, 0), new Point(0, -1), new Point(0, 1) };

for (Point direction : directions) {
    NurikabeCell cell = destBoardState.getCell(loc.x + direction.x, loc.y + direction.y);
    
    // avoid redundant nested clauses
    if (cell != null && (...) {
        // apply logic
    }
}

Copy link
Collaborator

@Corppet Corppet left a comment

Choose a reason for hiding this comment

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

Changes look good. I made a quick change to simply directions to one line, but everything else looks solid.

@Corppet Corppet merged commit 91366b0 into Bram-Hub:dev Nov 3, 2023
5 checks passed
Sign up for free to 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.

2 participants