Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 2, 2025

Summary

This PR implements improvements identified in the Issue #50 comprehensive review (docs/PR_REVIEW_ISSUE50.md).

Improvements:

  1. DRY Refactoring - Extract magic constants and eliminate code duplication
  2. Pre-PR Checklist - Add comprehensive quality checklist to DEVELOPMENT.md

Changes

1. Extract Magic Constants in TenantKey

Before:

mkdir($dir, 0700, true);
hash_hmac('sha256', $plaintext, $idxKey, true);

After:

private const KEY_DIRECTORY_PERMISSIONS = 0700;
private const HMAC_ALGORITHM = 'sha256';

mkdir($dir, self::KEY_DIRECTORY_PERMISSIONS, true);
hash_hmac(self::HMAC_ALGORITHM, $plaintext, $idxKey, true);

Benefit: Improves maintainability and makes intent explicit.


2. Use NormalizesPersonFields Trait in RebuildIndexCommand

Before:

$normalized = strtolower(trim($emailPlain));
$normalized = preg_replace('/\D/', '', $phonePlain);

After:

use NormalizesPersonFields;

$normalized = $this->normalizeEmail($emailPlain);
$normalized = $this->normalizePhone($phonePlain);

Benefit: Eliminates code duplication, ensures consistent normalization logic across PersonObserver, PersonRepository, and RebuildIndexCommand.


3. Add Pre-PR Quality Checklist to DEVELOPMENT.md

Added comprehensive checklist covering:

  • ✅ Parallel test execution (catches race conditions)
  • 🔍 Code duplication checks (grep patterns + PHPStan)
  • 🔢 Magic number detection
  • 📝 Conventional commit format
  • 📊 PR size discipline (<600 LOC guideline)
  • 🤖 Copilot review workflow

Quick Pre-PR Command:

ddev . ./vendor/bin/pest --parallel && \
ddev . ./vendor/bin/pint && \
ddev . ./vendor/bin/phpstan analyse && \
echo "✅ Ready for PR!"

Benefit: Documents best practices learned from Issue #50, prevents future follow-up PRs for avoidable issues.

Metrics

  • LOC Changed: 120 lines (+112, -8)
    • TenantKey.php: +8 LOC (2 constants)
    • RebuildIndexCommand.php: +3 LOC (use trait, remove duplication)
    • DEVELOPMENT.md: +101 LOC (Pre-PR checklist)
  • Tests: 114 passing, 332 assertions, 0 failures
  • PHPStan: Level 9, 0 errors
  • Pint: PSR-12, 0 violations

Review Impact

These changes address recommendations from Issue #50 comprehensive review:

  • DRY Adherence: 4/5 → 5/5 (code duplication eliminated)
  • Process Documentation: Enhanced for future PRs
  • Code Quality: Maintainability improved through named constants

Testing

All existing tests pass without modification - refactoring is transparent to behavior.

ddev . ./vendor/bin/pest
# Tests:    114 passed (332 assertions)
# Duration: 9.90s

Checklist

  • Parallel test execution passes
  • No code duplication (grep + PHPStan check)
  • Magic numbers replaced with named constants
  • All quality gates passing (Pint, PHPStan, PEST)
  • Conventional commit format used
  • PR size <600 LOC ✅ (120 LOC)
  • Pre-PR checklist followed (dogfooding our own docs!)

Related

- Extract magic constants in TenantKey (KEY_DIRECTORY_PERMISSIONS, HMAC_ALGORITHM)
- Use NormalizesPersonFields trait in RebuildIndexCommand (eliminate code duplication)
- Add comprehensive Pre-PR Quality Checklist to DEVELOPMENT.md
- Document parallel test execution, code duplication checks, and magic number detection

These improvements address recommendations from Issue #50 review:
- DRY adherence improved from 4/5 to 5/5
- Process documentation enhanced for future PRs
- Consistent normalization logic across commands and observers

Changes:
- app/Models/TenantKey.php: +8 LOC (2 constants)
- app/Console/Commands/RebuildIndexCommand.php: +3 LOC (use trait, remove duplication)
- DEVELOPMENT.md: +101 LOC (Pre-PR checklist)

All 114 tests passing, PHPStan Level 9 clean, Pint compliant.
Copilot AI review requested due to automatic review settings November 2, 2025 01:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors code to eliminate magic numbers and reduce duplication by extracting normalization logic into a trait. The changes improve maintainability by replacing hardcoded values with named constants and consolidating repeated normalization patterns.

  • Extracted email and phone normalization logic into a reusable NormalizesPersonFields trait
  • Replaced magic numbers (0700, 'sha256') with named class constants
  • Updated RebuildIndexCommand to use the trait instead of inline normalization logic

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
app/Models/TenantKey.php Introduced constants for directory permissions (0700) and HMAC algorithm (sha256) to replace magic values
app/Console/Commands/RebuildIndexCommand.php Added NormalizesPersonFields trait usage and replaced inline normalization with trait methods
DEVELOPMENT.md Added comprehensive pre-PR quality checklist with examples of magic numbers, code duplication checks, and quality gate commands

@kevalyq kevalyq merged commit ada3701 into main Nov 2, 2025
21 checks passed
@kevalyq kevalyq deleted the feat/post-issue50-improvements branch November 2, 2025 01:51
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