From c8d001b713fadaea4f13ba80fdb0c0651b4da95e Mon Sep 17 00:00:00 2001 From: cschantz Date: Tue, 2 Dec 2025 17:14:19 -0500 Subject: [PATCH] Add color code bug prevention: cecho helper + coding guidelines PREVENTION STRATEGY for "echo without -e" bug: 1. NEW HELPER FUNCTION - cecho() - Added to lib/common-functions.sh (lines 100-115) - Wrapper around echo -e for colored output - Clear documentation with examples - Usage: cecho "${BOLD}Text${NC}" instead of echo -e 2. COMPREHENSIVE CODING GUIDELINES - Created CODING_GUIDELINES.md - Documents the echo -e color bug with examples - Prevention rules and quick reference table - Search command to find potential issues - Pre-commit checklist for developers - Performance guidelines (subprocess elimination) 3. DOCUMENTATION INCLUDES: - Why the bug happens (escape sequences not interpreted) - How to identify it (grep pattern) - How to fix it (echo -e or cecho) - When to use each approach - Historical context (commit 7053b3b) BENEFITS: - Future developers can reference guidelines - cecho() provides cleaner, safer API - Search pattern helps audit existing code - Reduces recurring "This happens a lot" issues USER FEEDBACK ADDRESSED: User: "This happens a lot with you. is there a way for us to avoid this in the future?" Answer: Yes - cecho() helper + guidelines document + search pattern --- CODING_GUIDELINES.md | 172 ++++++++++++++++++++++++++++++++++++++++ lib/common-functions.sh | 17 ++++ 2 files changed, 189 insertions(+) create mode 100644 CODING_GUIDELINES.md diff --git a/CODING_GUIDELINES.md b/CODING_GUIDELINES.md new file mode 100644 index 0000000..93954a6 --- /dev/null +++ b/CODING_GUIDELINES.md @@ -0,0 +1,172 @@ +# Server Toolkit Coding Guidelines + +## Color Code Usage - CRITICAL + +### The Problem +Using `echo` without the `-e` flag causes ANSI escape sequences to display literally instead of being interpreted: + +```bash +# WRONG - Shows literal: \033[1m1\033[0m +echo " ${BOLD}1${NC} - Menu option" + +# RIGHT - Shows: 1 (bold and colored) +echo -e " ${BOLD}1${NC} - Menu option" +``` + +### Prevention Rules + +**Rule 1: Always use `echo -e` when string contains color variables** +```bash +# WRONG: +echo " ${BOLD}Option${NC}" +echo "Status: ${GREEN}OK${NC}" + +# RIGHT: +echo -e " ${BOLD}Option${NC}" +echo -e "Status: ${GREEN}OK${NC}" +``` + +**Rule 2: Use helper functions from common-functions.sh** +```bash +# Source the library +source /root/server-toolkit/lib/common-functions.sh + +# Use built-in print functions (already use echo -e) +print_success "Operation completed" +print_error "Something went wrong" +print_info "Information message" + +# For custom colored output, use cecho() +cecho "Custom ${RED}colored${NC} text" +``` + +**Rule 3: No -e flag needed when NOT using quotes** +```bash +# This is OK (no quotes around the whole thing) +echo ${BOLD}Title${NC} + +# But this needs -e (quotes around everything) +echo -e "${BOLD}Title${NC}" +``` + +### Quick Reference + +| Scenario | Command | +|----------|---------| +| Menu option with colors | `echo -e " ${BOLD}1${NC} - Option"` | +| Success message | `print_success "Done"` or `echo -e "${GREEN}✓${NC} Done"` | +| Error message | `print_error "Failed"` or `echo -e "${RED}✗${NC} Failed"` | +| Custom colored text | `cecho "Text ${RED}red${NC} normal"` | +| Plain text (no colors) | `echo "Plain text"` (no -e needed) | + +### How to Find and Fix + +**Search for potential issues:** +```bash +# Find echo statements with color variables that might be missing -e +grep -n 'echo ".*\${\(BOLD\|.*_COLOR\|NC\|RED\|GREEN\|YELLOW\|BLUE\)' your_script.sh +``` + +**Common patterns to fix:** +```bash +# BEFORE: +echo " ${BOLD}1${NC} - Enable Feature" +echo "Status: ${GREEN}Active${NC}" +echo "${RED}[ERROR]${NC} Failed" + +# AFTER: +echo -e " ${BOLD}1${NC} - Enable Feature" +echo -e "Status: ${GREEN}Active${NC}" +echo -e "${RED}[ERROR]${NC} Failed" + +# OR use helper: +cecho " ${BOLD}1${NC} - Enable Feature" +cecho "Status: ${GREEN}Active${NC}" +print_error "Failed" +``` + +### Why This Matters + +**Impact of the bug:** +- Menus show escape codes instead of colors: `\033[1m1\033[0m` +- Makes interface look broken and unprofessional +- Reduces readability and user experience +- Hard to debug because it "looks right" in the code + +**Historical issues:** +- Security hardening menu (fixed: commit 7053b3b) +- Various other menus and status displays +- User feedback: "This happens a lot with you" + +### Pre-commit Checklist + +Before committing code with color output: +- [ ] All `echo` statements with `${COLOR}` variables use `-e` flag +- [ ] Or use `cecho()` helper function instead +- [ ] Or use `print_*()` functions from common-functions.sh +- [ ] Test output in terminal to verify colors render correctly +- [ ] Run: `bash -n script.sh` to check syntax +- [ ] Run: `shellcheck script.sh` if available + +## Performance Guidelines + +### Avoid Subprocesses in Loops + +**Rule: Use bash built-ins instead of spawning processes** + +```bash +# WRONG - Spawns subprocess (slow) +url_lower=$(echo "$url" | tr '[:upper:]' '[:lower:]') +url_lower=$(echo "$url" | tr 'A-Z' 'a-z') + +# RIGHT - Bash built-in (fast) +url_lower="${url,,}" + +# WRONG - Multiple subprocesses +hostname=$(hostname) +for item in $list; do + check_hostname "$hostname" # Calls hostname 1000s of times +done + +# RIGHT - Cache the result +CACHED_HOSTNAME="${HOSTNAME:-$(hostname)}" +for item in $list; do + check_hostname "$CACHED_HOSTNAME" +done +``` + +**Impact:** On high-traffic servers (1000+ req/sec), subprocess elimination prevents tens of thousands of unnecessary forks per second. + +### Performance Quick Reference + +| Operation | Slow (subprocess) | Fast (built-in) | +|-----------|-------------------|-----------------| +| Lowercase | `$(echo "$var" \| tr '[:upper:]' '[:lower:]')` | `${var,,}` | +| Uppercase | `$(echo "$var" \| tr '[:lower:]' '[:upper:]')` | `${var^^}` | +| Substring | `$(echo "$var" \| cut -c1-5)` | `${var:0:5}` | +| Replace | `$(echo "$var" \| sed 's/old/new/')` | `${var//old/new}` | +| Length | `$(echo "$var" \| wc -c)` | `${#var}` | + +## Additional Guidelines + +### Error Handling +- Always check return codes for critical operations +- Use `set -euo pipefail` for strict error handling in new scripts +- Provide meaningful error messages with context + +### Function Documentation +- Add comment block above complex functions +- Document parameters, return values, and side effects +- Include usage examples for non-obvious functions + +### Variable Naming +- Use descriptive names: `user_count` not `uc` +- Constants in UPPER_CASE: `MAX_RETRIES=3` +- Local variables in lower_case: `local temp_file="/tmp/data"` +- Global exports in UPPER_CASE: `export LOG_DIR="/var/log"` + +### Testing +- Test with both light and dark terminal backgrounds +- Test with `TOOLKIT_NO_COLOR=1` for monochrome output +- Verify output doesn't overflow on 24-line terminals (standard) +- Test with realistic data volumes (not just 1-2 entries) diff --git a/lib/common-functions.sh b/lib/common-functions.sh index 7f2952e..439d50e 100755 --- a/lib/common-functions.sh +++ b/lib/common-functions.sh @@ -97,6 +97,23 @@ print_header() { echo -e "${CYAN}${BOLD}$1${NC}" } +############################################################################# +# Color Echo Helper - ALWAYS use this when printing colored text +# +# PROBLEM: Using 'echo' without -e flag doesn't interpret escape sequences +# BAD: echo " ${BOLD}1${NC} - Menu option" → Shows: \033[1m1\033[0m +# GOOD: cecho " ${BOLD}1${NC} - Menu option" → Shows: 1 (bold) +# +# USAGE: +# cecho "Normal text with ${RED}colored${NC} parts" +# cecho "${BOLD}Bold text${NC}" +# +# WHY: Prevents common bug where color codes show as literal text +############################################################################# +cecho() { + echo -e "$@" +} + # Wait for user input press_enter() { echo ""