Refactor: extract renderColoredText to deduplicate text rendering#800
Refactor: extract renderColoredText to deduplicate text rendering#800blade-running-man wants to merge 1 commit intoBlueforcer:mainfrom
Conversation
…ering Extract rainbow/gradient/plain text rendering into a shared DisplayManager method, replacing 4 identical blocks in Apps.cpp and Overlays.cpp. Also fixes a bug in Overlays.cpp where scrolling rainbow text was missing textOffset, causing inconsistent positioning compared to gradient and plain text modes.
|
Robert C. Martin famously said "Functions should do one thing. They should do it well. They should do it only". Different behaviour of a function depending on the argument is a bad design. |
Fair point about SRP, but I'd argue this function does one thing - renders text with the app's color settings. The mode branching is an implementation detail, similar to how printf handles different format specifiers internally. The reason I extracted it: the same 12-line if/else chain was copy-pasted 4 times across Apps.cpp and Overlays.cpp. One of those copies already had a bug — rainbow mode in Overlays.cpp:325 was missing textOffset, so scrolling rainbow text was offset differently from gradient/plain. That's exactly the kind of bug duplication creates. I'm ok to close this if you'd rather keep it inline |
To add some context - the existing codebase already follows this same pattern in several places: indicatorParser has 5 switch blocks branching on indicator, printText branches on bool centered, HSVtext/GradientText both branch on bool clear, drawBarChart/drawLineChart branch on bool withIcon. So this isn't introducing a new pattern, it's consistent with what's already here. Also "duplication is the root of all evil in software design" and these two principles are often in tension. In this case I leaned toward reducing duplication since it already caused an actual bug |
2 participants
Extract rainbow/gradient/plain text rendering into a shared
DisplayManager::renderColoredText()method, replacing 4 identical blocks acrossApps.cppandOverlays.cpp.Bug fix
Scrolling notification text in rainbow mode was missing
textOffset, causing inconsistent positioning compared to gradient and plain text modes (Overlays.cpp:325).Changes
src/DisplayManager.h— new method declarationsrc/DisplayManager.cpp—renderColoredText()implementationsrc/Apps.cpp— replaced 2 duplicated blocks (non-scrolling + scrolling)src/Overlays.cpp— replaced 2 duplicated blocks (non-scrolling + scrolling, includes textOffset fix)