Implement Text to ASCII Art Generator with Standard and Block Fonts (#139)#144
Implement Text to ASCII Art Generator with Standard and Block Fonts (#139)#144alistanikzay wants to merge 7 commits intomainfrom
Conversation
…Test - Test works
WalkthroughIntroduces a new ASCII art generation feature. Adds AsciiFont interface, two implementations (StandardFont, BlockFont), and AsciiArtGenerator orchestrating input validation and delegation to a font. Includes tests covering successful rendering for both fonts and validation for null, empty, overly long, and unsupported inputs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Gen as AsciiArtGenerator
participant Font as AsciiFont (Standard/Block)
Client->>Gen: generate(text)
alt text is null/blank or length > 50
Gen-->>Client: throw IllegalArgumentException
else valid input
Gen->>Font: render(uppercase(text))
Font-->>Gen: asciiArt (3 lines)
Gen-->>Client: asciiArt
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
♻️ Duplicate comments (1)
src/main/java/org/fungover/breeze/ascii2/StandardFont.java (1)
49-64: Duplicate rendering logic.This
render()method duplicates the logic fromBlockFont.render(). See the recommendation inBlockFont.java(lines 38-53) to extract common rendering logic to reduce code duplication.
🧹 Nitpick comments (3)
src/main/java/org/fungover/breeze/ascii2/AsciiArtGenerator.java (1)
11-15: Consider centralizing validation.Both
StandardFontandBlockFontduplicate the null/blank check from line 12. SinceAsciiArtGeneratoralready validates inputs, the font implementations could assume valid input.Remove duplicate validation from font implementations:
In
StandardFont.javaandBlockFont.java, remove lines that duplicate this check:- if (text == null || text.isBlank()) throw new IllegalArgumentException("Text cannot be null or empty");src/main/java/org/fungover/breeze/ascii2/BlockFont.java (2)
38-53: Extract common rendering logic to reduce duplication.The
render()method is nearly identical toStandardFont.render(). Consider extracting the common row-iteration and character-lookup logic to a shared abstract base class or helper method.Example approach:
public abstract class AbstractAsciiFont implements AsciiFont { protected abstract Map<Character, String[]> getFontMap(); protected abstract int getHeight(); @Override public String render(String text) { text = text.toUpperCase(); StringBuilder sb = new StringBuilder(); Map<Character, String[]> fontMap = getFontMap(); for (int row = 0; row < getHeight(); row++) { for (char c : text.toCharArray()) { if (!fontMap.containsKey(c)) { throw new IllegalArgumentException("Unsupported character: " + c); } sb.append(fontMap.get(c)[row]).append(" "); } sb.append("\n"); } return sb.toString(); } }
42-42: Consider making spacing and height configurable.The hardcoded
height = 3and spacing" "reduce flexibility for future font variations.Consider deriving height from font data:
- int height = 3; + int height = fontMap.values().iterator().next().length;And extracting spacing as a constant:
+ private static final String CHAR_SPACING = " "; // ... - sb.append(fontMap.get(c)[row]).append(" "); + sb.append(fontMap.get(c)[row]).append(CHAR_SPACING);Also applies to: 48-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(1 hunks)src/main/java/org/fungover/breeze/ascii2/AsciiArtGenerator.java(1 hunks)src/main/java/org/fungover/breeze/ascii2/AsciiFont.java(1 hunks)src/main/java/org/fungover/breeze/ascii2/BlockFont.java(1 hunks)src/main/java/org/fungover/breeze/ascii2/StandardFont.java(1 hunks)src/test/java/org/fungover/breeze/ascii2/AsciiArtGeneratorTest.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Java CI with Maven
pom.xml
[warning] 33-33: dependencies.dependency.version for junit:junit:jar is either LATEST or RELEASE (both deprecated). It is highly recommended to fix these problems to ensure build stability.
🔇 Additional comments (4)
src/main/java/org/fungover/breeze/ascii2/AsciiFont.java (1)
3-7: LGTM!The interface provides a clean abstraction for different font implementations with a straightforward contract.
src/main/java/org/fungover/breeze/ascii2/AsciiArtGenerator.java (1)
7-9: LGTM!Constructor dependency injection is clean and supports the pluggable font design.
src/main/java/org/fungover/breeze/ascii2/BlockFont.java (1)
14-35: LGTM!Font character definitions are clear and the initialization logic is straightforward.
src/test/java/org/fungover/breeze/ascii2/AsciiArtGeneratorTest.java (1)
6-40: LGTM!Test coverage appropriately validates both font implementations and all input validation scenarios specified in the requirements.
MudasirStanikzay
left a comment
MudasirStanikzay
left a comment
There was a problem hiding this comment.
Clean and well-structured test class.
Tests are clear, use proper naming, and cover different fonts and exception handling effectively.
The code follows good JUnit 5 practices and is easy to read and maintain.
MudasirStanikzay
left a comment
MudasirStanikzay
left a comment
There was a problem hiding this comment.
Clean and readable implementation.
The code is well-structured, easy to follow, and provides clear validation for invalid or unsupported input.
Font data is organized neatly in a map, and the rendering logic is straightforward and efficient.
… based by coderabbit
MudasirStanikzay
left a comment
MudasirStanikzay
left a comment
There was a problem hiding this comment.
Well-written and consistent implementation.
The code is clean, logically structured, and maintains clarity between data setup and rendering.
Exception handling is appropriate, and the use of maps for font storage makes it easy to extend and maintain.
… based by CodeRabbit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/org/fungover/breeze/ascii2/StandardFont.java (1)
48-64: Consider removing trailing spaces after the last character on each row.The rendering logic is correct and handles validation properly. However, line 59 appends
" "(two spaces) after every character, including the last one on each row. This creates trailing whitespace before the newline.Apply this diff to conditionally add spacing:
for (int row = 0; row < height; row++) { + int charIndex = 0; for (char c : text.toCharArray()) { if (!fontMap.containsKey(c)) { throw new IllegalArgumentException("Unsupported character: " + c); } - sb.append(fontMap.get(c)[row]).append(" "); + sb.append(fontMap.get(c)[row]); + if (charIndex < text.length() - 1) { + sb.append(" "); + } + charIndex++; } sb.append("\n"); }Alternatively, for a cleaner approach using iteration with index:
for (int row = 0; row < height; row++) { - for (char c : text.toCharArray()) { + char[] chars = text.toCharArray(); + for (int i = 0; i < chars.length; i++) { + char c = chars[i]; if (!fontMap.containsKey(c)) { throw new IllegalArgumentException("Unsupported character: " + c); } - sb.append(fontMap.get(c)[row]).append(" "); + sb.append(fontMap.get(c)[row]); + if (i < chars.length - 1) { + sb.append(" "); + } } sb.append("\n"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/fungover/breeze/ascii2/StandardFont.java(1 hunks)
🔇 Additional comments (3)
src/main/java/org/fungover/breeze/ascii2/StandardFont.java (3)
1-12: LGTM! Clean initialization and structure.The class structure is well-organized with proper encapsulation. The
fontMapis effectively immutable after construction, making the class thread-safe for rendering operations.
14-46: Font definitions look good.The ASCII art representations are clear and consistent. Note that 'A' uses 5 characters width while others use 4 characters width, which is acceptable for ASCII art (characters commonly have different widths like 'M' vs 'I'). The 3-row height is consistent across all characters.
66-69: LGTM!Simple and correct implementation returning the font name.
2 participants
Summary This pull request implements a Text to ASCII Art Generator that supports two font styles: Standard and Block.
Changes
Added AsciiArtGenerator class to handle text generation with validation (null, empty, max length 50).
Added AsciiFont interface for font abstraction.
Implemented StandardFont and BlockFont classes with simple ASCII mappings.
Added unit tests in AsciiArtGeneratorTest to verify:
Text rendering for both fonts
Exception handling for invalid inputs
Testing All tests pass using JUnit 5.
Verified correct ASCII output for "HELLO" in both fonts.
Related Issue
Closes #139
Summary by CodeRabbit