<![CDATA[TalkativeTurtles - Code Review & Feedback]]>

<![CDATA[TalkativeTurtles - Code Review & Feedback]]> https://talkativeturtles.club/ Wed, 05 Aug 2026 13:53:24 +0000 MyBB <![CDATA[Refactoring legacy code without breaking things - a practical approach]]> https://talkativeturtles.club/showthread.php?tid=105 Mon, 22 Jun 2026 12:45:29 +0000 Zero Two]]> https://talkativeturtles.club/showthread.php?tid=105
Step 1: Understand before touching
Read it. Run it. Add logging to understand the execution path. Don't start changing things until you understand what the code does, even if not why.

Step 2: Add tests before refactoring
If there are no tests, write characterisation tests first. These aren't tests of what the code should do - they document what it currently does, including any bugs. Now you have a safety net.

Step 3: Refactor in small steps
The goal is to keep the code working at every step. Extract a function. Rename a variable. Simplify a condition. Run tests after each change. A PR that changes logic AND refactors structure simultaneously is impossible to review and risky to merge.

The Strangler Fig pattern
For large systems: build new behaviour alongside old code, gradually redirect traffic to the new path, then delete the old code once the new is proven. Never do a big bang rewrite.

Common traps:

Refactoring and fixing bugs simultaneously - you won't know which change introduced a regression
Gold-plating - refactoring is not the time to add new features or redesign the architecture
Scope creep - "while I'm here" changes multiply risk. Separate PRs for separate concerns.
Not communicating to the team - if someone else is working in the same area, coordinate

When to not refactor:
If the code works, is rarely touched, and has no tests - leave it alone. The cost of adding tests and refactoring isn't always worth it for stable code that doesn't change.]]>
Step 1: Understand before touching
Read it. Run it. Add logging to understand the execution path. Don't start changing things until you understand what the code does, even if not why.

Step 2: Add tests before refactoring
If there are no tests, write characterisation tests first. These aren't tests of what the code should do - they document what it currently does, including any bugs. Now you have a safety net.

Step 3: Refactor in small steps
The goal is to keep the code working at every step. Extract a function. Rename a variable. Simplify a condition. Run tests after each change. A PR that changes logic AND refactors structure simultaneously is impossible to review and risky to merge.

The Strangler Fig pattern
For large systems: build new behaviour alongside old code, gradually redirect traffic to the new path, then delete the old code once the new is proven. Never do a big bang rewrite.

Common traps:

Refactoring and fixing bugs simultaneously - you won't know which change introduced a regression
Gold-plating - refactoring is not the time to add new features or redesign the architecture
Scope creep - "while I'm here" changes multiply risk. Separate PRs for separate concerns.
Not communicating to the team - if someone else is working in the same area, coordinate

When to not refactor:
If the code works, is rarely touched, and has no tests - leave it alone. The cost of adding tests and refactoring isn't always worth it for stable code that doesn't change.]]> <![CDATA[[Guide] What to look for when doing code review]]> https://talkativeturtles.club/showthread.php?tid=104 Mon, 22 Jun 2026 12:36:32 +0000 Zero Two]]> https://talkativeturtles.club/showthread.php?tid=104
Things worth flagging:

Correctness - will this actually work? Are there edge cases (empty inputs, nulls, concurrency, off-by-one errors) that aren't handled?

Security - is user input validated and sanitised? Are there SQL injection, XSS, or path traversal risks? Are secrets handled correctly (not logged, not in responses)?

Logic errors in error handling - does the code handle failure gracefully? Will an unhandled exception take down the service or just fail this request?

Naming and clarity - if you have to read a function twice to understand what it does, the name or structure needs work. This is worth raising because future maintainers (including the author) will hit the same confusion.

Missing tests - if this adds new behaviour, are there tests? If it fixes a bug, is there a regression test?

Things not worth bikeshedding:

Formatting, if there's a linter enforcing it
Minor style preferences that don't affect readability
"I would have done it differently" without a concrete reason

How to give useful feedback:

Be specific - "this could be clearer" is less useful than "the variable name d doesn't tell me what this stores"
Explain why, not just what
Distinguish blockers from suggestions - prefix with Blocker: / Suggestion: / Nit:
Praise good work - it's not all criticism

Timebox it - a focused 30-minute review is more valuable than two hours of exhaustive comments that overwhelm the author.]]>
Things worth flagging:

Correctness - will this actually work? Are there edge cases (empty inputs, nulls, concurrency, off-by-one errors) that aren't handled?

Security - is user input validated and sanitised? Are there SQL injection, XSS, or path traversal risks? Are secrets handled correctly (not logged, not in responses)?

Logic errors in error handling - does the code handle failure gracefully? Will an unhandled exception take down the service or just fail this request?

Naming and clarity - if you have to read a function twice to understand what it does, the name or structure needs work. This is worth raising because future maintainers (including the author) will hit the same confusion.

Missing tests - if this adds new behaviour, are there tests? If it fixes a bug, is there a regression test?

Things not worth bikeshedding:

Formatting, if there's a linter enforcing it
Minor style preferences that don't affect readability
"I would have done it differently" without a concrete reason

How to give useful feedback:

Be specific - "this could be clearer" is less useful than "the variable name d doesn't tell me what this stores"
Explain why, not just what
Distinguish blockers from suggestions - prefix with Blocker: / Suggestion: / Nit:
Praise good work - it's not all criticism

Timebox it - a focused 30-minute review is more valuable than two hours of exhaustive comments that overwhelm the author.]]> <![CDATA[[Template] Code Review Submission Format]]> https://talkativeturtles.club/showthread.php?tid=42 Sun, 21 Jun 2026 09:42:20 +0000 Zero Two]]> https://talkativeturtles.club/showthread.php?tid=42

Code:
Language & version:

Purpose of this code:

Is this code working? (if not, use Help & Troubleshooting instead)

Specific feedback wanted:

(e.g. "Is the error handling idiomatic?", "Any performance concerns?",

"Would you structure this differently?", "Is this secure?")

Constraints / context:

(e.g. "this is a hot path called 10k times/sec", "Python 3.9 max", "no external dependencies")

--- Code below ---

Reminder:

Use [noparse][code][/noparse] tags around all code
Code must already run correctly - this forum is for review, not debugging
Only submit code you have the right to share publicly

]]>

Code:
Language & version:

Purpose of this code:

Is this code working? (if not, use Help & Troubleshooting instead)

Specific feedback wanted:

(e.g. "Is the error handling idiomatic?", "Any performance concerns?",

"Would you structure this differently?", "Is this secure?")

Constraints / context:

(e.g. "this is a hot path called 10k times/sec", "Python 3.9 max", "no external dependencies")

--- Code below ---

Reminder:

Use [noparse][code][/noparse] tags around all code
Code must already run correctly - this forum is for review, not debugging
Only submit code you have the right to share publicly

]]> <![CDATA[[Rules] Code Review & Feedback — Forum Rules]]> https://talkativeturtles.club/showthread.php?tid=12 Sun, 21 Jun 2026 09:34:07 +0000 Zero Two]]> https://talkativeturtles.club/showthread.php?tid=12 Code Review & Feedback is for getting constructive critique on working code.

Posting Rules:

Code must already work (or be close to working). This is not a debugging forum - use Help & Troubleshooting or Programming & Development for broken code.
Always use [code] tags. Unformatted code will not be reviewed.
State what feedback you want. "Any thoughts?" gets vague answers. "Is my memory management correct?" gets useful ones.
Include context: language, runtime version, purpose of the code, any constraints you are working within.
Do not post code you do not have the right to share (employer code, client code under NDA).

When reviewing:

Be specific and constructive. Explain why something should change, not just that it should.
Acknowledge what is done well, not just what needs work.

]]> Code Review & Feedback is for getting constructive critique on working code.

Posting Rules:

Code must already work (or be close to working). This is not a debugging forum - use Help & Troubleshooting or Programming & Development for broken code.
Always use [code] tags. Unformatted code will not be reviewed.
State what feedback you want. "Any thoughts?" gets vague answers. "Is my memory management correct?" gets useful ones.
Include context: language, runtime version, purpose of the code, any constraints you are working within.
Do not post code you do not have the right to share (employer code, client code under NDA).

When reviewing:

Be specific and constructive. Explain why something should change, not just that it should.
Acknowledge what is done well, not just what needs work.

]]>