Vibeship-spawner-skills code-quality

id: code-quality

install

source · Clone the upstream repo

git clone https://github.com/vibeforge1111/vibeship-spawner-skills

manifest: mind/code-quality/skill.yaml

The test: Can you describe what the function does without using "and"?

BAD: Does multiple things

function processOrder(order) { // Validates order if (!order.items.length) throw new Error('Empty order');

// Calculates total
const total = order.items.reduce((sum, item) => sum + item.price, 0);

// Saves to database
await db.orders.insert({ ...order, total });

// Sends confirmation email
await sendEmail(order.user.email, 'Order confirmed', { order });

// Updates inventory
for (const item of order.items) {
  await db.inventory.decrement(item.productId, item.quantity);
}

}

GOOD: Each function has one job

function validateOrder(order) { if (!order.items.length) throw new Error('Empty order'); }

function calculateOrderTotal(items) { return items.reduce((sum, item) => sum + item.price, 0); }

async function processOrder(order) { validateOrder(order); const total = calculateOrderTotal(order.items); const savedOrder = await saveOrder({ ...order, total }); // Queue these as background jobs await queue.add('send-confirmation', { orderId: savedOrder.id }); await queue.add('update-inventory', { items: order.items }); return savedOrder; }

Note: Don't go too far. 3-5 line functions everywhere is also bad.

Balance single responsibility with reasonable locality.

name: Pragmatic SOLID description: Apply SOLID principles with judgment, not dogma when: Designing classes or modules example: |

SOLID is a guide, not a religion. Here's when to apply each:

Single Responsibility Principle (SRP)

WHEN TO APPLY: Class doing unrelated things

WHEN TO IGNORE: Would create class explosion for trivial logic

BAD: Too strict SRP

class UserNameValidator { } class UserEmailValidator { } class UserPasswordValidator { } class UserValidatorOrchestrator { }

GOOD: Pragmatic SRP

class UserValidator { validateName(name) { } validateEmail(email) { } validatePassword(password) { } }

Open/Closed Principle (OCP)

WHEN TO APPLY: You've already extended the same code 3+ times

WHEN TO IGNORE: Speculative future needs ("might need to extend")

YAGNI violation: Plugin architecture for 2 payment providers

Pragmatic: Switch statement for 2 providers, refactor if adding 3rd

Liskov Substitution Principle (LSP)

WHEN TO APPLY: You're using inheritance

BETTER ADVICE: Prefer composition over inheritance, then LSP rarely matters

Interface Segregation Principle (ISP)

WHEN TO APPLY: Classes implementing methods they don't use

WHEN TO IGNORE: Would create interface explosion

Dependency Inversion Principle (DIP)

WHEN TO APPLY: Testing is hard due to concrete dependencies

WHEN TO IGNORE: For stable, unlikely-to-change dependencies

BAD: Abstracting everything

interface ILogger { } interface IDateProvider { } interface IStringFormatter { }

GOOD: Abstract unstable dependencies

interface PaymentGateway { } // This might change // Just use console.log, Date, String directly
name: The Rule of Three description: Wait for three occurrences before abstracting when: Tempted to create an abstraction for duplicated code example: |

The pattern: Duplicate once is OK. Twice is a smell. Three times, abstract.

First occurrence: Just write it

function createAdminUser(data) { const hashedPassword = await hashPassword(data.password); return db.users.insert({ ...data, password: hashedPassword, role: 'admin' }); }

Second occurrence: Note the duplication, but don't abstract yet

function createRegularUser(data) { const hashedPassword = await hashPassword(data.password); return db.users.insert({ ...data, password: hashedPassword, role: 'user' }); }

Third occurrence: Now you understand the pattern, abstract

function createUser(data, role) { const hashedPassword = await hashPassword(data.password); return db.users.insert({ ...data, password: hashedPassword, role }); }

WHY WAIT?

- First two cases might diverge in unexpected ways

- The "right" abstraction isn't clear from 2 examples

- Wrong abstraction is worse than duplication
name: Comments That Add Value description: Comment the why, not the what when: Code behavior isn't obvious from context example: |

BAD: Comments that explain what (the code already says this)

// Increment counter by 1 counter++;

// Loop through users for (const user of users) { }

BAD: Comments that become lies

// Returns user or null function getUser(id) { return users.get(id) || { guest: true }; // Actually returns guest object }

GOOD: Comments that explain why

// Using 86400 instead of 606024 for performance (hot path) const SECONDS_PER_DAY = 86400;

// Skip validation for internal service calls (already validated upstream) if (request.source === 'internal') { return processRequest(request); }

// Retry 3 times because Stripe occasionally returns 500 on first attempt // See: https://github.com/stripe/stripe-node/issues/123 const result = await retry(3, () => stripe.charges.create(charge));

GOOD: Comments that warn

// WARNING: This function is called from a cron job AND the API. // Any changes must work for both contexts.

// HACK: Working around React 18 batching bug. Remove after upgrade. // See: JIRA-1234
name: Guard Clauses description: Handle edge cases early, keep happy path unindented when: Functions with multiple conditions or error cases example: |

BAD: Deeply nested conditions

function processPayment(order, user) { if (order) { if (user) { if (user.hasPaymentMethod) { if (order.total > 0) { // Finally, the actual logic return chargeUser(user, order.total); } else { throw new Error('Invalid order total'); } } else { throw new Error('No payment method'); } } else { throw new Error('User required'); } } else { throw new Error('Order required'); } }

GOOD: Guard clauses at the top

function processPayment(order, user) { if (!order) throw new Error('Order required'); if (!user) throw new Error('User required'); if (!user.hasPaymentMethod) throw new Error('No payment method'); if (order.total <= 0) throw new Error('Invalid order total');
```
// Happy path is clear and unindented
return chargeUser(user, order.total);
```
}

anti_patterns:

name: Premature Abstraction description: Creating abstractions before understanding the pattern why: | You see two similar things and immediately create an abstraction. But you don't yet understand how they're similar or different. The abstraction becomes a straitjacket that makes future changes harder, not easier. instead: Apply the Rule of Three. Wait until you've seen the pattern three times before abstracting.
name: Enterprise FizzBuzz description: Simple problems solved with excessive architecture why: | Interface for everything. Factory for every class. Strategy pattern for two options. The code is "extensible" for changes that will never come, while simple changes require touching 12 files. instead: Start with the simplest thing that works. Add patterns when complexity demands them, not before.
name: Clever Code description: Code that shows off rather than communicates why: | One-liners that require 5 minutes to understand. Clever bitwise operations. Regex that does 10 things. You feel smart writing it, everyone else suffers reading it. Including future you. instead: Write boring code. If you're proud of how clever it is, it's probably too clever.
name: Cargo Cult Patterns description: Using patterns because "that's how it's done" why: | Repository pattern for a 3-table app. CQRS for a blog. Event sourcing for a todo list. Patterns exist to solve specific problems. Using them without the problem adds complexity without benefit. instead: Understand WHY a pattern exists. Apply it when you have the problem it solves.
name: Comment Rot description: Comments that no longer match the code why: | Comments aren't checked by the compiler. When code changes, comments often don't. Misleading comments are worse than no comments - they actively deceive the reader. instead: Keep comments minimal and focused on why. Update or delete when code changes.
name: Boolean Parameters description: Functions with true/false parameters that hide meaning why: | What does
```
createUser(data, true, false)
```
do? You have to read the function signature to know. Boolean parameters are code that requires context from elsewhere to understand. instead: 'Use named parameters or options objects.
```
createUser(data, { sendEmail: true, skipValidation: false })
```
'

handoffs:

trigger: need to refactor existing code to: refactoring-guide context: User needs refactoring strategy, not quality principles
trigger: testing strategy or test design to: test-strategist context: User needs test design, not code quality
trigger: debugging an issue to: debugging-master context: User needs to find a bug, not improve code
trigger: architectural decisions to: system-designer context: User needs system design, not code-level quality
trigger: managing technical debt to: tech-debt-manager context: User needs debt strategy, not quality principles

Vibeship-spawner-skills code-quality

BAD: Clever one-liner

GOOD: Readable steps

ALSO GOOD: Clear single chain with line breaks

The rule: Would a new team member understand this in 10 seconds?

If not, simplify.

VARIABLES: Reveal what it is and why it exists

BAD: Generic or abbreviated

GOOD: Descriptive and contextual

FUNCTIONS: Verb + noun, describe the action and result

BAD: Vague or misleading

GOOD: Specific and honest

BOOLEANS: Read like a question

BAD: Ambiguous

GOOD: Reads naturally

The test: Can you describe what the function does without using "and"?

BAD: Does multiple things

GOOD: Each function has one job

Note: Don't go too far. 3-5 line functions everywhere is also bad.

Balance single responsibility with reasonable locality.

SOLID is a guide, not a religion. Here's when to apply each:

Single Responsibility Principle (SRP)

WHEN TO APPLY: Class doing unrelated things

WHEN TO IGNORE: Would create class explosion for trivial logic

BAD: Too strict SRP

GOOD: Pragmatic SRP

Open/Closed Principle (OCP)

WHEN TO APPLY: You've already extended the same code 3+ times

WHEN TO IGNORE: Speculative future needs ("might need to extend")

YAGNI violation: Plugin architecture for 2 payment providers

Pragmatic: Switch statement for 2 providers, refactor if adding 3rd

Liskov Substitution Principle (LSP)

WHEN TO APPLY: You're using inheritance

BETTER ADVICE: Prefer composition over inheritance, then LSP rarely matters

Interface Segregation Principle (ISP)

WHEN TO APPLY: Classes implementing methods they don't use

WHEN TO IGNORE: Would create interface explosion

Dependency Inversion Principle (DIP)

WHEN TO APPLY: Testing is hard due to concrete dependencies

WHEN TO IGNORE: For stable, unlikely-to-change dependencies

BAD: Abstracting everything

GOOD: Abstract unstable dependencies

The pattern: Duplicate once is OK. Twice is a smell. Three times, abstract.

First occurrence: Just write it

Second occurrence: Note the duplication, but don't abstract yet

Third occurrence: Now you understand the pattern, abstract

WHY WAIT?

- First two cases might diverge in unexpected ways

- The "right" abstraction isn't clear from 2 examples

- Wrong abstraction is worse than duplication

BAD: Comments that explain what (the code already says this)

BAD: Comments that become lies

GOOD: Comments that explain why

GOOD: Comments that warn

BAD: Deeply nested conditions

GOOD: Guard clauses at the top