## Coding Standards **Version:** 1.0 **Last Updated:** 2025-11-05 **Status:** Draft for Review ### Purpose This document serves as the source of truth for coding standards across all teams. It aims to: - Reduce code review friction by establishing objective standards - Speed up delivery by minimizing style-based discussions - Ensure consistency across teams and projects - Provide clear guidance for both authors and reviewers This is a living document. All teams are encouraged to propose updates through pull requests. --- ### 1. Code Formatting #### Ruby **Standard:** We use [StandardRB](https://github.com/standardrb/standard) for all Ruby code formatting. - **No configuration changes allowed.** StandardRB is intentionally zero-config. - Run `standardrb --fix` before committing. - CI will fail if StandardRB violations exist. - **Code reviews should not discuss formatting covered by StandardRB.** ##### Additional Ruby Style Guidelines Beyond StandardRB automation, follow these conventions from [Shopify's Ruby Style Guide](https://ruby-style-guide.shopify.dev): - **Prefer explicit returns** in public methods for clarity - **Use Ruby 3+ syntax** (e.g., endless methods for simple one-liners, pattern matching where it improves readability) - **Avoid clever code.** Optimize for readability over brevity - **Use keyword arguments** for methods with more than 2 parameters - **Prefer early returns** over deeply nested conditionals ```ruby # Good def process_payment(amount:, currency:, customer:) return failure_result("Invalid amount") if amount <= 0 return failure_result("Unknown currency") unless VALID_CURRENCIES.include?(currency) charge_customer(amount, currency, customer) end # Avoid def process_payment(amount, currency, customer) if amount > 0 if VALID_CURRENCIES.include?(currency) charge_customer(amount, currency, customer) else failure_result("Unknown currency") end else failure_result("Invalid amount") end end ``` #### JavaScript/TypeScript **Standard:** We use [Prettier](https://prettier.io/) for all JavaScript/TypeScript formatting. - Configuration is defined in `.prettierrc` - Run `yarn lint --fix` before committing - CI will fail if Prettier violations exist - **Code reviews should not discuss formatting covered by Prettier** --- ### 2. Ruby & Rails Standards #### Models - **Keep models focused.** Extract concerns for shared behavior, service objects for complex operations - **Use scopes** for reusable queries, not class methods - **Validations belong in models** unless they require external dependencies - **Use database constraints** (NOT NULL, foreign keys) alongside ActiveRecord validations ```ruby # Good class Product < ApplicationRecord validates :name, presence: true validates :price, numericality: { greater_than: 0 } scope :published, -> { where(published: true) } scope :by_category, ->(category) { where(category: category) } end # Avoid - Use scopes instead class Product < ApplicationRecord def self.published where(published: true) end end ``` #### Controllers - **Keep controllers thin.** Business logic belongs in service objects/interactors - **One happy path per action.** Use early returns or redirects for error cases - **Use strong parameters** - never permit all attributes - **Limit to RESTful actions when possible.** Non-RESTful actions should be exceptional and well-named ```ruby # Good class ProductsController < ApplicationController def create result = Products::Create.call(product_params: product_params, site: Current.site) if result.success? redirect_to product_path(result.product), notice: "Product created" else render :new, status: :unprocessable_entity end end end ``` #### Service Objects - **Use Interactors** for multi-step business logic - **Name services as verbs:** `Products::Create`, `Payments::Process`, `Reports::Generate` - **Keep services single-purpose.** If a service does multiple unrelated things, split it - **Always return a result.** Use `context.fail!` for failures in Interactors ```ruby class Products::Create include Interactor def call context.fail!(error: "Invalid site") unless context.site.present? context.product = context.site.products.create!(context.product_params) NotificationService.notify_team(context.product) end end ``` #### Database - **Never bypass multi-tenancy scoping.** Always scope by `Current.site` or equivalent - **Use transactions** for operations that modify multiple records - **Add indexes** for foreign keys and frequently queried columns - **Follow [strong_migrations](https://github.com/ankane/strong_migrations) guidelines** - CI will enforce this --- ### 3. Frontend Standards #### ViewComponents Follow [ViewComponent Best Practices](https://viewcomponent.org/best_practices.html): - **Use ViewComponents for reusable UI elements** - not one-off partials - **Avoid logic in templates** - move it to the component class - **Use slots** for flexible component composition - **Create Lookbook previews** for all components - **Test components independently** from controllers ```ruby # Good - Logic in component class class AlertComponent < ApplicationComponent def initialize(message:, type: :info) @message = message @type = type end def css_classes base_classes = "alert" "#{base_classes} alert-#{@type}" end end # Avoid - Logic in template # In template: class="alert <%= type == :error ? 'alert-error' : 'alert-info' %>" ``` #### Stimulus Controllers - **Keep controllers small and focused** - one behavior per controller - **Use data attributes** for configuration, not hardcoded values - **Name targets descriptively:** `submitButtonTarget` not `btnTarget` - **Disconnect cleanup:** Always clean up event listeners, intervals, etc. in `disconnect()` ```javascript // Good export default class extends Controller { static targets = ["submitButton", "errorMessage"]; async submit(event) { event.preventDefault(); this.submitButtonTarget.disabled = true; try { await this.performSubmit(); } finally { this.submitButtonTarget.disabled = false; } } } ``` #### React Components - **Use functional components** with hooks (no class components for new code) - **Prefer named exports** over default exports for better IDE support - **Extract custom hooks** for reusable stateful logic - **Keep components focused** - if it's doing multiple unrelated things, split it ```javascript // Good export function ProductCard({ product, onAddToCart }) { const { isLoading, addToCart } = useCart(); return ( <div className="product-card"> <h3>{product.name}</h3> <button onClick={() => addToCart(product)} disabled={isLoading}> Add to Cart </button> </div> ); } ``` #### Hotwire/Turbo - **Use Turbo Frames** for independent page sections that update separately - **Use Turbo Streams** for multi-element updates from server responses - **Avoid mixing Turbo with full-page React apps** - pick the right tool for each feature - **Test Turbo interactions** in system tests, not controller tests --- ### 4. Testing Standards #### General Principles - **Tests are documentation.** They should clearly show what the code does - **Test behavior, not implementation.** Tests should survive refactoring - **Test one thing per test.** If you're using "and" in your test name, split it - **Avoid brittle tests.** Don't test framework behavior, don't assert on every attribute #### RSpec (Ruby) **Structure:** ```ruby RSpec.describe ProductsController do describe "POST #create" do context "with valid parameters" do it "creates a new product" do expect { post :create, params: { product: valid_attributes } }.to change(Product, :count).by(1) end it "redirects to the product page" do post :create, params: { product: valid_attributes } expect(response).to redirect_to(product_path(Product.last)) end end context "with invalid parameters" do it "does not create a product" do expect { post :create, params: { product: invalid_attributes } }.not_to change(Product, :count) end end end end ``` **Guidelines:** - **Use FactoryBot** for test data, not fixtures - **Use `let` for reusable test data**, `let!` when you need it created immediately - **Use descriptive factory traits** instead of overriding multiple attributes - **System tests for user workflows**, unit tests for business logic - **Mock external services** - never make real API calls in tests #### Jest (JavaScript) ```javascript describe("ProductCard", () => { it("renders product name", () => { const product = { name: "Test Product", price: 29.99 }; render(<ProductCard product={product} />); expect(screen.getByText("Test Product")).toBeInTheDocument(); }); it("calls onAddToCart when button is clicked", () => { const handleAddToCart = jest.fn(); const product = { name: "Test Product", price: 29.99 }; render(<ProductCard product={product} onAddToCart={handleAddToCart} />); fireEvent.click(screen.getByRole("button", { name: /add to cart/i })); expect(handleAddToCart).toHaveBeenCalledWith(product); }); }); ``` **Guidelines:** - **Use Testing Library** queries (getByRole, getByText) over test IDs where possible - **Test user-visible behavior**, not internal state - **Mock API calls** with MSW or Jest mocks - **Avoid snapshot tests** for anything that changes frequently #### Test Coverage - **Aim for high coverage on business logic** (services, models) - **Don't obsess over 100% coverage** - focus on critical paths - **System tests should cover happy paths** for user workflows - **Unit tests should cover edge cases** and error handling --- ### 5. Documentation Standards #### Code Comments - **Document "why", not "what"** - the code shows what it does - **Use comments sparingly** - prefer self-documenting code with clear names - **Always comment "surprising" code** - if it's not obvious, explain it - **Keep comments up-to-date** - outdated comments are worse than no comments ```ruby # Good - Explains why # Stripe requires amount in cents, but our DB stores dollars amount_in_cents = (amount * 100).to_i # Avoid - States the obvious # Multiply amount by 100 amount_in_cents = (amount * 100).to_i ``` #### README Files Every major feature area should have a README if it: - Introduces non-standard patterns - Requires specific setup or configuration - Has complex business logic that benefits from a high-level overview Keep READMEs concise and up-to-date. Link to external documentation when appropriate. #### Method Documentation **Document public API methods** that: - Accept complex parameters - Have non-obvious return values - Have important side effects - Are called by external systems ```ruby # Processes a refund for a purchase # # @param purchase [Purchase] the purchase to refund # @param amount [BigDecimal] the amount to refund (must be <= purchase.total) # @param reason [String] the reason for the refund (required for audit trail) # @return [Refund, nil] the created refund or nil if the purchase is not refundable def process_refund(purchase:, amount:, reason:) # ... end ``` --- ### 6. Git Standards #### Branch Naming Format: `type/short-description` **Types:** - `feature/` - New functionality - `fix/` - Bug fixes - `refactor/` - Code improvements without behavior changes - `docs/` - Documentation only - `test/` - Adding or updating tests - `chore/` - Maintenance tasks (dependency updates, config changes) **Examples:** - `feature/add-product-bundles` - `fix/payment-rounding-error` - `refactor/extract-payment-service` **Guidelines:** - Use lowercase with hyphens (no underscores or camelCase) - Keep descriptions short but meaningful - Delete branches after merging #### Commit Messages Follow the [Conventional Commits](https://www.conventionalcommits.org/) format: ```sh <type>: <description> [optional body] [optional footer] ``` **Types:** `feat`, `fix`, `refactor`, `docs`, `test`, `chore`, `perf`, `style` **Examples:** ```sh feat: add product bundle support Customers can now purchase multiple products as a bundle with a single checkout flow. Bundles support individual product pricing or bundle-level discounts. Closes #1234 ``` ```sh fix: correct payment amount rounding Payment amounts are now rounded consistently with Stripe's requirements (cents, no decimal places). ``` **Guidelines:** - **First line max 72 characters** - it should complete "This commit will..." - **Use imperative mood** - "add feature" not "added feature" - **Body is optional** - use it for complex changes that need explanation - **Reference issues/tickets** in the footer #### Pull Request Titles Use the same format as commit messages: `type: description` **Examples:** - `feat: add product bundle support` - `fix: correct payment amount rounding` - `refactor: extract payment processing service` #### Pull Request Descriptions Every PR should include: 1. **What:** Brief summary of the changes 2. **Why:** The problem this solves or feature it enables 3. **How:** High-level approach (if not obvious from code) 4. **Testing:** How to test the changes 5. **Screenshots:** For UI changes (required) **Template:** ```markdown ## What Brief description of changes ## Why The problem this solves or feature it enables ## How High-level approach (optional if obvious) ## Testing - [ ] Unit tests added/updated - [ ] System tests added/updated - [ ] Manually tested in development ## Screenshots (For UI changes - required) ``` --- ### 7. Code Review Guidelines #### For Authors - **Review your own code first** - read the diff before requesting review - **Keep PRs small** - aim for < 400 lines of changes when possible - **Provide context** - use the PR description template - **Respond to feedback promptly** - acknowledge comments even if you disagree - **Push commits during review** - don't force push while review is in progress #### For Reviewers - **Assume good intent** - authors are trying to do their best work - **Distinguish between**: - **Must fix:** Bugs, security issues, violations of this standards doc - **Should consider:** Suggestions that would improve code quality - **Nitpick:** Personal preferences (prefix with "nitpick:" and author can choose to ignore) - **Approve if "must fix" items are resolved** - don't block on nitpicks - **Explain your reasoning** - especially when requesting changes - **Offer to pair** - if there's significant back-and-forth, jump on a call **Examples of good review comments:** ```sh Must fix: This query is not scoped to Current.site and could leak data across tenants. Should consider: Could we extract this validation logic into the model? It would make the controller thinner and make this validation reusable. Nitpick: I usually prefer guard clauses here, but this is fine as-is. ``` #### Scope of Review Focus on: - **Correctness** - Does it work? Are there bugs? - **Security** - Any SQL injection, XSS, or data leakage risks? - **Testing** - Are critical paths tested? - **Maintainability** - Can the team understand and modify this code? - **Standards compliance** - Does it follow this document? Don't focus on: - **Formatting** - StandardRB and Prettier handle this - **Personal preferences** - unless they impact maintainability - **Rewrites** - unless there's a significant architectural problem --- ### Enforcement #### Automated Checks CI will fail if: - StandardRB violations exist - Prettier violations exist - Tests fail - strong_migrations safety checks fail **These are not negotiable.** Code cannot merge until CI passes. #### During Code Review Reviewers should: - Point to specific sections of this document when requesting changes - Use "must fix" for violations of this document - Use "should consider" for suggestions that would improve code but aren't strictly required - Use "nitpick" for personal preferences #### Updates to This Document - Anyone can propose changes via pull request - Changes require approval from at least 2 engineering managers - Major changes should be discussed in engineering meetings before merging --- ### Resources #### Ruby/Rails - [StandardRB](https://github.com/standardrb/standard) - [Shopify Ruby Style Guide](https://ruby-style-guide.shopify.dev) - [Ruby Style Guide](https://rubystyle.guide) - [strong_migrations](https://github.com/ankane/strong_migrations) #### Frontend - [ViewComponent Best Practices](https://viewcomponent.org/best_practices.html) - [Stimulus Handbook](https://stimulus.hotwired.dev/handbook/introduction) - [React Documentation](https://react.dev) - [Prettier](https://prettier.io/) #### Testing - [Better Specs](https://www.betterspecs.org/) - [Testing Library Guiding Principles](https://testing-library.com/docs/guiding-principles/) #### Git - [Conventional Commits](https://www.conventionalcommits.org/)