Skip to content

CODE_REVIEW.md

Latest commit

 

History

History
180 lines (118 loc) · 11.5 KB

CODE_REVIEW.md

File metadata and controls

180 lines (118 loc) · 11.5 KB

Code Review: jayci

Author: Jason Stephens Repository: Personal dotfiles / shell configuration framework ~2,000 lines across 10+ files | Shell (Bash/ZSH), Elisp, Vimscript

1. Your Style

You write exploratory, conversational code. Your shell configuration reads less like a systems script and more like a journal -- annotated with section banners, commented-out experiments left in place, and aliases named after yourself. There's a clear aesthetic identity: jayci_, jaylias, jaygit_, JAY_SPACE. You brand your tooling.

Your formatting is consistent and intentional:

  • Heavy use of #=== and #--- banner separators to create visual sections
  • >>> SECTION NAME <<< markers throughout every file
  • Inline comments on nearly every alias explaining what the flags do
  • Attribution comments ("thx dougblack", "from Owen's bbsh", "from Rob MacGowan", "from Brian at Pado", "from the Chief Engineer at Padopolis 2011")

This is a notebook-style of development. You keep old code around as reference. You comment things out rather than delete them. You document not just what the code does but where you learned it and who taught you.

2. Skill Assessment

Level: Early-intermediate shell scripter, with the instincts of someone who has worked professionally in DevOps/SRE-adjacent roles.

Evidence for this assessment:

What you understand well:

  • Environment variable scoping and export -- proper use of export for globals, local vars in functions
  • The sourcing cascade -- you have a clear mental model of .zshrc -> .jayrc -> jayci.cfg -> scripts/*.sh -> configs/*.cfg
  • Aliasing conventions -- you know ls flags deeply and have built a well-organized family of l* aliases
  • Git workflow -- your README demonstrates strong conceptual understanding of branching, rebasing, reverting merges, and git fetch vs git pull
  • Defensive coding -- rm -i, cp -i, mv -i as safety nets, ${JAY_SPACE:?} parameter expansion to fail on unset vars (.jayrc:258)

Where you're between beginner and intermediate:

  • ZSH-specific features -- you migrated from Bash to ZSH and are still finding your footing (e.g., compgen -> print -l ${(ok)functions} transition visible in commit e2c0b0c)
  • Function design -- your functions work but lack input validation, return codes, and error handling
  • Elisp -- your init.el has multiple commented-out attempts at the same feature (hiding DONE items), which suggests trial-and-error against an unfamiliar API

3. Expert Patterns

These are things you're doing that experienced shell developers also do:

Pattern Location Why it's good
${JAY_SPACE:?} parameter expansion .jayrc:258 Fails immediately if variable is unset, preventing silent path errors. This is a defensive technique many intermediate scripters don't know.
Subshell ( ... ) in loop body jayci_utilities.sh:21-37 jayci_update_repos wraps each iteration in ( ), so cd inside the loop doesn't change the parent shell's directory. This is correct and shows awareness of process isolation.
pushd/popd for directory management .jayrc:223-231 src() uses pushd to remember location before jumping. Not just cd, but a stack-based approach.
vcs_info for ZSH prompt .zshrc:19-39 Using ZSH's native vcs_info with precmd hook rather than shelling out to git on every prompt. This is the correct, performant approach.
Local variables in functions jayci_utilities.sh:47-50 local WHITE_ON_RED=... prevents namespace pollution in the calling shell.
Glob sourcing with exclusion .jayrc:259 ls -1 ... | egrep -v "nose|setup" filters out files that shouldn't be sourced. Shows awareness that wildcard sourcing needs guardrails.

4. Novice Patterns & Weaknesses

4a. Parsing ls output (fragile)

.jayrc:259:

for i in $( ls -1 ${JAY_SPACE}/*/*.cfg|egrep -v "nose|setup" ); do

jayci_utilities.sh:21:

for i in $( lsd ) ; do

Parsing ls is a well-known antipattern. If a filename contains spaces, newlines, or special characters, this breaks silently. The idiomatic approach is for i in ${JAY_SPACE}/*/*.cfg; do with a glob directly, or find ... -print0 | while IFS= read -r -d '' file.

4b. Unquoted variable expansions

Throughout .jayrc and utilities:

if [ -f $HOME/.bashrc ]; then      # should be "$HOME"
source $i                           # should be "$i"
cd $i                               # should be "$i"

Unquoted variables break on paths with spaces. Every $variable used in a path context should be "$variable".

4c. Echo for structured output instead of printf

.jayrc:162-212 -- the jay alias is a massive chain of echo statements:

alias jay='echo     ;
echo ;
echo lalias:  \\t list aliases;

This is brittle, hard to maintain, and the \\t escaping is inconsistent across shells. The jayhere.sh heredoc approach (jaylias function) is the better pattern -- you already know this, you just haven't consolidated.

4d. Hardcoded paths

.jayrc:22:

PATH="/Users/jstephens/Library/Python/3.9/bin:$PATH"

This path is specific to one machine and one Python version. It will silently fail or do nothing on any other system. The rest of your config uses $HOME and $USER properly, but this line breaks the pattern.

4e. Dead code accumulation

notes.md is almost entirely commented-out code blocks. jayrc-prompt.cfg is ~50% commented-out PS1 experiments. init.el has 3 different commented-out approaches to hiding DONE items. .jayrc:79-86 preserves deprecated aliases with #nostalgia.

Keeping dead code as documentation is a common beginner habit. It signals uncertainty about git -- specifically, not trusting that git log and git diff can recover old code. You clearly know git well (your README is a git tutorial), but your habits haven't caught up with your knowledge.

4f. Naming inconsistency: echo"" missing space

jayci_utilities.sh:81,84,88:

echo""      # this is a command-not-found error in many shells

This should be echo "". Without the space, some shells try to find a command literally called echo"".

4g. jayci_error used for non-errors

jayci_utilities.sh:9:

jayci_error "Source pipeline verified..."

A function named _error that prints success messages in red is semantically misleading. The name implies something went wrong. If it's general-purpose console output, it should be named accordingly (jayci_banner, jayci_highlight).

4h. No shebang consistency

jayci_utilities.sh and jayhere.sh have #! /bin/zsh shebangs but are designed to be sourced, not executed. A sourced file doesn't use its shebang. These scripts also wouldn't work if actually executed with ./jayhere.sh because they define functions without calling them.

4i. Glob sourcing without error checking

jayci.cfg:23-24:

. ${JAY_SPACE}/jayci/scripts/*.sh
. ${JAY_SPACE}/jayci/configs/*.cfg

If the glob matches nothing (empty directory, wrong path), the shell attempts to source a literal file named *.sh, producing a confusing error. A for loop with a null-glob guard is safer.

5. The Narrative in the Comments

Your comments tell a story of a developer who learned by apprenticeship and accretion.

The Padopolis era (~2011): You reference "the Chief Engineer at Padopolis" and "Brian at Pado" as sources of knowledge. Your ssh -A alias, transfer_files function, and original repo-update loop all trace back here. This is where you learned shell scripting -- not from a book, but from watching experienced engineers and copying their patterns. The jayci_transfer_files function in notes.md:39 is preserved as an artifact from this period, entirely commented out but kept.

The Owen/Rob era: "from Owen's bbsh 20130403" and "from Rob MacGowan" -- you inherited .bash_profile and .amyrisrc configurations from colleagues. Your shell config is a palimpsest: layers of borrowed config from people you worked with, gradually customized.

The Amyris era (~2018-2020): JAMYRIS, repo, repocm, handbook, datadog -- these aliases paint a picture of your daily work. Configuration management, DevOps handbook, Datadog monitoring. The prompt config jayrc-prompt.cfg:41 timestamps this era: "AMYRIS prompt + show repo (18..06-20..04)".

The migration: Your codebase records a Bash-to-ZSH migration. The resh alias dynamically detects which RC file to source. The prompt config has layers of PS1 experiments commented out, leading to the current vcs_info-based ZSH prompt. Commit e2c0b0c explicitly documents: "Replace bash compgen with println for zsh."

The pyenv struggle: Commits 9c8d001 ("Adding all kinds of funkyness for pyenv sake. Heaven help me") and e2c8da6 ("Initiate pyenv at every prompt. Include dependencies.") followed by eece4f0 ("commented out python env setup") tell a classic story: tried to set up pyenv, fought with it, gave up and commented it out. The hardcoded Python 3.9 path at .jayrc:22 is likely the fallback after this battle.

The org-mode experiment: Your most recent commits track an Emacs org-mode setup for task tracking. The init.el shows someone actively wrestling with Elisp -- trying multiple approaches to hide DONE items, none fully working. Commit f906d1e ("More failed attempts to hide DONE by default") is refreshingly honest.

The overall narrative: a practitioner who learns by doing, collects patterns from mentors and colleagues, preserves history almost compulsively, and is currently expanding from shell configuration into editor customization and personal productivity tooling.

6. Options for Next Steps

Option A: Clean house

Strip dead code, remove commented-out experiments, trust git history. Convert the jay echo-chain alias into a heredoc function like jaylias. Quote all variable expansions. Replace ls-parsing loops with globs. This would reduce total LOC by ~30% and make the config more portable and robust.

Option B: Make it portable

Replace the hardcoded /Users/jstephens/ path with dynamic detection. Add a bootstrap script that creates symlinks automatically (your README describes manual ln -s steps). Add a Makefile or install.sh so you could set up a new machine in one command. This is the standard "dotfiles repo" evolution.

Option C: Add real CI to "jayci"

The name promises CI/CD but the repo is a dotfiles manager. You could add ShellCheck linting (catches every quoting issue identified above), a GitHub Action that runs ShellCheck on push, and a test script that sources the config in a Docker container to verify it loads cleanly. This would make the name honest and teach you CI/CD patterns hands-on.

Option D: Consolidate the help system

You have three overlapping help mechanisms: the jay alias (echo chain), jaylias (heredoc), and jayhere.sh (more heredocs). Merge them into a single jayhelp command with subcommands: jayhelp aliases, jayhelp git-tags, jayhelp git-branch. The commented-out jayhelp_pick function in notes.md:85 shows you already had this idea.

Option E: Finish the org-mode setup

Your init.el has three commented-out attempts at hiding DONE items. The one that's active (my-org-hide-done using org-after-todo-state-change-hook) only hides tasks at the moment they change to DONE -- it doesn't hide them on file open. The org-mode-hook version in comments (init.el:61-68) was closer to what you want but had a bug. This is a solvable problem with a focused debugging session.