Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Cache: Disabled due to Reviews > Disable Cache setting 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Java Pelican Crossing finite-state machine with README and public tests, and a synchronized multi-threaded drone assembly system (Agent, Technician, AssemblyTable, Components) that coordinates producers/consumers to assemble 20 drones. Changes
Sequence Diagram(s)sequenceDiagram
actor Agent
participant AssemblyTable
participant Technician_A
participant Technician_B
participant Technician_C
Agent->>AssemblyTable: addComponents(c1, c2) (wait if tableFull or done)
rect rgba(100, 150, 200, 0.5)
activate AssemblyTable
AssemblyTable->>AssemblyTable: place components, mark tableFull, notifyAll
AssemblyTable-->>Agent: return
deactivate AssemblyTable
end
Technician_A->>AssemblyTable: getComponents(myComponent) (wait if not full or mismatch)
rect rgba(100, 150, 200, 0.5)
activate AssemblyTable
AssemblyTable->>AssemblyTable: verify match, assemble drone, dronesMade++
AssemblyTable->>AssemblyTable: clear table, notifyAll
AssemblyTable-->>Technician_A: return
deactivate AssemblyTable
end
par Parallel producer & consumers
Agent->>AssemblyTable: addComponents(...)
Technician_B->>AssemblyTable: getComponents(...)
Technician_C->>AssemblyTable: getComponents(...)
end
Note over Agent,Technician_C: Loop continues until dronesMade == 20
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assignment03/SYSC3303_A03_W26_Starting_code/README.txt`:
- Line 92: The README has an inconsistent description: OPERATIONAL_PEDS_FLASH is
documented as "7-tick flashing" but the code and constants use PED_FLASH_N = 6
and elsewhere say "Pedestrian Flashing (6 ticks)"; update the README text for
OPERATIONAL_PEDS_FLASH to "6-tick flashing" so it matches the implementation and
PED_FLASH_N and the other mention of "Pedestrian Flashing (6 ticks)".
In
`@assignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossing.java`:
- Around line 155-158: The current code in PelicanCrossing catches
NullPointerException around the switch on the state variable; instead, remove
the try/catch and perform an explicit null check on the state before the switch
(e.g., if (state == null) { /* handle null state: log and return/throw as
appropriate */ } ), then execute the switch over state and keep the existing
default branch logic (replace the invalid 'case default' with 'default:').
Locate the switch that references state in the PelicanCrossing class, remove the
catch block catching NullPointerException, and add a clear null-handling path so
the exception is not swallowed.
- Around line 130-131: In PelicanCrossing.java replace the redundant ternary in
the switch (currently written as switch (state != null ? state : null)) with a
direct switch on state (switch (state)) and update any pattern-match cases to
avoid invalid constructs for older Java if you intend to keep compatibility; if
you rely on Java 21+ features (e.g., comma-separated case labels with null and
pattern matching like case OPERATIONAL_CARS_GREEN_NO_PED, null: or case
default:), update the README minimum Java version to Java 21+ to reflect that
requirement and ensure build scripts/CI are aligned to that JDK.
In
`@assignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossingPublicTest.java`:
- Around line 16-96: Add a unit test that puts the PelicanCrossing into offline
mode (e.g., create PelicanCrossing, dispatch Event.OFF), then dispatch Event.ON
and assert the FSM returns to
PelicanCrossing.State.OPERATIONAL_CARS_GREEN_NO_PED and that getCarSignal() ==
PelicanCrossing.CarSignal.GREEN and getPedSignal() ==
PelicanCrossing.PedSignal.DONT_WALK_ON; reference the PelicanCrossing class,
Event.OFF/Event.ON, getState(), getCarSignal(), and getPedSignal() when locating
where to add the test.
In `@assignment04/Agent.java`:
- Line 32: The loop condition uses this.assemblyTable.getDronesAssembled() which
reads the shared dronesMade counter without synchronization, risking stale
visibility; change the implementation so reads are synchronized or mark the
dronesMade field as volatile and ensure getDronesAssembled() returns the
volatile value (or make getDronesAssembled() synchronized), and also replace the
equality check with a robust bound (use < 20 instead of != 20) in Agent.java to
avoid missing the target if the counter jumps past 20.
- Around line 40-43: The sleep/comment mismatch and swallowed interrupt should
be fixed in Agent.java: change the sleep range used in the Thread.sleep call to
match the comment (use Math.random() * 5000 for 0–5 seconds) and in the catch
block for InterruptedException do not leave it empty—restore the thread's
interrupt status by calling Thread.currentThread().interrupt() (optionally also
log or rethrow) so callers can observe the interruption; update the code around
the Thread.sleep(...) call and the InterruptedException catch to implement these
two changes.
In `@assignment04/AssemblyTable.java`:
- Line 19: Update the misleading comment on the field tableFull in class
AssemblyTable: change the text from "True if there is at least 1 component on
the table" to accurately state that tableFull is true only when both components
have been placed (e.g., "True if both components have been placed on the
table"). Ensure the comment sits immediately above the private boolean tableFull
declaration so future readers see the corrected semantics.
- Around line 47-49: The empty catch blocks around Thread.sleep in AssemblyTable
should not swallow InterruptedException; update the catch handlers in the
AssemblyTable methods that call Thread.sleep (the try/catch at the shown sleep
sites) to restore the interrupt status by calling
Thread.currentThread().interrupt() and then either return/exit or rethrow as
appropriate for the method's control flow (or at minimum log the interruption),
so the interrupt isn't lost.
- Around line 135-137: The comments alongside the Technician creation lines are
leftover copy-paste from the Cigarette Smokers problem; update the inline
comments for TechnicianFrame, TechnicianPropulsion, and TechnicianControl (the
lines that call makeNewTechnician(assemblyTable, Components.Frame),
makeNewTechnician(assemblyTable, Components.PropulsionUnit), and
makeNewTechnician(assemblyTable, Components.ControlFirmware)) to reference the
correct drone component technicians (e.g., "Frame Technician", "Propulsion
Technician", "Control Firmware Technician") instead of "Beans", "Water", and
"Sugar".
- Around line 108-110: getDronesAssembled currently reads the shared field
dronesMade without synchronization, causing possible visibility of stale values
for callers like Agent and Technician; fix this by making the getter
synchronized (declare public synchronized int getDronesAssembled()) so reads see
updates made inside synchronized blocks that modify dronesMade, or alternatively
declare dronesMade as volatile if you prefer not to synchronize the
method—choose one approach and update the getDronesAssembled method accordingly.
In `@assignment04/Components.java`:
- Around line 22-25: getRandomComponent currently instantiates a new Random on
each call which can be inefficient and lead to poor seeding; make a single
shared Random (e.g., a private static final Random RANDOM) in the Components
enum/class and use that inside getRandomComponent (or replace with
ThreadLocalRandom.current()) so the method reads
values()[RANDOM.nextInt(values().length)] (refer to getRandomComponent and the
Components type).
In `@assignment04/Technician.java`:
- Line 34: The loop condition in Technician.java uses equality in while
(this.assemblyTable.getDronesAssembled() != 20) which relies on an
unsynchronized read of shared state; change the loop to while
(this.assemblyTable.getDronesAssembled() < 20) for robustness and ensure the
shared counter accessed by getDronesAssembled() is made thread-safe by either
declaring the underlying field volatile or synchronizing the getter (or using an
AtomicInteger) in the AssemblyTable class so reads reflect updates across
threads.
- Around line 36-39: Update the misleading comment and stop swallowing the
InterruptedException in Technician: replace the copy-paste comment that mentions
"calculating n!" with a correct description like "sleep for up to 5 seconds
before performing work" and in the catch block for InterruptedException restore
the interrupt status by calling Thread.currentThread().interrupt() (or rethrow
as appropriate) so the interrupt is not lost; locate the sleep call and its
catch in the Technician class (e.g., the run or performWork method) and make
these two changes.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8220e5e-62a5-41b4-9495-260f224d36f5
⛔ Files ignored due to path filters (3)
assignment03/SYSC3303_A03_W26_Starting_code/README.pdfis excluded by!**/*.pdfassignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/.DS_Storeis excluded by!**/.DS_Storeassignment03/sysc3303aw26_a03_lavjiF543.zipis excluded by!**/*.zip
📒 Files selected for processing (10)
assignment02/assignment02/Protocol.javaassignment03/SYSC3303_A03_W26_Starting_code/README.txtassignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossing.javaassignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossingPublicTest.javaassignment03/SYSC3303_A03_W26_Starting_code/__MACOSX/SYSC3303_A03_W26_Starting_code/._.DS_Storeassignment03/SYSC3303_A03_W26_Starting_code/__MACOSX/SYSC3303_A03_W26_Starting_code/._PelicanCrossing.javaassignment04/Agent.javaassignment04/AssemblyTable.javaassignment04/Components.javaassignment04/Technician.java
📜 Review details
🔇 Additional comments (12)
assignment04/AssemblyTable.java (1)
63-72: Code is thread-safe; add defensive re-check after while loop for consistency.The
synchronizedkeyword ongetComponents()ensures only one technician can execute this method at a time, preventing the race condition where multiple technicians could both assemble drones. However, when a technician wakes fromwait()and the while condition becomes false (table full and components available), it proceeds directly to incrementdronesMadewithout re-checking ifdronesMade == 20. While the code is correct—subsequent technicians will exit early via the check inside the loop—theaddComponents()method (lines 38–40) includes a defensive re-check after its while loop. For consistency and defensive programming, add a similar check after line 72:if (this.dronesMade == 20) { return; }This ensures no technician increments
dronesMadebeyond 20, even if the condition state changes unexpectedly.assignment03/SYSC3303_A03_W26_Starting_code/README.txt (1)
1-166: Comprehensive and well-structured documentation.The README provides excellent coverage of the state machine design, including state transitions, timing behaviour, safety guarantees, and compilation instructions. This level of documentation is helpful for understanding and maintaining the FSM implementation.
assignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossing.java (5)
44-90: LGTM!The enums and timing constants are well-defined and align with the documented state machine behaviour.
165-169: LGTM!The tick method correctly dispatches Q_TIMEOUT events in a loop. If
nis negative or zero, the loop simply doesn't execute, which is acceptable behaviour.
194-234: LGTM!The initial state handler correctly processes all events with appropriate state transitions and signal updates.
240-266: LGTM!The handler correctly manages the pedestrian-waiting state. The implicit no-op for additional PEDS_WAITING events (via default case) is acceptable since pedestrians are already waiting.
272-470: LGTM!The remaining state handlers correctly implement the documented state transitions. The flash toggling logic and offline mode cycling are properly implemented.
assignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossingPublicTest.java (5)
18-24: LGTM!The test correctly validates the initial state of the FSM after construction.
26-35: LGTM!The test correctly validates that the pedestrian waiting event transitions the state while maintaining signals.
38-50: LGTM!The test correctly validates the automatic yellow transition when minimum green ends with pedestrians waiting.
52-67: LGTM!The test correctly validates the walk-to-flash transition sequence and verifies appropriate signals.
69-95: LGTM!The test effectively validates offline mode behaviour, including safe signal outputs and state toggling on timeout events.
assignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossing.java
Show resolved
Hide resolved
assignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossing.java
Show resolved
Hide resolved
...SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossingPublicTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR introduces two separate coursework codebases: a multi-threaded “drone assembly line” (agent + technicians + shared table) under assignment04/, and a Pelican Crossing finite state machine implementation + public JUnit harness + README under assignment03/.
Changes:
- Add
assignment04concurrency simulation withAgent,Technician,AssemblyTable, andComponents. - Add
assignment03Pelican Crossing FSM implementation, public JUnit tests, and accompanying README documentation. - Add a PDF version of the README, plus (unintentionally) macOS metadata files.
Reviewed changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| assignment04/AssemblyTable.java | Shared monitor coordinating agent/technicians, drone counter, and program entrypoint |
| assignment04/Agent.java | Agent thread that randomly selects and places two components |
| assignment04/Technician.java | Technician thread that waits for the two missing components to assemble drones |
| assignment04/Components.java | Enum for component types + helper to pick random component |
| assignment03/.../PelicanCrossing.java | Pelican Crossing FSM implementation (state/event dispatch + timers/signals) |
| assignment03/.../PelicanCrossingPublicTest.java | JUnit 5 public test harness for FSM behavior |
| assignment03/SYSC3303_A03_W26_Starting_code/README.txt | Build/run instructions + design/timing documentation |
| assignment03/SYSC3303_A03_W26_Starting_code/README.pdf | PDF copy of README content |
| assignment03/.../.DS_Store | macOS metadata file (should not be in repo) |
| assignment03/.../__MACOSX/.../._* | macOS resource-fork artifacts (should not be in repo) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
assignment03/SYSC3303_A03_W26_Starting_code/SYSC3303_A03_W26_Starting_code/PelicanCrossing.java
Show resolved
Hide resolved
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 7 file(s) based on 13 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Update is not a fast forward - https://docs.github.com/rest/git/refs#update-a-reference |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assignment03/SYSC3303_A03_W26_Starting_code/README.txt`:
- Around line 99-110: The README's "Pedestrian Flashing (7 ticks)" is out of
sync with the code constant PED_FLASH_N (which equals 6); update the
documentation text to "Pedestrian Flashing (6 ticks)" so it matches the
implementation and ensure any nearby references to the flashing duration reflect
PED_FLASH_N = 6.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1256d68-a87e-43d1-802a-7fcaf6c5b715
📒 Files selected for processing (1)
assignment03/SYSC3303_A03_W26_Starting_code/README.txt
📜 Review details
🔇 Additional comments (1)
assignment03/SYSC3303_A03_W26_Starting_code/README.txt (1)
92-92: Verify flash phase duration against implementation.This line documents "7-tick flashing" for OPERATIONAL_PEDS_FLASH, but a previous review indicated the implementation uses
PED_FLASH_N = 6. Line 110 also states "7 ticks". Please verify the actual constant value in the implementation and ensure the documentation is consistent.Run the following script to verify the flash duration constant:
#!/bin/bash # Description: Check the PED_FLASH_N constant value in PelicanCrossing.java # Search for the flash constant definition rg -n 'PED_FLASH_N\s*=' assignment03/Also applies to: 110-110
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
No description provided.