openharmonyinsight/comprehensive-code-review

Comprehensive Code Review for ACE Engine

Quick Start

Perform comprehensive code review analysis directly with AI:

"Review the code in frameworks/core/components_ng for memory management issues"

"Analyze this pull request focusing on security vulnerabilities"

"Check path/to/code.cpp for ACE Engine architecture compliance"

Review Workflow

Phase 1: Code Analysis

AI will analyze code across all relevant dimensions based on:

1. Reading the code files - Understanding implementation details
2. Applying reference standards - Using criteria from references/ directory
3. Checking against ACE Engine patterns - Validating project-specific conventions

Phase 2: Dimension-Based Review

Review code across 19+ dimensions. See DIMENSIONS.md for complete coverage.

High-Priority Dimensions:

• Security (CRITICAL) - Input validation, command injection, buffer overflows
• Memory (CRITICAL) - Smart pointers, leaks, ownership semantics
• Threading (HIGH) - Data races, deadlocks, callback safety
• Stability (HIGH) - Error handling, boundary conditions

Medium-Priority Dimensions:

• Performance - Algorithm complexity, optimization opportunities
• SOLID Principles - Single responsibility, Open/Closed, etc.
• Code Smells - Long methods, large classes, duplicate code
• Testability - Dependency injection, decoupling

Phase 3: Report Generation

AI generates comprehensive review report including:

• Executive Summary - Issue counts by severity
• Critical Issues (🔴) - Must fix before merge
• High Priority Issues (🟠) - Should fix before merge
• Medium Priority Issues (🟡) - Fix soon
• Low Priority Issues (🟢) - Nice to have
• Dimension Breakdown - Issues grouped by category
• File-by-File Details - Specific line numbers and code snippets
• Recommendations - Prioritized action items with before/after examples
• Review Checklist - Pre-merge verification

Dimension Reference

Quick reference to all dimensions:

| Dimension | Focus | Severity Levels | Reference | |-----------|-------|-----------------|-----------| | Stability | Error handling, boundaries | 🔴🟠🟡🟢 | STABILITY.md | | Performance | Algorithms, optimization | 🔴🟠🟡🟢 | DIMENSIONS.md | | Threading | Concurrency, synchronization | 🔴🟠🟡 | DIMENSIONS.md | | Security | Input validation, vulnerabilities | 🔴🟠🟡 | SECURITY.md | | Memory | Smart pointers, leaks | 🔴🟠🟡 | MEMORY.md | | Modern C++ | C++11/14/17/20 features | 🟠🟡 | DIMENSIONS.md | | Effective C++ | RAII, Rule of Five | 🟠🟡 | DIMENSIONS.md | | Code Smells | 22 types of smells | 🟠🟡 | CODE_SMELLS.md | | SOLID | Design principles | 🟠🟡 | SOLID.md | | Design Patterns | Pattern usage | 🟡🟢 | DIMENSIONS.md | | Robustness | Fault tolerance | 🟠🟡 | DIMENSIONS.md | | Testability | Dependency injection | 🟠🟡 | DIMENSIONS.md | | Maintainability | Code complexity | 🟡🟢 | DIMENSIONS.md | | Observability | Logging, monitoring | 🟡🟢 | DIMENSIONS.md | | API Design | Interface quality | 🟠🟡 | DIMENSIONS.md | | Technical Debt | TODO/FIXME tracking | 🟡🟢 | DIMENSIONS.md | | Backward Compatibility | API stability | 🟠🟡 | DIMENSIONS.md | | Architecture | ACE Engine compliance | 🔴🟠🟡 | ACE_ENGINE_SPECIFIC.md |

ACE Engine Specific Checks

Architecture Validation

Four-Layer Architecture:

Frontend Bridge Layer
    ↓
Component Framework Layer (Pattern/Model/Property)
    ↓
Layout/Render Layer
    ↓
Platform Adapter Layer (OHOS/Preview)

Check:

• No violations of layer boundaries
• Proper use of Pattern/Model/Property separation
• No direct platform calls from framework layer

Component Structure

components_ng/pattern/<component>/
├── *_pattern.h/cpp         # Business logic & lifecycle
├── *_model.h/cpp           # Data model interface
├── *_layout_property.h/cpp # Layout properties
├── *_paint_property.h/cpp  # Render properties
└── *_event_hub.h/cpp       # Event handling

RefPtr Usage

// ✅ Creation
auto node = AceType::MakeRefPtr<FrameNode>();

// ✅ Type-safe casting
auto pattern = AceType::DynamicCast<MenuPattern>(node->GetPattern());
if (!pattern) {
    LOGE("Failed to get pattern");
    return false;
}

// ✅ Breaking cycles with WeakPtr
class Child {
    WeakPtr<Parent> parent_;  // Use weak, not strong
};

// ✅ Safe callbacks
auto weak = AceType::WeakClaim(this);
PostTask([weak]() {
    auto pattern = AceType::DynamicCast<MenuPattern>(weak.Upgrade());
    if (pattern) pattern->Update();
});

Naming Conventions

class MenuPattern {           // PascalCase
public:
    void OnModifyDone();      // PascalCase
    int GetWidth() const;     // Get prefix

private:
    int width_;               // snake_case_ with trailing underscore
    std::string component_id_;  // Abbreviations lowercase
};

constexpr int MAX_MENU_ITEMS = 100;  // UPPER_CASE

Severity Levels

🔴 CRITICAL - Must fix before merge

• Security vulnerabilities (command injection, buffer overflow)
• Memory leaks in long-running processes
• Use-after-free, double-free
• Data races
• Crashes / undefined behavior
• Architecture violations

🟠 HIGH - Should fix before merge

• Performance degradation
• API violations
• SOLID principle violations
• Major code smells (Large Class, Shotgun Surgery)
• Improper RefPtr usage
• Unsafe callback captures

🟡 MEDIUM - Fix soon

• Minor performance issues
• Code smells (Long Method, Duplicate Code)
• Style violations
• Missing error handling in non-critical paths

🟢 LOW - Nice to have

• Missing comments
• Minor style issues
• Optimization opportunities
• Code documentation improvements

Common Issue Patterns

| Pattern | Dimension | Severity | Detection | |---------|-----------|----------|-----------| | Raw pointer instead of RefPtr | Memory | HIGH | new T() without RefPtr | | Command injection | Security | CRITICAL | system(user_input) | | Buffer overflow | Security | CRITICAL | strcpy(), sprintf() | | Unsafe this capture | Threading | HIGH | [this] in PostTask | | Long method (>50 lines) | Code Smell | MEDIUM | Function length | | Large class (>500 lines) | Code Smell | HIGH | Class size | | Missing null check | Stability | HIGH | Pointer use without validation | | static_cast instead of DynamicCast | Memory | MEDIUM | static_cast<T*> |

Usage Examples

Review a Pull Request

"Review the changes in this PR with focus on memory management and threading safety"

AI will:

• Analyze all changed files
• Check memory management (RefPtr, WeakPtr, raw pointers)
• Verify threading safety (callback captures, PostTask usage)
• Report issues with line numbers and severity

Security Audit

"Perform a security audit of path/to/code, focusing on input validation and potential vulnerabilities"

AI will check for:

• Command injection risks (system() calls)
• Buffer overflow vulnerabilities (strcpy, sprintf)
• Unvalidated user input
• Unsafe type casts

Memory Leak Detection

"Analyze frameworks/core/components for potential memory leaks and improper RefPtr usage"

AI will identify:

• Raw new/delete without smart pointers
• Circular references without WeakPtr
• Missing null checks after DynamicCast
• Improper ownership semantics

Architecture Compliance Check

"Verify that components_ng/menu follows the four-layer architecture and Pattern/Model/Property separation"

AI will validate:

• Four-layer architecture compliance
• Pattern/Model/Property separation
• No layer boundary violations
• Component lifecycle methods (OnModifyDone, etc.)

Integration Examples

Pre-commit Hook

For pre-commit hooks, consider using traditional static analysis tools:

#!/bin/bash
# .git/hooks/pre-commit

CHANGED_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.(cpp|cc|cxx|h|hpp|ts|tsx)$')

if [ -n "$CHANGED_FILES" ]; then
    echo "Running basic static analysis..."
    # Use clang-tidy, cppcheck, eslint, etc.
    for file in $CHANGED_FILES; do
        if [[ $file == *.cpp || $file == *.h ]]; then
            clang-tidy $file --checks='*' || true
        elif [[ $file == *.ts ]]; then
            eslint $file || true
        fi
    done
fi

Manual Review Workflow

For comprehensive AI-powered review:

1. Identify code to review (PR, specific files, directory)
2. Request review with specific focus areas
3. AI analyzes code using reference standards
4. Review results with severity-tagged issues
5. Address critical and high-priority issues
6. Re-request review for verification

References

Detailed documentation for all dimensions in references/:

• DIMENSIONS.md - Quick reference for all 19+ dimensions
• STABILITY.md - Error handling, boundary conditions
• MEMORY.md - Smart pointers, leaks, ownership
• SECURITY.md - Input validation, vulnerabilities
• CODE_SMELLS.md - 22 types of code smells
• SOLID.md - Five design principles
• ACE_ENGINE_SPECIFIC.md - Project-specific rules

Assets

• report_template.md - Customizable report template for AI-generated reviews

Review Checklist

Before merging code:

Code Quality:

• [ ] No CRITICAL issues
• [ ] HIGH priority issues addressed or documented
• [ ] No compiler warnings
• [ ] Static analysis clean

Architecture:

• [ ] Four-layer architecture respected
• [ ] Proper Pattern/Model/Property separation
• [ ] No layer boundary violations

Memory:

• [ ] No memory leaks
• [ ] RefPtr used correctly
• [ ] WeakPtr used to break cycles
• [ ] Callbacks use WeakClaim

Security:

• [ ] All input validated
• [ ] No command injection
• [ ] No buffer overflows
• [ ] Sensitive data protected

Testing:

• [ ] Unit tests added
• [ ] Edge cases covered
• [ ] Error scenarios tested