Refactoring pass

สิ่งที่จะสร้าง

ไม่มีฟีเจอร์ใหม่ — มีแต่การ refactor Slugs & validation และ Draft → published ต่างก็ส่งมอบโค้ดที่ใช้งานได้จริงแล้ว แต่ทั้งคู่ก็ทิ้งอะไรบางอย่างไว้ข้างหลัง: PostsService.create/update ตอนนี้คำนวณ slug-กับ-excerpt ซ้ำกัน ตรรกะ excerpt เองก็เป็น private method ที่ยัดอยู่ใน service ที่ยังเป็นเจ้าของ Mongoose query ด้วย และ PostsResolver.posts ก็ถือการตัดสินใจด้าน access-control ไว้เอง แทนที่จะส่งต่อลงไปข้างล่าง บทเรียนนี้แยก apps/api/src/common/excerpt.service.ts (ExcerptService) ออกมา ยุบความซ้ำซ้อนใน PostsService ให้อยู่หลัง private helper ตัวเดียว และย้ายการตัดสินใจเรื่องสิทธิ์เข้าถึง draft กลับไปที่ PostsService.findPage — โดยพฤติกรรมภายนอกเหมือนเดิมทุกประการ ยืนยันได้ด้วยการรัน Verify section ของสองบทเรียนก่อนหน้าซ้ำโดยไม่เปลี่ยนอะไรเลย

ทำไม

การ refactor ในความหมายของ Fowler คือการเปลี่ยนโครงสร้างภายในของโปรแกรมโดยไม่เปลี่ยนพฤติกรรมที่มองเห็นจากข้างนอก ทุก request ที่คอร์สนี้ verify ไว้แล้วต้องยังให้ response เดิมทุกประการหลังจากนี้ วิธีนี้จะใช้ได้ก็ต่อเมื่อมีอะไรให้เช็คเทียบ นั่นคือเหตุผลที่ การ refactor แบบมี test คุ้มกัน โดยเจตนา (แม้จะยังไม่มี automated suite — Testing เป็น module ที่จะมาทีหลัง) หมายถึงการรัน Verify ซ้ำ ไม่ใช่แค่อ่าน diff

สาม smell ที่สะสมมาตลอดสองบทเรียนที่ผ่านมา แต่ละอันคุ้มค่าที่จะเรียกชื่อให้ชัดเจน แทนที่จะบอกแค่ว่า “โค้ดรกขึ้น”:

1. ความซ้ำซ้อน (Duplication) create และ update ต่างก็คำนวณ slug ที่ไม่ซ้ำผ่าน SlugService.generateUnique และต่างก็ดึง excerpt ผ่านการตัดที่ 160 ตัวอักษรแบบเดียวกัน — ตรรกะที่แทบจะเหมือนกันเป๊ะ เขียนไว้สองครั้ง ต่างกันแค่ filter การยกเว้นใน collision predicate จุดเรียกที่สามในอนาคต (เช่น mutation สำหรับ bulk-import) ก็จะก๊อปตรรกะชุดนี้ไปเป็นครั้งที่สาม หรือที่น่าจะเกิดกว่าคือก๊อปไปผิดนิดหน่อย
2. private method ที่ไม่ควรอยู่ในคลาสนั้น deriveExcerpt ไม่เกี่ยวอะไรกับ Mongoose, PostDocument หรือ persistence เลย เป็นการแปลง string ล้วน ๆ ที่บังเอิญไปอยู่ใน PostsService เพราะเป็นที่แรกที่ต้องใช้ ทั้งที่ Slugs & validation แยกตรรกะ slug ที่เทียบเท่ากันออกไปเป็น SlugService ของตัวเองไปแล้ว การปล่อยให้การดึง excerpt ยังเป็น private method อยู่จึงเป็นความไม่สอดคล้องกัน ไม่ใช่ปัญหาที่เล็กกว่า
3. resolver ที่ตัดสินใจเอง ไม่ใช่แค่ส่งต่อการตัดสินใจ Draft → published ใส่ branch ทั้งหมดของ “นี่คือ draft request หรือเปล่า ถ้าใช่ ผู้เรียกคนนี้มีสิทธิ์ทำแบบนั้นไหม” ไว้ใน PostsResolver.posts โดยตรง — ตรรกะแบบเดียวกับที่ convention ของคอร์สนี้เอง (ทุก module ก่อนหน้า) กันออกจาก resolver เสมอ method อื่น ๆ ของ PostsResolver เป็นการส่งต่อบรรทัดเดียวไปยัง PostsService ทั้งหมด มีแต่ posts เท่านั้นที่กลายเป็นข้อยกเว้น

ทั้งสามข้อนี้ไม่ได้ผิดในแง่ที่ให้ผลลัพธ์ไม่ถูกต้อง Verify section ของ Draft → published ผ่านหมดแล้ว แต่ผิดในแง่ที่บทเรียนถัดไปซึ่งจะมาแตะโค้ดส่วนนี้ต้องจ่ายภาษีเพิ่มขึ้นเรื่อย ๆ: จุดซ้ำซ้อนจุดที่สองที่ต้องคอยไล่ให้ตรงกัน service ที่ปนความรับผิดชอบสองเรื่องที่ไม่เกี่ยวกัน และ resolver ที่มองเป็นแค่ “ท่อส่งข้อมูล” ไม่ได้อีกต่อไป

ข้อดีข้อเสีย

Refactor ตอนนี้ กลาง module เทียบกับการเลื่อนออกไปจนกว่าจุดเรียกที่สามจะต้องการตรรกะที่ซ้ำจริง ๆ การแยก ExcerptService และยุบ create/update เข้าด้วยกัน กินเวลาบทเรียนจริงบนโค้ดที่ใช้งานได้อยู่แล้ว และแตะไฟล์ที่ไม่มีใครขอให้เปลี่ยนอีก ทางเลือกอื่น — ปล่อยให้ความซ้ำซ้อนอยู่ตรงนั้นจนกว่าผู้เรียกคนที่สามที่แท้จริงจะโผล่มา แล้วค่อยแยกตอนนั้น — คือเหตุผลแบบตำรา “rule of three” ที่ไม่ควร abstract ก่อนเวลาอันควร DevBlog เลือก refactor ตั้งแต่ตอนนี้โดยเฉพาะเพราะความซ้ำซ้อนมีอยู่แล้วสองจุดเรียก และ ไม่สอดคล้องกับการแยก SlugService ที่เป็นพี่น้องกันอยู่แล้วด้วย การรอจุดที่สามมาพิสูจน์ความจำเป็นของ ExcerptService จะปล่อยให้ deriveExcerpt กับ SlugService นั่งอยู่ข้าง ๆ กันในไฟล์เดียวกัน ดูเหมือนปรัชญาการออกแบบสองแบบที่ต่างกัน ที่เป็นสัญญาณที่แย่กว่าสำหรับผู้อ่านมากกว่าการจ่ายต้นทุนการแยกตอนนี้ ความซ้ำซ้อนเล็ก ๆ ที่มีจุดเรียกเดียวในที่อื่นของ codebase นี้ ต่างหากที่เป็นตัวอย่างที่ดีของสิ่งที่ควรปล่อยไว้จนกว่าจะเกิดซ้ำจริง ๆ

ติดตั้ง

Extract Class/Service deriveExcerpt ย้ายออกจาก PostsService ไปเป็น injectable ของตัวเอง มีรูปทรงเหมือน SlugService เป๊ะ สร้าง apps/api/src/common/excerpt.service.ts:

import { Injectable } from '@nestjs/common';

@Injectable()
export class ExcerptService {
  derive(body: string): string {
    const plain = body
      .replace(/[#*_`>[\]!]/g, '')
      .replace(/\s+/g, ' ')
      .trim();

    return plain.length <= 160 ? plain : `${plain.slice(0, 160).trimEnd()}...`;
  }
}

อัปเดต apps/api/src/common/common.module.ts ให้ provide และ export ExcerptService ควบคู่กับ SlugService:

import { Module } from '@nestjs/common';
import { SlugService } from './slug.service';
import { ExcerptService } from './excerpt.service';

@Module({
  providers: [SlugService, ExcerptService],
  exports: [SlugService, ExcerptService],
})
export class CommonModule {}

Remove Duplication ผ่าน Extract Method ก่อนบทเรียนนี้ create และ update ต่างก็รัน block slug-กับ-excerpt ของตัวเอง:

// BEFORE — apps/api/src/posts/posts.service.ts
async create(authorId: string, input: CreatePostInput): Promise<PostDocument> {
  const slug = await this.slugService.generateUnique(input.title, (candidate) =>
    this.postModel.exists({ slug: candidate }).exec().then(Boolean),
  );
  const excerpt = input.excerpt ?? this.deriveExcerpt(input.body);

  const created = new this.postModel({ ...input, slug, excerpt, author: authorId });
  return created.save();
}

async update(id: string, input: UpdatePostInput): Promise<PostDocument> {
  const patch: Partial<UpdatePostInput> & { slug?: string; excerpt?: string } = { ...input };

  if (input.title) {
    patch.slug = await this.slugService.generateUnique(input.title, (candidate) =>
      this.postModel.exists({ slug: candidate, _id: { $ne: id } }).exec().then(Boolean),
    );
  }
  if (input.body && !input.excerpt) {
    patch.excerpt = this.deriveExcerpt(input.body);
  }

  const updated = await this.postModel.findByIdAndUpdate(id, patch, { new: true }).exec();
  if (!updated) {
    throw new NotFoundException('Post not found');
  }
  return updated;
}

private deriveExcerpt(body: string): string {
  const plain = body
    .replace(/[#*_`>[\]!]/g, '')
    .replace(/\s+/g, ' ')
    .trim();

  return plain.length <= 160 ? plain : `${plain.slice(0, 160).trimEnd()}...`;
}

ทั้งสอง block คำนวณสอง field เดียวกัน ต่างกันแค่ว่าการเช็ค collision ของ slug ยกเว้น _id ของโพสต์เองหรือไม่ private applyContentFields ตัวเดียวแทนที่ทั้งสองอัน โดยเขียนผลลัพธ์ลงบน object ธรรมดาที่ create/update ค่อย merge ทับ input ด้วย spread — เป็น mapper เล็ก ๆ ที่ความซ้ำซ้อนนี้ต้องการ โดยไม่ต้องมีคลาสใหม่ทั้งคลาสสำหรับสอง field:

// AFTER — apps/api/src/posts/posts.service.ts
async create(authorId: string, input: CreatePostInput): Promise<PostDocument> {
  const fields: Partial<Post> = {};
  await this.applyContentFields(fields, input);

  const created = new this.postModel({ ...input, ...fields, author: authorId });
  return created.save();
}

async update(id: string, input: UpdatePostInput): Promise<PostDocument> {
  const fields: Partial<Post> = {};
  await this.applyContentFields(fields, input, id);

  const updated = await this.postModel
    .findByIdAndUpdate(id, { ...input, ...fields }, { new: true })
    .exec();
  if (!updated) {
    throw new NotFoundException('Post not found');
  }
  return updated;
}

private async applyContentFields(
  target: Partial<Post>,
  input: CreatePostInput | UpdatePostInput,
  excludeId?: string,
): Promise<void> {
  if (input.title) {
    target.slug = await this.slugService.generateUnique(input.title, (candidate) =>
      this.postModel
        .exists({ slug: candidate, ...(excludeId ? { _id: { $ne: excludeId } } : {}) })
        .exec()
        .then(Boolean),
    );
  }
  if (input.body && !input.excerpt) {
    target.excerpt = this.excerptService.derive(input.body);
  }
}

การมีหรือไม่มี excludeId คือความแตกต่างจริงหนึ่งเดียวระหว่าง create กับ update — clause _id: { $ne: excludeId } ของการเช็ค collision — เขียนไว้ครั้งเดียวเป็น conditional spread แทนที่จะเป็น predicate closure สองอันที่เขียนแยกกัน

Move Method Draft → published ทิ้งการตัดสินใจเรื่องสิทธิ์เข้าถึง draft ไว้ใน PostsResolver.posts ตรงนี้ย้ายเข้าไปที่ PostsService.findPage ซึ่งตอนนี้รับตัวผู้เรียกเข้ามาตรง ๆ แทนที่จะรับค่าที่คำนวณไว้ล่วงหน้าสองค่า:

// BEFORE — apps/api/src/posts/posts.resolver.ts (the decision lived here)
posts(
  @Args('status', { type: () => PostStatus, nullable: true }) status?: PostStatus,
  @Args('tag', { type: () => String, nullable: true }) tag?: string,
  @Args('page', { type: () => Int, nullable: true }) page?: number,
  @Args('pageSize', { type: () => Int, nullable: true }) pageSize?: number,
  @CurrentUser() currentUser?: AuthenticatedUser,
): Promise<PostsPageResult> {
  let effectiveStatus = status;
  let authorFilter: string | undefined;

  if (status === PostStatus.DRAFT) {
    if (!currentUser) {
      throw new ForbiddenException('Sign in to view drafts');
    }
    if (currentUser.role !== 'admin') {
      authorFilter = currentUser.userId;
    }
  } else if (!status) {
    effectiveStatus = PostStatus.PUBLISHED;
  }

  return this.postsService.findPage({ status: effectiveStatus, tag, page, pageSize, authorFilter });
}

Replace Temp with Query การอัปเดต findPage เดียวกันนี้ยังแทนที่สิ่งที่ปกติจะเป็น temp const isDraftRequest = status === PostStatus.DRAFT ที่คำนวณครั้งเดียวแล้วอ่านสองครั้ง ด้วย private query method ที่ถูกเรียกในแต่ละจุดที่ใช้แทน:

// AFTER — apps/api/src/posts/posts.service.ts
export interface FindPostsPageOptions {
  status?: PostStatus;
  tag?: string;
  page?: number;
  pageSize?: number;
  requestingUser?: AuthenticatedUser;
}

async findPage(options: FindPostsPageOptions = {}): Promise<PostsPageResult> {
  const { status, tag, page = 1, pageSize = 10, requestingUser } = options;
  const filter: Record<string, unknown> = {};

  if (this.isDraftRequest(status)) {
    if (!requestingUser) {
      throw new ForbiddenException('Sign in to view drafts');
    }
    filter.status = PostStatus.DRAFT;
    if (requestingUser.role !== 'admin') {
      filter.author = requestingUser.userId;
    }
  } else {
    filter.status = status ?? PostStatus.PUBLISHED;
  }

  if (tag) {
    filter.tags = tag;
  }

  const skip = (page - 1) * pageSize;
  const [items, total] = await Promise.all([
    this.postModel.find(filter).sort({ createdAt: -1 }).skip(skip).limit(pageSize).exec(),
    this.postModel.countDocuments(filter).exec(),
  ]);

  return { items, total, page, pageSize };
}

private isDraftRequest(status?: PostStatus): boolean {
  return status === PostStatus.DRAFT;
}

PostsResolver.posts หดกลับมาเป็นการส่งต่อบรรทัดเดียวแบบเดียวกับที่ method อื่น ๆ ของ resolver นี้เป็นอยู่แล้ว:

// AFTER — apps/api/src/posts/posts.resolver.ts
@Query(() => PostPage)
@UseGuards(OptionalGqlAuthGuard)
posts(
  @Args('status', { type: () => PostStatus, nullable: true }) status?: PostStatus,
  @Args('tag', { type: () => String, nullable: true }) tag?: string,
  @Args('page', { type: () => Int, nullable: true }) page?: number,
  @Args('pageSize', { type: () => Int, nullable: true }) pageSize?: number,
  @CurrentUser() currentUser?: AuthenticatedUser,
): Promise<PostsPageResult> {
  return this.postsService.findPage({ status, tag, page, pageSize, requestingUser: currentUser });
}

ForbiddenException และ temp ในเครื่อง effectiveStatus/authorFilter หลุดออกจาก posts.resolver.ts ไปทั้งหมด — ไม่มีอะไรตรงนั้น import ForbiddenException อีกต่อไป PostsService รับ interface AuthenticatedUser เล็ก ๆ แบบเดียวกับที่ PostsResolver และ AuthResolver ต่างก็ประกาศไว้เป็น private ของตัวเองอยู่แล้ว — convention ที่ตั้งไว้แล้วของ repo นี้คือ interface เล็ก ๆ ที่ซ้ำกันในแต่ละไฟล์ แทนที่จะเป็น type เดียวที่ใช้ร่วมกัน และการ refactor นี้ก็ทำตาม convention นั้น แทนที่จะนำ shared module ใหม่มาใช้กับรูปทรงสาม field

apps/api/src/posts/posts.service.ts เวอร์ชันปัจจุบันฉบับเต็มหลังจากทั้งสามการย้าย:

import { ForbiddenException, Injectable, NotFoundException } from '@nestjs/common';
import { InjectModel } from '@nestjs/mongoose';
import { Model } from 'mongoose';
import { Post, PostDocument } from './schemas/post.schema';
import { PostStatus } from './enums/post-status.enum';
import { CreatePostInput } from './dto/create-post.input';
import { UpdatePostInput } from './dto/update-post.input';
import { SlugService } from '../common/slug.service';
import { ExcerptService } from '../common/excerpt.service';

interface AuthenticatedUser {
  userId: string;
  email: string;
  role: 'author' | 'admin';
}

export interface FindPostsPageOptions {
  status?: PostStatus;
  tag?: string;
  page?: number;
  pageSize?: number;
  requestingUser?: AuthenticatedUser;
}

export interface PostsPageResult {
  items: PostDocument[];
  total: number;
  page: number;
  pageSize: number;
}

@Injectable()
export class PostsService {
  constructor(
    @InjectModel(Post.name) private readonly postModel: Model<PostDocument>,
    private readonly slugService: SlugService,
    private readonly excerptService: ExcerptService,
  ) {}

  async create(authorId: string, input: CreatePostInput): Promise<PostDocument> {
    const fields: Partial<Post> = {};
    await this.applyContentFields(fields, input);

    const created = new this.postModel({ ...input, ...fields, author: authorId });
    return created.save();
  }

  async findPage(options: FindPostsPageOptions = {}): Promise<PostsPageResult> {
    const { status, tag, page = 1, pageSize = 10, requestingUser } = options;
    const filter: Record<string, unknown> = {};

    if (this.isDraftRequest(status)) {
      if (!requestingUser) {
        throw new ForbiddenException('Sign in to view drafts');
      }
      filter.status = PostStatus.DRAFT;
      if (requestingUser.role !== 'admin') {
        filter.author = requestingUser.userId;
      }
    } else {
      filter.status = status ?? PostStatus.PUBLISHED;
    }

    if (tag) {
      filter.tags = tag;
    }

    const skip = (page - 1) * pageSize;
    const [items, total] = await Promise.all([
      this.postModel.find(filter).sort({ createdAt: -1 }).skip(skip).limit(pageSize).exec(),
      this.postModel.countDocuments(filter).exec(),
    ]);

    return { items, total, page, pageSize };
  }

  findBySlug(slug: string): Promise<PostDocument | null> {
    return this.postModel.findOne({ slug }).exec();
  }

  findById(id: string): Promise<PostDocument | null> {
    return this.postModel.findById(id).exec();
  }

  async update(id: string, input: UpdatePostInput): Promise<PostDocument> {
    const fields: Partial<Post> = {};
    await this.applyContentFields(fields, input, id);

    const updated = await this.postModel
      .findByIdAndUpdate(id, { ...input, ...fields }, { new: true })
      .exec();
    if (!updated) {
      throw new NotFoundException('Post not found');
    }
    return updated;
  }

  async remove(id: string): Promise<PostDocument> {
    const removed = await this.postModel.findByIdAndDelete(id).exec();
    if (!removed) {
      throw new NotFoundException('Post not found');
    }
    return removed;
  }

  async publish(id: string): Promise<PostDocument> {
    const published = await this.postModel
      .findByIdAndUpdate(id, { status: PostStatus.PUBLISHED, publishedAt: new Date() }, { new: true })
      .exec();
    if (!published) {
      throw new NotFoundException('Post not found');
    }
    return published;
  }

  private async applyContentFields(
    target: Partial<Post>,
    input: CreatePostInput | UpdatePostInput,
    excludeId?: string,
  ): Promise<void> {
    if (input.title) {
      target.slug = await this.slugService.generateUnique(input.title, (candidate) =>
        this.postModel
          .exists({ slug: candidate, ...(excludeId ? { _id: { $ne: excludeId } } : {}) })
          .exec()
          .then(Boolean),
      );
    }
    if (input.body && !input.excerpt) {
      target.excerpt = this.excerptService.derive(input.body);
    }
  }

  private isDraftRequest(status?: PostStatus): boolean {
    return status === PostStatus.DRAFT;
  }
}

TagsService ไม่ต้องแก้อะไรเลย เพราะมีแค่ method เดียวที่แตะ SlugService จึงไม่มีความซ้ำซ้อนให้กำจัดและไม่มีตรรกะการตัดสินใจให้ย้าย refactoring pass ควรแตะเฉพาะโค้ดที่สะสม smell ไว้จริง ไม่ใช่ทุกไฟล์ที่ module หนึ่งบังเอิญสร้างขึ้นมา

ตรวจสอบผล

npm run start:dev

การเช็คทุกอันด้านล่างคือ request ที่รันไปแล้วในบทเรียนก่อนหน้า ทำซ้ำเป๊ะ ๆ เพื่อยืนยันว่า refactor นี้ไม่ได้เปลี่ยนอะไรที่สังเกตเห็นได้เลย

จาก Slugs & validation: createPost ด้วยชื่อ "Hello, DevBlog" เป็นครั้งที่สามยังคงคืน slug: "hello-devblog-3" และ excerpt ก็ยังดึงออกมาแบบเดิมเมื่อไม่ได้ระบุ createTag(name: "NestJS") ยังคงล้มเหลวด้วย 409 ConflictException เดิมเมื่อเรียกซ้ำ

จาก Draft → published: posts ที่ไม่มี header Authorization และไม่มี argument status ยังคงเป็นค่าเริ่มต้น PUBLISHED และคืนเฉพาะโพสต์ที่ publish ไปแล้วจริง ๆ เท่านั้น ส่วน posts(status: DRAFT) ที่ไม่มี token ยังคง fail ด้วย 403 เหมือนเดิม ถ้ายิงด้วย token ของ author ที่ใช้ได้ ก็ยังคืนเฉพาะ draft ของ author คนนั้น ถ้ายิงด้วย token ของ admin ก็ยังคืน draft ทุกอัน ส่วน publishPost ยังตั้ง status: PUBLISHED กับ publishedAt ให้เหมือนเดิม และโพสต์ที่ระบุยังโผล่ใน query posts แบบไม่ล็อกอินครั้งถัดไปทันที

ทุกข้อในนี้ให้ response เดิมทุกประการกับก่อนบทเรียนนี้ ซึ่งคือหน้าที่เดียวของการ refactor

สรุป

สี่ท่าที่มีชื่อเรียกชัดเจน โดยพฤติกรรมไม่เปลี่ยนแม้แต่นิดเดียว Extract Class/Service ดึง deriveExcerpt ออกจาก PostsService ไปเป็น ExcerptService ของตัวเอง ให้เข้าคู่กับ SlugService ที่เคยไม่สอดคล้องกัน Remove Duplication ผ่าน Extract Method ยุบ block slug-กับ-excerpt ที่แทบเหมือนกันเป๊ะใน create/update ให้เหลือ private helper applyContentFields ตัวเดียว แล้วรวมกลับเข้ากับ input ด้วย spread แทนที่จะสร้างคลาส mapper เฉพาะ Move Method ย้ายการตัดสินใจเรื่องสิทธิ์เข้าถึง draft จาก PostsResolver.posts ไปที่ PostsService.findPage ที่ซึ่งการตัดสินใจด้านสิทธิ์อื่น ๆ ใน codebase นี้อยู่กันหมดแล้ว และ Replace Temp with Query เปลี่ยนตัวแปรชั่วคราว isDraftRequest ให้กลายเป็น private method ที่เรียกใช้ตรงจุดที่ต้องการ ส่วน TagsService และ resolver method อื่น ๆ ไม่ถูกแตะเลย เพราะไม่มีอะไรในนั้นสะสม smell ไว้จริง

Next: Comments →