Part 4: GitHub Best Practices and Collaboration

Introduction

I learned the importance of GitHub collaboration practices the hard way. In one project, our team of five developers had no pull request template, no code review guidelines, and no branch protection. The result? Production bugs slipped through, code quality varied wildly, and we spent more time fixing issues than building features.

After implementing proper GitHub workflows and collaboration practices, our code quality improved dramatically, deployment confidence increased, and onboarding new developers became much easier. In this part, I'll share the GitHub practices I've refined through managing TypeScript microservices in production.

Setting Up Your GitHub Repository

Repository Initialization Best Practices

When creating a new repository on GitHub:

1. Choose a Clear, Descriptive Name

# Bad
git clone https://github.com/my-company/app.git
git clone https://github.com/my-company/project1.git

# Good
git clone https://github.com/my-company/payment-service.git
git clone https://github.com/my-company/user-authentication-api.git

2. Add a Comprehensive README

# Payment Service

TypeScript microservice for processing payments in our POS system.

## Features
- Credit card processing via Stripe
- Refund handling
- Payment webhooks
- Multi-currency support

## Prerequisites
- Node.js 18+
- PostgreSQL 14+
- Stripe account

## Quick Start

\`\`\`bash
# Install dependencies
npm install

# Set up environment
cp .env.example .env
# Edit .env with your credentials

# Run database migrations
npm run migrate

# Start development server
npm run dev
\`\`\`

## Environment Variables
- `DATABASE_URL`: PostgreSQL connection string
- `STRIPE_SECRET_KEY`: Stripe API secret key
- `WEBHOOK_SECRET`: Stripe webhook signing secret

## API Documentation
See [API.md](./API.md) or visit http://localhost:3000/api-docs

## Contributing
See [CONTRIBUTING.md](./CONTRIBUTING.md)

## License
MIT

3. Create .gitignore from Start

# .gitignore for TypeScript/Node.js
node_modules/
dist/
build/
.env
.env.local
.env.production
*.log
npm-debug.log*
coverage/
.nyc_output/
.DS_Store
.vscode/
.idea/

4. Choose Appropriate License

For my open-source projects, I use MIT. For company projects, I use proprietary licenses.

Essential Repository Files

CONTRIBUTING.md:

# Contributing to Payment Service

## Development Workflow

1. Fork the repository
2. Create a feature branch (`git checkout -b feature/amazing-feature`)
3. Make your changes
4. Run tests (`npm test`)
5. Commit your changes (`git commit -m 'feat: add amazing feature'`)
6. Push to branch (`git push origin feature/amazing-feature`)
7. Open a Pull Request

## Code Style

We use ESLint and Prettier. Run before committing:

\`\`\`bash
npm run lint
npm run format
\`\`\`

## Commit Message Format

We follow Conventional Commits:

- `feat:` New feature
- `fix:` Bug fix
- `docs:` Documentation changes
- `refactor:` Code refactoring
- `test:` Test additions/changes
- `chore:` Maintenance tasks

## Testing

All new features must include tests:

\`\`\`bash
npm run test
npm run test:coverage
\`\`\`

Target: 80% code coverage

CODE_OF_CONDUCT.md:

# Code of Conduct

## Our Standards

- Be respectful and inclusive
- Accept constructive criticism gracefully
- Focus on what's best for the project
- Show empathy towards other contributors

## Unacceptable Behavior

- Harassment or discriminatory language
- Trolling or insulting comments
- Public or private harassment
- Publishing others' private information

## Enforcement

Violations can be reported to [[email protected]]

Branch Protection Rules

Setting up branch protection is critical for code quality.

Configuring Protection on GitHub

Navigate to: Repository → Settings → Branches → Add rule

My Standard Configuration:

Branch name pattern: main

Protection rules:
☑ Require a pull request before merging
  ☑ Require approvals: 1
  ☑ Dismiss stale pull request approvals when new commits are pushed
  ☑ Require review from Code Owners

☑ Require status checks to pass before merging
  ☑ Require branches to be up to date before merging
  Status checks:
    - build
    - test
    - lint
    - security-scan

☑ Require conversation resolution before merging

☑ Require signed commits (optional, for high-security projects)

☑ Require linear history (prevents merge commits)

☑ Include administrators

☐ Allow force pushes (NEVER enable)
☐ Allow deletions (NEVER enable)

CODEOWNERS File

# .github/CODEOWNERS

# Default owners for everything
* @my-company/core-team

# Payment-related code requires review from payment team
/src/services/payment/ @my-company/payment-team
/src/controllers/payment.controller.ts @my-company/payment-team

# Database migrations require DBA review
/prisma/migrations/ @my-company/database-team

# CI/CD changes require DevOps review
/.github/workflows/ @my-company/devops-team

# Security-sensitive code
/src/middleware/auth.middleware.ts @my-company/security-team
/src/services/encryption.service.ts @my-company/security-team

Pull Request Best Practices

Creating Effective Pull Requests

1. Use a PR Template

# .github/pull_request_template.md

## Description
<!-- Provide a clear description of what this PR does -->

## Type of Change
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Refactoring (no functional changes)
- [ ] Documentation update

## Related Issues
<!-- Link to related issues -->
Closes #

## Changes Made
<!-- List the main changes -->
- 
- 
- 

## Testing
<!-- Describe how you tested these changes -->
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Manual testing completed
- [ ] Tested in staging environment

## Screenshots
<!-- If applicable, add screenshots -->

## Checklist
- [ ] My code follows the project's code style
- [ ] I have performed a self-review of my code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published

## Additional Notes
<!-- Any additional information reviewers should know -->

2. Write Descriptive PR Titles

# Bad
"Fix bug"
"Update files"
"Changes"

# Good
"fix: resolve null pointer exception in payment webhook handler"
"feat: add email verification to user registration flow"
"refactor: extract payment processing logic into separate service"

3. Keep PRs Small and Focused

Bad PR (too large):

Files changed: 47
+2,459 −1,234

Changes:
- Add user authentication
- Refactor database layer
- Update payment processing
- Fix various bugs
- Update documentation

Good PR (focused):

Files changed: 5
+156 −89

Changes:
- Add JWT-based user authentication
- Add authentication middleware
- Add authentication tests

4. Provide Context and Reasoning

## What This PR Does

Adds rate limiting to the payment API to prevent abuse.

## Why This Change is Needed

We've been experiencing DDoS attacks on the payment endpoint. This implements:
- 10 requests per minute per IP address
- 100 requests per hour per authenticated user
- Redis-backed rate limiting for distributed systems

## Implementation Details

Using `express-rate-limit` with Redis store:
- Configured in `src/middleware/rate-limit.middleware.ts`
- Applied to all `/api/payment/*` endpoints
- Returns 429 Too Many Requests when exceeded

## Testing Strategy

- Unit tests verify rate limiting logic
- Integration tests confirm Redis integration
- Load testing shows 99.9% legitimate traffic passes through

## Deployment Notes

Requires Redis instance:
- Development: Uses local Redis
- Production: Uses AWS ElastiCache cluster (already configured)

Real-World PR Example

Here's a PR I created for adding multi-currency support:

# feat: Add Multi-Currency Support to Payment Service

## Summary
Implements multi-currency support allowing customers to pay in their local currency.

## Changes

### 1. Database Schema
- Added `currency` column to `payments` table
- Added `exchange_rates` table for currency conversion
- Migration: `20240207_add_multi_currency_support.sql`

### 2. Payment Service
- Updated `PaymentService` to accept currency parameter
- Added `CurrencyConversionService` for real-time exchange rates
- Integrated with ExchangeRate-API for rate fetching

### 3. API Endpoints
- `POST /api/payments` now accepts `currency` in request body
- `GET /api/currencies/supported` lists supported currencies
- `GET /api/currencies/rates` returns current exchange rates

### 4. Testing
- 45 new unit tests
- 12 integration tests
- Manual testing with USD, EUR, GBP, JPY

## Type of Change
- [x] New feature

## Related Issues
Closes #234
Related to #189 (international expansion initiative)

## Testing

### Unit Tests
\`\`\`bash
npm test

PASS  src/services/payment.service.test.ts
PASS  src/services/currency-conversion.service.test.ts
PASS  src/controllers/payment.controller.test.ts

Test Suites: 3 passed, 3 total
Tests:       45 passed, 45 total
Coverage:    94.2%
\`\`\`

### Integration Tests
\`\`\`bash
npm run test:integration

✓ should process payment in EUR
✓ should convert USD to EUR correctly
✓ should handle unsupported currency gracefully
✓ should fetch latest exchange rates

All integration tests passed
\`\`\`

### Manual Testing
- ✅ Tested payment in USD, EUR, GBP, JPY
- ✅ Verified currency conversion accuracy
- ✅ Tested error scenarios (invalid currency, API timeout)
- ✅ Tested in staging environment

## Screenshots

### API Request/Response
\`\`\`json
POST /api/payments
{
  "amount": 100,
  "currency": "EUR",
  "customerId": "cus_123"
}

Response:
{
  "id": "pay_456",
  "amount": 100,
  "currency": "EUR",
  "amountUsd": 108.50,
  "exchangeRate": 1.085,
  "status": "succeeded"
}
\`\`\`

## Deployment Notes

### Environment Variables (Already Added to Production)
- `EXCHANGE_RATE_API_KEY`: ExchangeRate-API key

### Database Migration
\`\`\`bash
npm run migrate
\`\`\`

### Rollout Plan
1. Deploy to staging ✅ (completed)
2. Monitor for 24 hours ✅ (no issues)
3. Deploy to production 
4. Enable for 10% of traffic (feature flag)
5. Monitor metrics and gradually increase to 100%

## Monitoring

Dashboard: https://grafana.my-company.com/d/payments

Metrics to watch:
- `payment_currency_distribution`
- `currency_conversion_errors`
- `exchange_rate_api_latency`

## Documentation Updated
- ✅ API documentation
- ✅ README.md
- ✅ Database schema docs
- ✅ Deployment guide

## Checklist
- [x] Code follows style guidelines
- [x] Self-reviewed
- [x] Commented complex logic
- [x] Documentation updated
- [x] Tests added and passing
- [x] No new warnings
- [x] Staging deployment successful

## Reviewers
@john-payment-team @sarah-backend-lead

Code Review Guidelines

Being a Good Reviewer

1. Review Promptly

I aim to review PRs within 4 hours during work hours.

2. Be Constructive and Respectful

# Bad
"This code is terrible"
"Why would you do it this way?"

# Good
"Consider using a switch statement here for better readability"
"What do you think about extracting this into a separate function?"

3. Ask Questions, Don't Assume

# Instead of:
"This is wrong"

# Ask:
"Can you help me understand why this approach was chosen over XYZ?"

4. Provide Context for Suggestions

// ❌ Bad Review Comment
"Change this"

// ✅ Good Review Comment
"Consider using Promise.all() here instead of awaiting in a loop. 
This would allow parallel execution and improve performance.

Current:
for (const user of users) {
  await sendEmail(user);  // Sequential, slow
}

Suggested:
await Promise.all(users.map(user => sendEmail(user)));  // Parallel, fast

5. Distinguish Between Must-Fix and Nice-to-Have

# Critical (blocking)
🚨 BLOCKER: This introduces a security vulnerability (SQL injection)

# Important (should fix)
⚠️ IMPORTANT: Missing error handling could cause crashes

# Suggestion (nice to have)
💡 SUGGESTION: Consider adding JSDoc comments here

# Nitpick (optional)
🔍 NITPICK: Minor typo in variable name

Real Code Review Examples

Example 1: Security Issue

// ❌ Code in PR
app.get('/api/users/:id', async (req, res) => {
  const query = `SELECT * FROM users WHERE id = ${req.params.id}`;
  const user = await db.query(query);
  res.json(user);
});

Review Comment:

🚨 BLOCKER: SQL Injection Vulnerability

This code is vulnerable to SQL injection. An attacker could send a request like:
`GET /api/users/1; DROP TABLE users;` to execute arbitrary SQL.

**Suggested Fix:**

\`\`\`typescript
app.get('/api/users/:id', async (req, res) => {
  const user = await db.query(
    'SELECT * FROM users WHERE id = $1',
    [req.params.id]
  );
  res.json(user);
});
\`\`\`

Or better yet, use Prisma ORM:

\`\`\`typescript
app.get('/api/users/:id', async (req, res) => {
  const user = await prisma.user.findUnique({
    where: { id: req.params.id }
  });
  res.json(user);
});
\`\`\`

References:
- [OWASP SQL Injection](https://owasp.org/www-community/attacks/SQL_Injection)
- [Our Security Guidelines](./docs/SECURITY.md)

Example 2: Performance Improvement

// ❌ Code in PR
async function getOrdersWithProducts(userId: string) {
  const orders = await prisma.order.findMany({
    where: { userId }
  });
  
  for (const order of orders) {
    order.products = await prisma.product.findMany({
      where: { orderId: order.id }
    });
  }
  
  return orders;
}

Review Comment:

⚠️ IMPORTANT: N+1 Query Problem

This code makes 1 query for orders, then N additional queries for products (one per order). 
For 100 orders, this results in 101 database queries, which is slow.

**Suggested Fix:**

\`\`\`typescript
async function getOrdersWithProducts(userId: string) {
  const orders = await prisma.order.findMany({
    where: { userId },
    include: {
      products: true  // Single query with JOIN
    }
  });
  
  return orders;
}
\`\`\`

This reduces 101 queries to just 1 query, dramatically improving performance.

Performance comparison (100 orders):
- Current: ~1000ms
- Optimized: ~50ms

Would you like me to pair with you on optimizing this?

Example 3: Code Quality Suggestion

// Code in PR
function calculateTotal(items: any[]) {
  let total = 0;
  for (let i = 0; i < items.length; i++) {
    if (items[i].quantity && items[i].price) {
      total = total + items[i].quantity * items[i].price;
      if (items[i].discount) {
        total = total - items[i].discount;
      }
    }
  }
  return total;
}

Review Comment:

💡 SUGGESTION: Improve Type Safety and Readability

The current implementation works, but we could improve type safety and readability:

\`\`\`typescript
interface CartItem {
  quantity: number;
  price: number;
  discount?: number;
}

function calculateTotal(items: CartItem[]): number {
  return items.reduce((total, item) => {
    const itemTotal = item.quantity * item.price;
    const discountedTotal = itemTotal - (item.discount ?? 0);
    return total + discountedTotal;
  }, 0);
}
\`\`\`

Benefits:
1. Strong typing catches errors at compile time
2. More functional, easier to test
3. More concise and readable
4. Handles undefined discount gracefully

Thoughts? Happy to approve either way, just a suggestion!

Responding to Review Comments

As a PR Author:

1. Thank Reviewers

@reviewer Thanks for catching that SQL injection vulnerability! 
I've updated the code to use parameterized queries.

2. Engage in Discussion

@reviewer Good question about the performance. I chose this approach because:
1. We typically only have 5-10 orders per user
2. The products table has an index on orderId

However, you're right that this could be a problem as we scale. 
I'll refactor to use the include approach. Thanks!

3. Mark Resolved After Fixing

✅ Fixed in commit abc123

Changed to use parameterized query with Prisma.

4. Explain Disagreements Respectfully

I understand the concern about performance, but in this specific case:

1. This code path is only used in admin panel (low traffic)
2. Orders are paginated to max 20 per request
3. I profiled this: averages 45ms which is acceptable for this use case

However, I've added a TODO comment to optimize if we see this becoming 
a bottleneck in metrics. WDYT?

GitHub Actions for Automation

Basic CI/CD Pipeline

# .github/workflows/ci.yml
name: CI

on:
  push:
    branches: [ main, develop ]
  pull_request:
    branches: [ main, develop ]

jobs:
  test:
    runs-on: ubuntu-latest
    
    services:
      postgres:
        image: postgres:14
        env:
          POSTGRES_PASSWORD: postgres
        options: >-
          --health-cmd pg_isready
          --health-interval 10s
          --health-timeout 5s
          --health-retries 5
    
    steps:
      - uses: actions/checkout@v3
      
      - name: Setup Node.js
        uses: actions/setup-node@v3
        with:
          node-version: '18'
          cache: 'npm'
      
      - name: Install dependencies
        run: npm ci
      
      - name: Run linter
        run: npm run lint
      
      - name: Run type check
        run: npm run type-check
      
      - name: Run tests
        run: npm test
        env:
          DATABASE_URL: postgresql://postgres:postgres@localhost:5432/test
      
      - name: Upload coverage
        uses: codecov/codecov-action@v3
        with:
          files: ./coverage/coverage-final.json
  
  build:
    runs-on: ubuntu-latest
    needs: test
    
    steps:
      - uses: actions/checkout@v3
      
      - name: Setup Node.js
        uses: actions/setup-node@v3
        with:
          node-version: '18'
          cache: 'npm'
      
      - name: Install dependencies
        run: npm ci
      
      - name: Build
        run: npm run build
      
      - name: Upload build artifacts
        uses: actions/upload-artifact@v3
        with:
          name: dist
          path: dist/

Auto-Deploy on Merge

# .github/workflows/deploy.yml
name: Deploy

on:
  push:
    branches: [ main ]

jobs:
  deploy:
    runs-on: ubuntu-latest
    
    steps:
      - uses: actions/checkout@v3
      
      - name: Setup Node.js
        uses: actions/setup-node@v3
        with:
          node-version: '18'
      
      - name: Install dependencies
        run: npm ci
      
      - name: Build
        run: npm run build
      
      - name: Deploy to Production
        run: npm run deploy
        env:
          AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
          AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
      
      - name: Notify Slack
        uses: 8398a7/action-slack@v3
        with:
          status: ${{ job.status }}
          text: 'Deployment to production completed!'
          webhook_url: ${{ secrets.SLACK_WEBHOOK }}
        if: always()

Automated PR Labeling

# .github/workflows/label-pr.yml
name: Label PR

on:
  pull_request:
    types: [opened, synchronize]

jobs:
  label:
    runs-on: ubuntu-latest
    
    steps:
      - uses: actions/labeler@v4
        with:
          repo-token: ${{ secrets.GITHUB_TOKEN }}
          configuration-path: .github/labeler.yml

# .github/labeler.yml
'type: feature':
  - 'feat/**'
  - 'feature/**'

'type: bugfix':
  - 'fix/**'
  - 'bugfix/**'

'area: payment':
  - 'src/services/payment/**'
  - 'src/controllers/payment.controller.ts'

'area: database':
  - 'prisma/**'
  - '**/*.prisma'

'needs: documentation':
  - '**/*.ts'
  - '!**/*.test.ts'

GitHub Project Management

Using Issues Effectively

Good Issue Template:

# .github/ISSUE_TEMPLATE/bug_report.md
---
name: Bug Report
about: Report a bug in the application
labels: bug, needs-triage
---

## Bug Description
<!-- Clear description of the bug -->

## Steps to Reproduce
1. 
2. 
3. 

## Expected Behavior
<!-- What should happen -->

## Actual Behavior
<!-- What actually happens -->

## Screenshots/Logs
<!-- If applicable -->

## Environment
- OS: 
- Browser: 
- Version: 

## Additional Context
<!-- Any other relevant information -->

# .github/ISSUE_TEMPLATE/feature_request.md
---
name: Feature Request
about: Suggest a new feature
labels: enhancement, needs-discussion
---

## Feature Description
<!-- Clear description of the feature -->

## Problem This Solves
<!-- What problem does this address? -->

## Proposed Solution
<!-- How should this work? -->

## Alternatives Considered
<!-- Other approaches you've thought about -->

## Additional Context
<!-- Screenshots, mockups, examples, etc. -->

Project Boards

I organize work using GitHub Projects:

Columns:

• 📝 Backlog

• 🎯 Ready for Development

• 🚧 In Progress

• 👀 In Review

• ✅ Done

Automation:

# .github/workflows/project-automation.yml
name: Project Automation

on:
  issues:
    types: [opened, assigned]
  pull_request:
    types: [opened, review_requested, closed]

jobs:
  automate:
    runs-on: ubuntu-latest
    steps:
      - uses: alex-page/[email protected]
        with:
          project: Development
          column: In Progress
          repo-token: ${{ secrets.GITHUB_TOKEN }}

Conclusion

Effective GitHub collaboration requires:

• Clear repository setup with README, contributing guidelines, and code of conduct

• Branch protection to enforce code quality and review processes

• Well-crafted PRs that are small, focused, and well-documented

• Constructive code reviews that improve code quality and share knowledge

• Automated workflows with GitHub Actions for CI/CD

• Organized project management with issues, labels, and project boards

From my experience, investing time in these practices upfront pays dividends in code quality, team productivity, and deployment confidence.

In Part 5, we'll explore advanced version control workflows including release management, hotfix procedures, Git hooks, and troubleshooting common issues.

---

Key Takeaways:

• Branch protection prevents accidental damage to main branch

• Small, focused PRs are easier to review and merge

• Constructive code reviews improve both code and team skills

• GitHub Actions automate repetitive tasks

• Good documentation and templates guide contributors

Remember: GitHub collaboration is about people, not just code!