Skip to content

Update function-decomposition.qmd#290

Open
d-morrison wants to merge 2 commits into
mainfrom
d-morrison-patch-1
Open

Update function-decomposition.qmd#290
d-morrison wants to merge 2 commits into
mainfrom
d-morrison-patch-1

Conversation

@d-morrison

Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings June 3, 2026 20:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates the “When to decompose a function” guidance in the lab manual to use more general wording, improving clarity while keeping the intent of the checklist the same.

Changes:

  • Reworded one decomposition criterion from “top-level function” to “current function” for broader applicability.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1-2-g6ad689f

QR code for preview link

🚀 View preview at
https://UCD-SERG.github.io/lab-manual/pr-preview/pr-290/

Built to branch gh-pages at 2026-06-17 17:27 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@d-morrison

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 25s —— View job


Code Review

  • Gather context
  • Read changed file and diff
  • Provide review feedback

The change is a single-line wording improvement in coding-practices/function-decomposition.qmd:23:

Before: The top-level function is hard to scan because implementation details hide the main idea
After: The current function is hard to scan because implementation details hide the main idea

Assessment: Looks good.

The original phrase "top-level function" was ambiguous — it could be read as referring only to the outermost function in a call hierarchy, whereas the intent is to apply to any function being considered for decomposition. Changing it to "current function" makes the checklist item applicable at any level of nesting, which is more accurate and consistent with the rest of the checklist (which uses second-person "you" constructions throughout).

The change is minimal, correct, and improves clarity without altering the meaning or introducing any inconsistencies elsewhere in the document.

@d-morrison

Copy link
Copy Markdown
Member Author

Claude Code CLI (local session) is working on this — paws off until I'm done.

@d-morrison

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 37s —— View job


Code Review

  • Gather context
  • Read changed file and diff
  • Provide review feedback

File reviewed: coding-practices/function-decomposition.qmd

Change (line 23):

- The top-level function is hard to scan because implementation details hide the main idea
+ The current function is hard to scan because implementation details hide the main idea

Assessment: Looks good. ✓

The wording change is a clear improvement:

  • "top-level function" is ambiguous — it implies the checklist item only applies to the outermost function in a hierarchy, which is too narrow.
  • "current function" is more accurate — it makes the criterion applicable at any level of nesting, which matches how decomposition is actually practiced (you decompose whichever function you're working on, regardless of depth).

The rest of the document is well-structured. The checklist items use consistent second-person framing ("you are repeating…", "you need comments…", "you want to test…"), and "current function" fits naturally into that voice without requiring any other edits.

No issues found. This is ready to merge.


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.

2 participants