Skip to content

Fix: Resolved string bounding bug in UI (Fixes #1234)#7499

Open
Abhi13shek wants to merge 2 commits into
TheAlgorithms:masterfrom
Abhi13shek:main
Open

Fix: Resolved string bounding bug in UI (Fixes #1234)#7499
Abhi13shek wants to merge 2 commits into
TheAlgorithms:masterfrom
Abhi13shek:main

Conversation

@Abhi13shek

Copy link
Copy Markdown
  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized it.
  • All filenames are in PascalCase.
  • All functions and variable names follow Java naming conventions.
  • All new algorithms have a URL in their comments that points to Wikipedia or other similar explanations.
  • All new algorithms include a corresponding test class that validates their functionality.
  • All new code is formatted with clang-format -i --style=file path/to/your/file.java

@codecov-commenter

codecov-commenter commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.23%. Comparing base (ef986c4) to head (9002e4c).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...a/com/thealgorithms/backtracking/SudokuSolver.java 14.28% 6 Missing ⚠️
...n/java/com/thealgorithms/backtracking/NQueens.java 20.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #7499      +/-   ##
============================================
+ Coverage     79.91%   80.23%   +0.31%     
- Complexity     7353     7357       +4     
============================================
  Files           811      810       -1     
  Lines         23879    23789      -90     
  Branches       4705     4679      -26     
============================================
+ Hits          19082    19086       +4     
+ Misses         4036     3944      -92     
+ Partials        761      759       -2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@prashantpiyush1111

Copy link
Copy Markdown
Contributor

Hi @Abhi13shek , thanks for the contribution! I reviewed the diff and have a few observations:

  1. Issue reference mismatch

The PR title says "Fixes #1234", but issue #1234 ("Can I add convex hull algorithms, some missing graph algos and hashing?") is a 2020 feature request that was auto-closed as stale in 2021. It has no connection to the changes in this PR — no convex hull, graph algorithm, or hashing code is touched here. Could you confirm the correct issue number this PR is meant to address?
2. Unrelated changes bundled together

This PR mixes two separate concerns:

NQueens.java: replacing System.out.println with Logger.info
StackPostfixNotation.java: refactoring Stack to ArrayDeque/Deque, plus try-with-resources for the Scanner

Per the contributing guidelines, it's usually cleaner to split unrelated changes into separate PRs — this makes review and rollback easier if something needs to be reverted later.
3. Test coverage gap

Codecov flags 4 missing lines in NQueens.java (63.6% patch coverage). It might be worth adding a test case that covers the logger branches (the "no solution found" and "arrangement found" paths) to close that gap.
4. Minor: empty log line

In NQueens.java, LOGGER.info("") (replacing the old blank System.out.println()) doesn't really add value — loggers print structured lines (timestamp/level), so an empty call just creates a noisy blank entry rather than visual spacing. Might be worth removing it.
cc @DenizAltunkapan @alxkm @yanglbme — flagging in case this needs a second look before review, since the issue reference doesn't seem to match the actual changes.

@DenizAltunkapan DenizAltunkapan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with @prashantpiyush1111 — the same points block this for me. Short summary of what to change:

  1. Fix the issue reference. "Fixes #1234" points to an unrelated closed issue. Remove it or use the correct number.
  2. Split the PR. NQueens (logger) and StackPostfixNotation (Deque) are unrelated and should be two PRs.
  3. Remove LOGGER.info(""). An empty log line only adds noise.
  4. Add a test for the NQueens logger branches to cover the 4 missing lines.

Requesting changes until these are addressed.

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.

4 participants