Engineering Fundamentals Review
"Code is read more than it is written. Write for the reader, not the machine."
When to Apply
Activate this skill when reviewing:
- Any code changes
- Function and variable naming
- Code organization and structure
- General refactoring decisions
Review Checklist
Naming
Function Design
Code Organization
Comments & Documentation
Common Mistakes (Anti-Patterns)
1. Magic Numbers
❌ if (status === 2) { ... }
setTimeout(callback, 86400000);
✅ const STATUS = { ACTIVE: 2, INACTIVE: 1 };
if (status === STATUS.ACTIVE) { ... }
const * 60 * 60 * 1000;
setTimeout(callback, ONE_DAY_MS);
2. Unclear Naming
❌ const d = new Date();
const temp = getUser();
const flag = true;
✅ const createdAt = new Date();
const currentUser = getUser();
const isAuthenticated = true;
3. God Functions
❌ function processOrder(order) {
// 200 lines: validate, calculate, save, email, log...
}
✅ function processOrder(order) {
validateOrder(order);
const total = calculateTotal(order);
await saveOrder(order, total);
await sendConfirmationEmail(order);
logOrderProcessed(order);
}
4. Deep Nesting
❌ function check(user) {
if (user) {
if (user.active) {
if (user.role === 'admin') {
return true;
}
}
}
return false;
}
✅ function check(user) {
if (!user) return false;
if (!user.active) return false;
if (user.role !== 'admin') return false;
return true;
}
5. Premature Abstraction
❌ // Used once, but has 10 configuration options
createFlexibleReusableButton({ ... });
✅ // Just make the button
<button className="primary">Submit</button>
// Abstract when you need it 3+ times
SOLID Principles Quick Check
| Principle |
Question |
Red Flag |
| Single Responsibility |
"Does this class/function do one thing?" |
Class with 10+ methods |
| Open/Closed |
"Can I extend without modifying?" |
Switch statements for types |
| Liskov Substitution |
"Can I swap implementations?" |
Overriding methods that break contracts |
| Interface Segregation |
"Are interfaces focused?" |
Clients forced to depend on unused methods |
| Dependency Inversion |
"Do high-level modules depend on abstractions?" |
Direct instantiation of dependencies |
Socratic Questions
Ask the junior these questions instead of giving answers:
- Naming: "Would a new developer understand this name without context?"
- Function Size: "Can you describe what this function does in one sentence?"
- Duplication: "I see this pattern in three places. What happens if it needs to change?"
- Abstraction: "How many times is this abstraction actually used?"
- Readability: "If you came back to this code in 6 months, would you understand it?"
Naming Conventions
| Type |
Convention |
Example |
| Variables |
camelCase |
userName, isActive |
| Constants |
UPPER_SNAKE_CASE |
MAX_RETRIES, API_URL |
| Functions |
camelCase + verb |
getUser(), handleSubmit() |
| Classes |
PascalCase |
UserService, AuthProvider |
| Files (components) |
PascalCase |
UserProfile.tsx |
| Files (utilities) |
camelCase |
formatDate.ts |
Standards Reference
See detailed patterns in:
/standards/global/naming-conventions.md
Red Flags to Call Out
| Flag |
Question to Ask |
| Single letter variables |
"What does d represent?" |
| Functions > 30 lines |
"Can we break this into smaller functions?" |
| > 3 levels of nesting |
"Can we use early returns?" |
| Copy-pasted code |
"If this logic changes, how many places need updating?" |
| Commented-out code |
"Is this needed? Can we delete it?" |
| TODO without tracking |
"Is there a ticket for this?" |
| Magic strings/numbers |
"Should this be a named constant?" |
1---2name: engineering-fundamentals3description: TRIGGERS: "review my code", "is this clean?", "code review", "refactor this", "code quality", "refactoring", "cleaning up", "organizing", "structuring code", "naming this", "splitting function", "extract method", "too long", "messy code", "better way", naming conventions, DRY, SOLID, clean code, refactoring, code organization, best practices. USE WHEN: Junior is refactoring, organizing code, or asking about code quality and naming. PROVIDES: Naming standards, function size limits, DRY enforcement, SOLID principles, organization. PROACTIVE: Triggers when junior mentions refactoring or improving code structure.4---5
6# Engineering Fundamentals Review
7
8> "Code is read more than it is written. Write for the reader, not the machine."
9
10## When to Apply
11
12Activate this skill when reviewing:
13- Any code changes
14- Function and variable naming
15- Code organization and structure
16- General refactoring decisions
17
18---
19
20## Review Checklist
21
22### Naming
23
24- [ ] **Descriptive**: Can you understand the variable without context?
25- [ ] **No abbreviations**: Are names spelled out? (`user` not `usr`)
26- [ ] **No generic names**: No `data`, `temp`, `info`, `stuff`?
27- [ ] **Boolean prefix**: Do booleans start with `is`, `has`, `can`, `should`?
28- [ ] **Function verbs**: Do functions start with action verbs?
29
30### Function Design
31
32- [ ] **Single responsibility**: Does each function do ONE thing?
33- [ ] **Size limit**: Are functions under 20-30 lines?
34- [ ] **Parameter count**: Are there fewer than 4 parameters?
35- [ ] **No side effects**: Are pure functions actually pure?
36- [ ] **Early returns**: Are guard clauses used instead of deep nesting?
37
38### Code Organization
39
40- [ ] **DRY**: Is duplicated code extracted into functions?
41- [ ] **But not too DRY**: Are abstractions justified (rule of three)?
42- [ ] **Cohesion**: Are related things grouped together?
43- [ ] **Separation**: Are unrelated things separated?
44
45### Comments & Documentation
46
47- [ ] **Why, not what**: Do comments explain reasoning, not obvious code?
48- [ ] **No commented-out code**: Is dead code deleted, not commented?
49- [ ] **JSDoc on public APIs**: Are exported functions documented?
50
51---
52
53## Common Mistakes (Anti-Patterns)
54
55### 1. Magic Numbers
56```
57❌ if (status === 2) { ... }
58 setTimeout(callback, 86400000);
59
60✅ const STATUS = { ACTIVE: 2, INACTIVE: 1 };
61 if (status === STATUS.ACTIVE) { ... }
62
63 const ONE_DAY_MS = 24 * 60 * 60 * 1000;
64 setTimeout(callback, ONE_DAY_MS);
65```
66
67### 2. Unclear Naming
68```
69❌ const d = new Date();
70 const temp = getUser();
71 const flag = true;
72
73✅ const createdAt = new Date();
74 const currentUser = getUser();
75 const isAuthenticated = true;
76```
77
78### 3. God Functions
79```
80❌ function processOrder(order) {
81 // 200 lines: validate, calculate, save, email, log...
82 }
83
84✅ function processOrder(order) {
85 validateOrder(order);
86 const total = calculateTotal(order);
87 await saveOrder(order, total);
88 await sendConfirmationEmail(order);
89 logOrderProcessed(order);
90 }
91```
92
93### 4. Deep Nesting
94```
95❌ function check(user) {
96 if (user) {
97 if (user.active) {
98 if (user.role === 'admin') {
99 return true;
100 }
101 }
102 }
103 return false;
104 }
105
106✅ function check(user) {
107 if (!user) return false;
108 if (!user.active) return false;
109 if (user.role !== 'admin') return false;
110 return true;
111 }
112```
113
114### 5. Premature Abstraction
115```
116❌ // Used once, but has 10 configuration options
117 createFlexibleReusableButton({ ... });
118
119✅ // Just make the button
120 <button className="primary">Submit</button>
121
122 // Abstract when you need it 3+ times
123```
124
125---
126
127## SOLID Principles Quick Check
128
129| Principle | Question | Red Flag |
130|-----------|----------|----------|
131| **S**ingle Responsibility | "Does this class/function do one thing?" | Class with 10+ methods |
132| **O**pen/Closed | "Can I extend without modifying?" | Switch statements for types |
133| **L**iskov Substitution | "Can I swap implementations?" | Overriding methods that break contracts |
134| **I**nterface Segregation | "Are interfaces focused?" | Clients forced to depend on unused methods |
135| **D**ependency Inversion | "Do high-level modules depend on abstractions?" | Direct instantiation of dependencies |
136
137---
138
139## Socratic Questions
140
141Ask the junior these questions instead of giving answers:
142
1431. **Naming**: "Would a new developer understand this name without context?"
1442. **Function Size**: "Can you describe what this function does in one sentence?"
1453. **Duplication**: "I see this pattern in three places. What happens if it needs to change?"
1464. **Abstraction**: "How many times is this abstraction actually used?"
1475. **Readability**: "If you came back to this code in 6 months, would you understand it?"
148
149---
150
151## Naming Conventions
152
153| Type | Convention | Example |
154|------|------------|---------|
155| Variables | camelCase | `userName`, `isActive` |
156| Constants | UPPER_SNAKE_CASE | `MAX_RETRIES`, `API_URL` |
157| Functions | camelCase + verb | `getUser()`, `handleSubmit()` |
158| Classes | PascalCase | `UserService`, `AuthProvider` |
159| Files (components) | PascalCase | `UserProfile.tsx` |
160| Files (utilities) | camelCase | `formatDate.ts` |
161
162---
163
164## Standards Reference
165
166See detailed patterns in:
167- `/standards/global/naming-conventions.md`
168
169---
170
171## Red Flags to Call Out
172
173| Flag | Question to Ask |
174|------|-----------------|
175| Single letter variables | "What does `d` represent?" |
176| Functions > 30 lines | "Can we break this into smaller functions?" |
177| > 3 levels of nesting | "Can we use early returns?" |
178| Copy-pasted code | "If this logic changes, how many places need updating?" |
179| Commented-out code | "Is this needed? Can we delete it?" |
180| TODO without tracking | "Is there a ticket for this?" |
181| Magic strings/numbers | "Should this be a named constant?" |
majiayu000