Comment Style Guide

Code Comment Style Guidelines

Philosophy

Write code that doesn't need comments. When comments are needed, make them count.

🚨 CRITICAL RULES

Rule #1: No Redundant Comments

FORBIDDEN: Comments that restate what the code obviously does

// ❌ WRONG - Comment restates the code
// Convert pageNumber/pageSize to offset/limit
if (results.pageNumber && results.pageSize) {
  params.offset = (results.pageNumber - 1) * results.pageSize;
  params.limit = Math.min(Math.max(results.pageSize, 1), 1000);
}

// ✅ CORRECT - No comment needed, code is self-explanatory
if (results.pageNumber && results.pageSize) {
  params.offset = (results.pageNumber - 1) * results.pageSize;
  params.limit = Math.min(Math.max(results.pageSize, 1), 1000);
}

// ❌ WRONG - Obvious from context
// Apply mappers and set pagination info from response structure
results.items = response.data.data.map(toUser);
results.count = response.data.totalCount || 0;

// ✅ CORRECT - Code speaks for itself
results.items = response.data.data.map(toUser);
results.count = response.data.totalCount || 0;

Rule #2: No Structural Comments in Tests

FORBIDDEN: Comments that describe test structure instead of business logic

// ❌ WRONG
// Validate structure
const groups = groupsResult.items;
expect(groups).to.be.an('array');

// If groups exist, validate the first one
if (groups.length > 0) {
  const firstGroup = groups[0];

  // Validate common group fields that should exist
  if (firstGroup.id) {
    // ID should be a string
    expect(firstGroup.id).to.be.a('string');
  }
}

// ✅ CORRECT - Let the test structure be self-documenting
const groups = groupsResult.items;
expect(groups).to.be.an('array');

if (groups.length > 0) {
  const firstGroup = groups[0];

  if (firstGroup.id) {
    expect(firstGroup.id).to.be.a('string');
  }
}

Rule #3: Concise JSDoc

Keep JSDoc focused and minimal. Avoid redundant examples.

// ❌ WRONG - Excessive examples
/**
 * Normalizes null to undefined for optional properties
 *
 * This helper ensures consistent handling of optional values by converting
 * null to undefined, while preserving all other values including falsy ones
 * like 0, false, and empty strings.
 *
 * @param value - The value to normalize
 * @returns undefined if input is null or undefined, otherwise returns the value as-is
 *
 * @example
 * // Replaces manual normalization
 * // Before:
 * phoneNumber: raw.phoneNumber ?? undefined,
 * avatarUrl: raw.avatarUrl ?? undefined,
 *
 * // After:
 * phoneNumber: optional(raw.phoneNumber),
 * avatarUrl: optional(raw.avatarUrl),
 *
 * @example
 * // Preserves falsy values (0, false, "")
 * optional(0)        // returns 0 (not undefined)
 * optional(false)    // returns false (not undefined)
 * optional("")       // returns "" (not undefined)
 * optional(null)     // returns undefined
 * optional(undefined) // returns undefined
 */
export function optional<T>(value: T | null | undefined): T | undefined {
  return (value ?? undefined) as T | undefined;
}

// ✅ CORRECT - Concise and focused
/**
 * Normalizes null to undefined for optional properties
 *
 * Converts null to undefined while preserving other falsy values (0, false, "")
 *
 * @param value - The value to normalize
 * @returns undefined if input is null or undefined, otherwise returns the value as-is
 */
export function optional<T>(value: T | null | undefined): T | undefined {
  return (value ?? undefined) as T | undefined;
}

Rule #4: No Function Name Restating

FORBIDDEN: JSDoc that only restates the function name

// ❌ WRONG - Adds no information
/**
 * Mock authenticated request with proper headers
 */
export function mockAuthenticatedRequest(baseUrl: string, token: string) {
  // ...
}

/**
 * Clean up all nock mocks
 */
export function cleanNock(): void {
  nock.cleanAll();
}

// ✅ CORRECT - Function names are self-documenting for simple functions
export function mockAuthenticatedRequest(baseUrl: string, token: string) {
  // ...
}

export function cleanNock(): void {
  nock.cleanAll();
}

When Comments ARE Valuable

1. Business Logic Explanation

// ✅ GOOD - Explains WHY, not WHAT
// API returns 400 for invalid credentials but 404 for missing users
// We normalize both to NotConnectedError for consistent handling
if (error.status === 400 || error.status === 404) {
  throw new NotConnectedError();
}

2. Non-Obvious Behavior

// ✅ GOOD - Warns about API quirk
// API returns user data in response.data.data for single gets
// but directly in response.data for list operations
const rawData = response.data.data || response.data;

3. TODOs and Future Work

// ✅ GOOD - Tracks technical debt
// TODO: Move to @zerobias-org/util-hub-module-utils once stabilized
export function mapWith<T>(mapper: (raw: any) => T, value: any): T | undefined {
  // ...
}

4. Complex Algorithm Explanation

// ✅ GOOD - Explains non-trivial logic
// Calculate weighted priority: active bugs (3x) + planned features (1x)
const priority = (bugCount * 3) + featureCount;

Producer-Specific Guidelines

Pagination Pattern Comments

NEVER comment standard pagination conversion:

// ❌ WRONG
// Convert pageNumber/pageSize to offset/limit
if (results.pageNumber && results.pageSize) {
  params.offset = (results.pageNumber - 1) * results.pageSize;
  params.limit = Math.min(Math.max(results.pageSize, 1), 1000);
}

// ✅ CORRECT - Standard pattern, no comment needed
if (results.pageNumber && results.pageSize) {
  params.offset = (results.pageNumber - 1) * results.pageSize;
  params.limit = Math.min(Math.max(results.pageSize, 1), 1000);
}

Parameter Handling

NEVER comment obvious parameter handling:

// ❌ WRONG
// Add optional filter parameter
if (filter) {
  params.filter = filter;
}

// Add optional options parameter
if (options) {
  params.options = options;
}

// ✅ CORRECT
if (filter) {
  params.filter = filter;
}

if (options) {
  params.options = options;
}

Test-Specific Guidelines

Assertion Comments

NEVER comment assertions that match their code:

// ❌ WRONG
// Required fields
expect(entry).to.have.property('id');
expect(entry).to.have.property('name');

// ID should be a string
expect(entry.id).to.be.a('string');

// ✅ CORRECT
expect(entry).to.have.property('id');
expect(entry).to.have.property('name');
expect(entry.id).to.be.a('string');

Setup Comments

Only comment non-obvious test setup:

// ❌ WRONG - Obvious from describe block and test name
describe('Group Operations', () => {
  it('should list groups with pagination', async () => {
    // First get a list to find valid IDs
    const result = await groupApi.list(orgId, 1, 10);
    // ...
  });
});

// ✅ CORRECT - Comment explains special case
describe('Group Operations', () => {
  it('should handle groups with special characters in names', async () => {
    // Unicode group names require special URL encoding on this endpoint
    const result = await groupApi.get(orgId, '测试组');
    // ...
  });
});

Summary

Write Comments That:

• ✅ Explain WHY, not WHAT
• ✅ Document non-obvious behavior
• ✅ Warn about API quirks
• ✅ Track technical debt (TODOs)

Avoid Comments That:

• ❌ Restate the code
• ❌ Describe obvious structure
• ❌ Repeat function names
• ❌ Explain self-documenting code