Feature/optimize day 09 #52
Conversation
Codacy's Analysis Summary0 new issue (≤ 1 minor issue) Review Pull Request in Codacy →
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@sourcery-ai review |
Reviewer's GuideRefactors the Day 09 "movie theater" solution by introducing a new sweep-line based MovieTheaterRefactor implementation that operates on precomputed rectangles and comparable 1D segments, replacing the old per-rectangle sweep logic and updating tests to target the new implementation. Sequence diagram for sweep-based rectangle validation in solveBsequenceDiagram
actor Caller
participant MTR as MovieTheaterRefactor
participant RectP as Rectangle
participant PQ as PriorityQueue_Event1DRefactor
participant Ctx as SweepContext
Caller->>MTR: solveB()
loop generate rectangles
MTR->>MTR: new Rectangle(a, b)
MTR->>RectP: Rectangle(a, b)
RectP-->>MTR: Rectangle instance
MTR->>MTR: rectangles.add(Rect)
end
MTR->>MTR: sweep()
MTR->>MTR: sweep(true)
MTR->>MTR: prepareEvents(true)
MTR->>PQ: create vertical events
PQ-->>MTR: PriorityQueue events
MTR->>Ctx: new SweepContext(true)
loop process vertical events
MTR->>PQ: poll()
PQ-->>MTR: Event1DRefactor evt
alt evt.type == START
MTR->>Ctx: addSegment(evt.segment)
else evt.type == END
MTR->>Ctx: removeSegment(evt.segment)
Ctx->>Ctx: validate activeRectangles
else evt.type == START_RECTANGLE
MTR->>Ctx: addRectangle(evt.rectangle)
Ctx->>Ctx: validate(rectangle)
else evt.type == END_RECTANGLE
MTR->>Ctx: removeRectangle(evt.rectangle)
end
end
MTR->>MTR: sweep(false)
MTR->>MTR: prepareEvents(false)
MTR->>PQ: create horizontal events
PQ-->>MTR: PriorityQueue events
MTR->>Ctx: new SweepContext(false)
loop process horizontal events
MTR->>PQ: poll()
PQ-->>MTR: Event1DRefactor evt
alt START / END / START_RECTANGLE / END_RECTANGLE
MTR->>Ctx: update segments/rectangles
Ctx->>Ctx: validate as needed
end
end
MTR->>MTR: rectangles.stream().filter(isValid)
MTR->>MTR: mapToLong(Rect.getArea).max()
MTR-->>Caller: maxArea
Class diagram for refactored Day09 movie theater sweep-line modelclassDiagram
class MovieTheaterRefactor {
- List~Point~ redTiles
- Set~Rectangle~ rectangles
- Set~Segment1D~ verticalSegments
- Set~Segment1D~ horizontalSegments
+ MovieTheaterRefactor(List~String~ inputs)
+ long solveA()
+ long solveB()
- void sweep()
- void sweep(boolean isVerticalSweep)
- PriorityQueue~Event1DRefactor~ prepareEvents(boolean isVerticalSweep)
- PriorityQueue~Event1DRefactor~ prepareVerticalEvents()
- PriorityQueue~Event1DRefactor~ prepareHorizontalEvents()
- long calculateArea(Point firstCorner, Point secondCorner)
}
class Rectangle {
- long area
- Set~Segment1D~ segments
- Set~Segment1D~ verticalSegments
- Set~Segment1D~ horizontalSegments
- boolean isValid
+ Rectangle(Point a, Point b)
+ long getArea()
+ Set~Segment1D~ getSegments()
+ Set~Segment1D~ getVerticalSegments()
+ Set~Segment1D~ getHorizontalSegments()
+ boolean isValid()
+ void setValid(boolean isValid)
}
class SweepContext {
- TreeSet~Segment1D~ activeSegments
- Set~Rectangle~ activeRectangles
+ SweepContext(boolean isVerticalSweep)
+ void addSegment(Segment1D segment)
+ void removeSegment(Segment1D segment)
+ void addRectangle(Rectangle rectangle)
+ void removeRectangle(Rectangle rectangle)
- Rectangle validate(Rectangle rectangle)
- boolean hasCeiling(Segment1D segment)
- boolean hasFloor(Segment1D segment)
}
class Event1DRefactor {
- int coordinate
- Segment1D segment
- Rectangle rectangle
- EventType1D type
+ Event1DRefactor(int coordinate, Segment1D segment, EventType1D type)
+ Event1DRefactor(int coordinate, Rectangle rectangle, EventType1D type)
+ int compareTo(Event1DRefactor other)
}
class Segment1D {
- Point first
- Point second
- int minX
- int maxX
- int minY
- int maxY
- boolean isVertical
+ Segment1D(Point a, Point b)
+ int getMinX()
+ int getMaxX()
+ int getMinY()
+ int getMaxY()
+ boolean isVertical()
+ int compareTo(Segment1D other)
}
class ComparatorSegment1DVertical {
+ int compare(Segment1D o1, Segment1D o2)
}
class ComparatorSegment1DHorizontal {
+ int compare(Segment1D o1, Segment1D o2)
}
class EventType1D {
<<enum>>
START
START_RECTANGLE
END_RECTANGLE
END
+ int getPriority()
}
class Point {
+ int x()
+ int y()
}
MovieTheaterRefactor --> Rectangle : creates
MovieTheaterRefactor --> Segment1D : uses
MovieTheaterRefactor --> Event1DRefactor : uses
MovieTheaterRefactor --> SweepContext : controls
Rectangle --> Segment1D : composes
Rectangle --> Point : constructs
SweepContext --> Rectangle : validates
SweepContext --> Segment1D : tracks active
SweepContext --> ComparatorSegment1DVertical : uses
SweepContext --> ComparatorSegment1DHorizontal : uses
Event1DRefactor --> Segment1D : references
Event1DRefactor --> Rectangle : references
Event1DRefactor --> EventType1D : typed
Segment1D --> Point : endpoints
ComparatorSegment1DVertical ..|> Comparator : implements
ComparatorSegment1DHorizontal ..|> Comparator : implements
Segment1D ..|> Comparable : implements
Event1DRefactor ..|> Comparable : implements
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The
Segment1D.compareToimplementation only considersminXandminYwhileequals/hashCode(via Lombok) include more fields, which breaks theComparablecontract and can cause incorrect behavior in theTreeSetusages in the sweep contexts; consider either aligningequals/hashCodewith the ordering or extendingcompareToto fully distinguish segments. - In
MovieTheaterRefactor.sweep()you always use aSweepContextXfor both the vertical and horizontal event queues, even though a dedicatedSweepContextYexists; it would be clearer and likely more correct to useSweepContextXfor one axis andSweepContextYfor the other. - The rectangle event generation in
prepareVerticalEventsandprepareHorizontalEventsunconditionally labels rectangle side events asSTART_SQ_LOWER/END_SQ_HIGHERregardless of whether the segment is actually the lower/upper (or left/right) side, which makes the semantics of these event types unclear and may break assumptions in the sweep logic; consider differentiating event types based on the actual side being processed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Segment1D.compareTo` implementation only considers `minX` and `minY` while `equals`/`hashCode` (via Lombok) include more fields, which breaks the `Comparable` contract and can cause incorrect behavior in the `TreeSet` usages in the sweep contexts; consider either aligning `equals`/`hashCode` with the ordering or extending `compareTo` to fully distinguish segments.
- In `MovieTheaterRefactor.sweep()` you always use a `SweepContextX` for both the vertical and horizontal event queues, even though a dedicated `SweepContextY` exists; it would be clearer and likely more correct to use `SweepContextX` for one axis and `SweepContextY` for the other.
- The rectangle event generation in `prepareVerticalEvents` and `prepareHorizontalEvents` unconditionally labels rectangle side events as `START_SQ_LOWER`/`END_SQ_HIGHER` regardless of whether the segment is actually the lower/upper (or left/right) side, which makes the semantics of these event types unclear and may break assumptions in the sweep logic; consider differentiating event types based on the actual side being processed.
## Individual Comments
### Comment 1
<location> `src/main/java/com/adventofcode/flashk/day09/Segment1D.java:10-19` </location>
<code_context>
@Getter
@EqualsAndHashCode
-public class Segment1D {
+public class Segment1D implements Comparable<Segment1D> {
- private Point up;
- private Point down;
- private Point left;
- private Point right;
- private boolean isVertical;
+ private final Point first;
+ private final Point second;
+ private final int minX;
+ private final int maxX;
+ private final int minY;
+ private final int maxY;
+ private final boolean isVertical;
- public Segment1D(Point first, Point second) {
+ public Segment1D(Point a, Point b) {
+ first = a;
+ second = b;
+ minX = Math.min(first.x(), second.x());
+ maxX = Math.max(first.x(), second.x());
+ minY = Math.min(first.y(), second.y());
+ maxY = Math.max(first.y(), second.y());
+ isVertical = first.x() == second.x();
+ }
+
+ @Override
+ public int compareTo(@NotNull Segment1D other) {
- if(first.x() == second.x()) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Segment1D.compareTo is inconsistent with @EqualsAndHashCode and may break TreeSet-based logic.
compareTo only uses minX and minY, while Lombok’s @EqualsAndHashCode includes all fields (first, second, minX, maxX, minY, maxY, isVertical). This allows two non-equal segments to compare as 0, violating the Comparable contract and breaking TreeSet behavior (dropped entries, wrong ceiling/floor). Please either make compareTo fully consistent with equals or remove @EqualsAndHashCode and define equals/hashCode to match the ordering semantics.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/adventofcode/flashk/day09/refactor/SweepContextX.java:12-21` </location>
<code_context>
+ private final TreeSet<Segment1D> activeSegments = new TreeSet<>();
</code_context>
<issue_to_address>
**issue (bug_risk):** The ordering in TreeSet<Segment1D> does not align with how hasCeiling/hasFloor are used for horizontal segments.
Because activeSegments uses Segment1D.compareTo (minX, then minY), ceiling/floor on horizontal segments won’t reflect vertical neighbors (above/below in Y); horizontally offset but vertically stacked segments may not be adjacent. To keep ceiling/floor semantically correct, use a comparator that orders primarily by Y (then X) where these methods are used, or separate X-sweep and Y-sweep structures with appropriate comparators.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public class Segment1D implements Comparable<Segment1D> { | ||
|
|
||
| private Point up; | ||
| private Point down; | ||
| private Point left; | ||
| private Point right; | ||
| private boolean isVertical; | ||
| private final Point first; | ||
| private final Point second; | ||
| private final int minX; | ||
| private final int maxX; | ||
| private final int minY; | ||
| private final int maxY; | ||
| private final boolean isVertical; | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): Segment1D.compareTo is inconsistent with @EqualsAndHashCode and may break TreeSet-based logic.
compareTo only uses minX and minY, while Lombok’s @EqualsAndHashCode includes all fields (first, second, minX, maxX, minY, maxY, isVertical). This allows two non-equal segments to compare as 0, violating the Comparable contract and breaking TreeSet behavior (dropped entries, wrong ceiling/floor). Please either make compareTo fully consistent with equals or remove @EqualsAndHashCode and define equals/hashCode to match the ordering semantics.
src/main/java/com/adventofcode/flashk/day09/refactor/SweepContextX.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This Pull Request introduces a significant refactoring of the Day 09 solution, aiming to optimize it using a sweep-line algorithm with new abstractions. While the diffCoverage is high at 98.21%, the overall project coverage has decreased, and there are specific uncovered branches in the Segment1D.compareTo method. A major logical concern has been identified in the SweepContext.validate method, which currently misapplies the TreeSet.ceiling and TreeSet.floor operations for enclosure checks, potentially leading to incorrect rectangle validation. This issue should be addressed before merging.
💡 Codacy uses AI. Check for mistakes.
| private Rectangle validate(Rectangle rectangle) { | ||
|
|
||
| long validSides = rectangle.getSegments() | ||
| .stream() | ||
| .filter(this::hasCeiling) | ||
| .filter(this::hasFloor) | ||
| .count(); | ||
|
|
||
| if (validSides != 4) { | ||
| rectangle.setValid(false); | ||
| } | ||
|
|
||
| return rectangle; | ||
|
|
||
| } | ||
|
|
||
| private boolean hasCeiling(Segment1D segment) { | ||
| return activeSegments.ceiling(segment) != null; | ||
| } | ||
|
|
||
| private boolean hasFloor(Segment1D segment) { | ||
| return activeSegments.floor(segment) != null; | ||
| } |
There was a problem hiding this comment.
🔴 HIGH RISK
The validate method's hasCeiling and hasFloor logic, when applied using activeSegments.ceiling(segment) and activeSegments.floor(segment) on Segment1D objects, might not be correctly performing the enclosure check.
Typically, for sweep-line algorithms checking enclosure, you would query active segments based on a single coordinate value (e.g., is there a polygon segment whose Y coordinate is at or below the rectangle's minY?). The Segment1D.compareTo (or its comparators) compares segments based on minX then minY (or vice-versa), which is suitable for ordering segments but not for determining if a rectangle's boundary is enclosed by polygon segments by checking a single coordinate axis. This needs to be re-evaluated to ensure the logic correctly identifies valid enclosed rectangles as per the problem's requirements. Consider re-implementing hasCeiling and hasFloor to work with coordinate values (e.g., segment.getMinY(), segment.getMaxY()) and querying a TreeSet of relevant coordinates, similar to how the old MovieTheater handled yCoordinates.floor(squareLowerY).
| return Integer.compare(minX, other.minX); | ||
| } | ||
|
|
||
| // Order by lowest 'y' coordinate | ||
| return Integer.compare(minY, other.minY); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The compareTo method has uncovered branches. Specifically, the return Integer.compare(minX, other.minX); (line 35) and return Integer.compare(minY, other.minY); (line 39) statements are not being executed by tests. This indicates that there are no test cases where two Segment1D objects have different minX values, or where they have the same minX but different minY values. Please add test cases to ensure these branches are covered and the comparison logic behaves as intended.
| public class ComparatorSegment1DHorizontal implements Comparator<Segment1D> { | ||
|
|
||
| @Override | ||
| public int compare(Segment1D o1, Segment1D o2) { | ||
|
|
||
| if(o1.getMinX() != o2.getMinX()) { | ||
| return Integer.compare(o1.getMinX(), o2.getMinX()); | ||
| } | ||
|
|
||
| return Integer.compare(o1.getMinY(), o2.getMinY()); | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The ComparatorSegment1DHorizontal class provides the exact same comparison logic as the compareTo method already implemented in Segment1D. This makes ComparatorSegment1DHorizontal redundant. You can remove this class and rely on Segment1D's natural ordering when sorting horizontal segments.
| @@ -0,0 +1,19 @@ | |||
| package com.adventofcode.flashk.day09.refactor; | |||
|
|
|||
| import static java.lang.IO.println; | |||
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The import static java.lang.IO.println; is not used in this file and should be removed.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
MovieTheaterRefactor.solveB,max().getAsLong()will throw if no rectangles remain valid; consider usingorElse(...)or handling the empty case explicitly to avoid a runtime exception in degenerate inputs. - In
SweepContext.removeSegment, you recompute validity for every active rectangle on each segment removal; if performance becomes an issue, consider narrowing revalidation to only rectangles touched by the removed segment or deferring validation until the end of the sweep.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MovieTheaterRefactor.solveB`, `max().getAsLong()` will throw if no rectangles remain valid; consider using `orElse(...)` or handling the empty case explicitly to avoid a runtime exception in degenerate inputs.
- In `SweepContext.removeSegment`, you recompute validity for every active rectangle on each segment removal; if performance becomes an issue, consider narrowing revalidation to only rectangles touched by the removed segment or deferring validation until the end of the sweep.
## Individual Comments
### Comment 1
<location> `src/main/java/com/adventofcode/flashk/day09/refactor/SweepContext.java:13-22` </location>
<code_context>
+public class SweepContext {
+
+ private final TreeSet<Segment1D> activeSegments;
+ private Set<Rectangle> activeRectangles = new HashSet<>();
+
+ public SweepContext(boolean isVerticalSweep) {
+ if(isVerticalSweep) {
+ activeSegments = new TreeSet<>(new ComparatorSegment1DVertical());
+ } else {
+ activeSegments = new TreeSet<>(new ComparatorSegment1DHorizontal());
+ }
+ }
+
+ public void addSegment(Segment1D segment) {
+ activeSegments.add(segment);
+ }
+
+ public void removeSegment(Segment1D segment) {
+ activeSegments.remove(segment);
+ activeRectangles = activeRectangles.stream()
+ .map(this::validate)
+ .filter(Rectangle::isValid)
</code_context>
<issue_to_address>
**issue (bug_risk):** Using HashSet with rectangles whose equality/hashCode depend on mutable fields can break removals from activeRectangles.
Because Rectangle uses @EqualsAndHashCode and has mutable fields (e.g., isValid and segment sets), any mutation of fields contributing to equals/hashCode after insertion into a HashSet can corrupt the set’s behavior. Here, validate(rectangle) calls rectangle.setValid before activeRectangles.remove(rectangle), so if isValid is included in equals/hashCode, the removal can silently fail and leave stale entries. Please either exclude mutable fields like isValid from equals/hashCode (e.g., base it only on the corner points/area) or use an identity-based collection for activeRectangles.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/adventofcode/flashk/day09/Segment1D.java:10-19` </location>
<code_context>
+public class Segment1D implements Comparable<Segment1D> {
</code_context>
<issue_to_address>
**issue (bug_risk):** The natural ordering of Segment1D is inconsistent with its equality and with the custom comparators, which can cause subtle TreeSet/TreeMap issues.
`@EqualsAndHashCode` considers all fields, but `compareTo` (and the vertical/horizontal comparators) only use `minX`/`minY`. Two different segments that share `minX`/`minY` but differ in length/orientation will compare as 0 while not being `equals()`, violating the `Comparable` contract and breaking `TreeSet`/`TreeMap` semantics (elements can be dropped as “duplicates”). Please either align ordering with equality (e.g., include `maxX`, `maxY`, `isVertical`) or remove `Comparable` and rely solely on explicit comparators.
</issue_to_address>
### Comment 3
<location> `src/test/java/com/adventofcode/flashk/day09/Day09Test.java:33` </location>
<code_context>
List<String> inputs = Input.readStringLines(INPUT_FOLDER, TestFilename.SAMPLE_FILE);
- MovieTheater movieTheater = new MovieTheater(inputs);
+ MovieTheaterRefactor movieTheater = new MovieTheaterRefactor(inputs);
long result = movieTheater.solveA();
</code_context>
<issue_to_address>
**suggestion (testing):** Add focused unit tests with minimal/hand-crafted inputs to exercise the new sweep logic beyond the AoC sample and full input files.
Current tests for `MovieTheaterRefactor` only cover the AoC sample and full puzzle inputs, which don’t isolate the new rectangle/sweep behavior. Please add small, hand-crafted cases (e.g., 2x2 rectangle, L-shape, shape with a hole) where the expected best rectangle for part1/part2 is explicitly asserted, so regressions in the sweep/validation logic are easier to detect and debug.
Suggested implementation:
```java
// Read input file
List<String> inputs = Input.readStringLines(INPUT_FOLDER, TestFilename.SAMPLE_FILE);
// Focused regression tests for MovieTheaterRefactor sweep/rectangle logic
// 2x2 fully available rectangle
List<String> full2x2 = List.of("..", "..");
MovieTheaterRefactor refactorFull2x2 = new MovieTheaterRefactor(full2x2);
Assertions.assertEquals(4L, refactorFull2x2.solveA(),
"2x2 full block should yield area 4 for part 1");
// L-shaped available area
List<String> lShape = List.of("..", ".#");
MovieTheaterRefactor refactorLShape = new MovieTheaterRefactor(lShape);
Assertions.assertEquals(2L, refactorLShape.solveA(),
"L-shape should yield best rectangle of area 2 for part 1");
// 3x3 with a blocked center (hole)
List<String> holeShape = List.of("...", ".#.", "...");
MovieTheaterRefactor refactorHoleShape = new MovieTheaterRefactor(holeShape);
long part1Hole = refactorHoleShape.solveA();
long part2Hole = refactorHoleShape.solveB();
Assertions.assertEquals(4L, part1Hole,
"3x3 with a center hole should yield best rectangle of area 4 for part 1");
Assertions.assertTrue(part2Hole >= part1Hole,
"part 2 should not produce a worse rectangle than part 1 for the same layout");
```
1. If the seat-layout encoding is not `"."` for available and `"#"` for blocked, adjust the `full2x2`, `lShape`, and `holeShape` definitions to match the actual input format used by `MovieTheaterRefactor`.
2. If you prefer these to be separate, focused unit tests instead of extra assertions inside an existing test, move each block into its own `@Test` method on `Day09Test` (or a nested test class), and keep the same layouts and assertions.
3. Ensure `java.util.List` and `org.junit.jupiter.api.Assertions` are imported at the top of `Day09Test.java` if they are not already.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| private Set<Rectangle> activeRectangles = new HashSet<>(); | ||
|
|
||
| public SweepContext(boolean isVerticalSweep) { | ||
| if(isVerticalSweep) { | ||
| activeSegments = new TreeSet<>(new ComparatorSegment1DVertical()); | ||
| } else { | ||
| activeSegments = new TreeSet<>(new ComparatorSegment1DHorizontal()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): Using HashSet with rectangles whose equality/hashCode depend on mutable fields can break removals from activeRectangles.
Because Rectangle uses @EqualsAndHashCode and has mutable fields (e.g., isValid and segment sets), any mutation of fields contributing to equals/hashCode after insertion into a HashSet can corrupt the set’s behavior. Here, validate(rectangle) calls rectangle.setValid before activeRectangles.remove(rectangle), so if isValid is included in equals/hashCode, the removal can silently fail and leave stale entries. Please either exclude mutable fields like isValid from equals/hashCode (e.g., base it only on the corner points/area) or use an identity-based collection for activeRectangles.
| public class Segment1D implements Comparable<Segment1D> { | ||
|
|
||
| private Point up; | ||
| private Point down; | ||
| private Point left; | ||
| private Point right; | ||
| private boolean isVertical; | ||
| private final Point first; | ||
| private final Point second; | ||
| private final int minX; | ||
| private final int maxX; | ||
| private final int minY; | ||
| private final int maxY; | ||
| private final boolean isVertical; | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): The natural ordering of Segment1D is inconsistent with its equality and with the custom comparators, which can cause subtle TreeSet/TreeMap issues.
@EqualsAndHashCode considers all fields, but compareTo (and the vertical/horizontal comparators) only use minX/minY. Two different segments that share minX/minY but differ in length/orientation will compare as 0 while not being equals(), violating the Comparable contract and breaking TreeSet/TreeMap semantics (elements can be dropped as “duplicates”). Please either align ordering with equality (e.g., include maxX, maxY, isVertical) or remove Comparable and rely solely on explicit comparators.
| List<String> inputs = Input.readStringLines(INPUT_FOLDER, TestFilename.SAMPLE_FILE); | ||
|
|
||
| MovieTheater movieTheater = new MovieTheater(inputs); | ||
| MovieTheaterRefactor movieTheater = new MovieTheaterRefactor(inputs); |
There was a problem hiding this comment.
suggestion (testing): Add focused unit tests with minimal/hand-crafted inputs to exercise the new sweep logic beyond the AoC sample and full input files.
Current tests for MovieTheaterRefactor only cover the AoC sample and full puzzle inputs, which don’t isolate the new rectangle/sweep behavior. Please add small, hand-crafted cases (e.g., 2x2 rectangle, L-shape, shape with a hole) where the expected best rectangle for part1/part2 is explicitly asserted, so regressions in the sweep/validation logic are easier to detect and debug.
Suggested implementation:
// Read input file
List<String> inputs = Input.readStringLines(INPUT_FOLDER, TestFilename.SAMPLE_FILE);
// Focused regression tests for MovieTheaterRefactor sweep/rectangle logic
// 2x2 fully available rectangle
List<String> full2x2 = List.of("..", "..");
MovieTheaterRefactor refactorFull2x2 = new MovieTheaterRefactor(full2x2);
Assertions.assertEquals(4L, refactorFull2x2.solveA(),
"2x2 full block should yield area 4 for part 1");
// L-shaped available area
List<String> lShape = List.of("..", ".#");
MovieTheaterRefactor refactorLShape = new MovieTheaterRefactor(lShape);
Assertions.assertEquals(2L, refactorLShape.solveA(),
"L-shape should yield best rectangle of area 2 for part 1");
// 3x3 with a blocked center (hole)
List<String> holeShape = List.of("...", ".#.", "...");
MovieTheaterRefactor refactorHoleShape = new MovieTheaterRefactor(holeShape);
long part1Hole = refactorHoleShape.solveA();
long part2Hole = refactorHoleShape.solveB();
Assertions.assertEquals(4L, part1Hole,
"3x3 with a center hole should yield best rectangle of area 4 for part 1");
Assertions.assertTrue(part2Hole >= part1Hole,
"part 2 should not produce a worse rectangle than part 1 for the same layout");- If the seat-layout encoding is not
"."for available and"#"for blocked, adjust thefull2x2,lShape, andholeShapedefinitions to match the actual input format used byMovieTheaterRefactor. - If you prefer these to be separate, focused unit tests instead of extra assertions inside an existing test, move each block into its own
@Testmethod onDay09Test(or a nested test class), and keep the same layouts and assertions. - Ensure
java.util.Listandorg.junit.jupiter.api.Assertionsare imported at the top ofDay09Test.javaif they are not already.
Attempt to optimize day 09 by processing all rectangles at the same time, in one or just two sweeps instead 2 sweeps per rectangle.
Summary by Sourcery
Refactor the Day 09 movie theater solution to introduce a new optimized implementation for rectangle processing and update tests to use it.
Enhancements:
Tests: