8
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

Day 16

クソコードお焚き上げの会 Advent Calendar 2025

@shimada_sljin株式会社スピードリンクジャパン

【黒歴史お焚き上げ🔥】「クソコード」4連発

8
Last updated at Posted at 2025-12-15

はじめに

こんにちは、エンジニア4年目の嶋田です。

クソコードお焚き上げ、面白いカレンダーを見つけたので参加してみようと思います！！
温かい目で見守ってください！

今回は私が1年目に書いた黒歴史をお焚き上げさせていただきます。
マジックナンバー、意味不明な変数名、コピペの連鎖、そして神クラス。
当時はSOLID原則もDRY原則も知りませんでした。

この記事を読んでいる新人エンジニアの皆さん、安心してください。誰もが通る道です。
でも、早めに気づいて改善していきましょう😇

目次

やらかし1: マジックナンバー祭り

どんなコードを書いたか

ユーザーのステータス管理機能を実装した時のコードです。

// 1年目の私が書いたコード
class UserController extends Controller
{
    public function updateStatus(Request $request, $userId)
    {
        $user = User::find($userId);
        
        if ($user->status == 1) {
            // 何かの処理
        } elseif ($user->status == 2) {
            // 別の処理
        } elseif ($user->status == 3 && $user->type == 5) {
            // さらに別の処理
        }
        
        $user->status = 4;
        $user->save();
    }
}

何が問題だったか

  • 1, 2, 3, 4, 5が何を意味するのか全くわからない
  • 書いた本人ですら、1ヶ月後には忘れている
  • 他のメンバーがコードレビューで「これ何？」と質問攻めに
  • 仕様書を見ないと理解できないコード

実際にレビューで言われました。「このマジックナンバー、全部定数にしてください」

どう直したか

// 改善後
class User extends Model
{
    // ステータス定数
    const STATUS_PENDING = 1;      // 承認待ち
    const STATUS_ACTIVE = 2;       // 有効
    const STATUS_SUSPENDED = 3;    // 停止中
    const STATUS_DELETED = 4;      // 削除済み
    
    // タイプ定数
    const TYPE_NORMAL = 1;         // 一般ユーザー
    const TYPE_PREMIUM = 5;        // プレミアムユーザー
}

class UserController extends Controller
{
    public function updateStatus(Request $request, $userId)
    {
        $user = User::find($userId);
        
        if ($user->status == User::STATUS_PENDING) {
            // 承認待ちの処理
        } elseif ($user->status == User::STATUS_ACTIVE) {
            // 有効ユーザーの処理
        } elseif ($user->status == User::STATUS_SUSPENDED && 
                  $user->type == User::TYPE_PREMIUM) {
            // 停止中のプレミアムユーザーの処理
        }
        
        $user->status = User::STATUS_DELETED;
        $user->save();
    }
}

コードが自己文書化されて、仕様書を見なくても理解できるようになりました。

やらかし2: 変数名が適当すぎた

どんなコードを書いたか

データを取得して加工する処理を書いた時のコードです。

// 1年目の私が書いたコード
public function getData($id)
{
    $data = User::find($id);
    $tmp = $data->posts;
    $result = [];
    
    foreach ($tmp as $t) {
        $x = $t->created_at;
        $y = Carbon::parse($x);
        $z = $y->format('Y-m-d');
        $result[] = [
            'a' => $t->title,
            'b' => $z,
            'c' => $t->view_count
        ];
    }
    
    return $result;
}

何が問題だったか

  • 変数名が意味を持っていない
  • $data, $tmp, $result は何のデータ？
  • $x, $y, $z は何？
  • $a, $b, $c も意味不明
  • 処理を追うのに時間がかかる
  • バグが見つけにくい

レビューで「変数名、全部やり直してください」と言われました。

どう直したか

// 改善後
public function getUserPostsSummary($userId)
{
    $user = User::find($userId);
    $posts = $user->posts;
    $postsSummary = [];
    
    foreach ($posts as $post) {
        $createdAt = $post->created_at;
        $createdDate = Carbon::parse($createdAt);
        $formattedDate = $createdDate->format('Y-m-d');
        
        $postsSummary[] = [
            'title' => $post->title,
            'published_date' => $formattedDate,
            'view_count' => $post->view_count
        ];
    }
    
    return $postsSummary;
}

変数名を見ただけで、何をしているか理解できるようになりました。

良い変数名は、最高のドキュメントです。

やらかし3: コピペの連鎖

どんなコードを書いたか

似たような処理を複数書く必要があった時のコードです。

// 1年目の私が書いたコード
class ReportController extends Controller
{
    public function getUserReport()
    {
        $users = User::all();
        $data = [];
        
        foreach ($users as $user) {
            $data[] = [
                'name' => $user->name,
                'email' => $user->email,
                'created_at' => $user->created_at->format('Y-m-d'),
                'status' => $user->status == 1 ? '有効' : '無効'
            ];
        }
        
        return view('reports.users', ['data' => $data]);
    }
    
    public function getCompanyReport()
    {
        $companies = Company::all();
        $data = [];
        
        foreach ($companies as $company) {
            $data[] = [
                'name' => $company->name,
                'email' => $company->email,
                'created_at' => $company->created_at->format('Y-m-d'),
                'status' => $company->status == 1 ? '有効' : '無効'
            ];
        }
        
        return view('reports.companies', ['data' => $data]);
    }
    
    public function getProductReport()
    {
        $products = Product::all();
        $data = [];
        
        foreach ($products as $product) {
            $data[] = [
                'name' => $product->name,
                'code' => $product->code,
                'created_at' => $product->created_at->format('Y-m-d'),
                'status' => $product->status == 1 ? '有効' : '無効'
            ];
        }
        
        return view('reports.products', ['data' => $data]);
    }
}

何が問題だったか

  • ほぼ同じコードが3つ
  • 日付フォーマットを変更したくなった → 3箇所直す必要がある
  • ステータス表示にバグ発見 → 3箇所直す必要がある
  • 新しいレポートを追加 → またコピペ

実際に、ステータス表示のロジックを変更する時に1箇所直し忘れて、本番でバグが発生しました。

DRY原則（Don't Repeat Yourself）を知らなかった私の代償です。

どう直したか

// 改善後
class ReportService
{
    public function generateReport($model, $columns)
    {
        $items = $model::all();
        $data = [];
        
        foreach ($items as $item) {
            $row = [];
            
            foreach ($columns as $column => $label) {
                if ($column === 'status') {
                    $row[$label] = $item->$column == 1 ? '有効' : '無効';
                } elseif ($column === 'created_at') {
                    $row[$label] = $item->$column->format('Y-m-d');
                } else {
                    $row[$label] = $item->$column;
                }
            }
            
            $data[] = $row;
        }
        
        return $data;
    }
}

class ReportController extends Controller
{
    protected $reportService;
    
    public function __construct(ReportService $reportService)
    {
        $this->reportService = $reportService;
    }
    
    public function getUserReport()
    {
        $data = $this->reportService->generateReport(
            User::class,
            ['name' => 'name', 'email' => 'email', 'created_at' => 'created_at', 'status' => 'status']
        );
        
        return view('reports.users', ['data' => $data]);
    }
    
    public function getCompanyReport()
    {
        $data = $this->reportService->generateReport(
            Company::class,
            ['name' => 'name', 'email' => 'email', 'created_at' => 'created_at', 'status' => 'status']
        );
        
        return view('reports.companies', ['data' => $data]);
    }
}

共通処理を1箇所にまとめたことで、修正が1箇所で済むようになりました。

やらかし4: 神クラス降臨

どんなコードを書いたか

ユーザー管理機能を実装した時、1つのクラスに全部詰め込みました。

// 1年目の私が書いたコード
class UserService
{
    // ユーザー登録
    public function register($data)
    {
        // バリデーション
        if (empty($data['email'])) {
            throw new Exception('Email is required');
        }
        
        // メールアドレス重複チェック
        if (User::where('email', $data['email'])->exists()) {
            throw new Exception('Email already exists');
        }
        
        // パスワードハッシュ化
        $data['password'] = Hash::make($data['password']);
        
        // ユーザー作成
        $user = User::create($data);
        
        // ウェルカムメール送信
        Mail::to($user->email)->send(new WelcomeMail($user));
        
        // ログ記録
        Log::info('User registered: ' . $user->id);
        
        return $user;
    }
    
    // ユーザー更新
    public function update($userId, $data)
    {
        // バリデーション
        // メール重複チェック
        // 更新処理
        // ログ記録
        // 通知メール送信
        // ... 100行くらい続く
    }
    
    // ユーザー削除
    public function delete($userId)
    {
        // 削除前チェック
        // 関連データ削除
        // 削除処理
        // ログ記録
        // 通知メール送信
        // ... さらに続く
    }
    
    // ユーザー一覧取得
    public function getList($filters)
    {
        // フィルター処理
        // ページネーション
        // ソート
        // ... また続く
    }
    
    // パスワードリセット
    public function resetPassword($email)
    {
        // ... 
    }
    
    // メール認証
    public function verifyEmail($token)
    {
        // ...
    }
    
    // プロフィール画像アップロード
    public function uploadAvatar($userId, $file)
    {
        // ...
    }
    
    // ユーザー統計情報取得
    public function getStats()
    {
        // ...
    }
    
    // ... メソッドが延々と続く（最終的に500行超え）
}

何が問題だったか

  • 1つのクラスが多すぎる責務を持っている
  • バリデーション、DB操作、メール送信、ログ記録、ファイル操作、すべてが混在
  • どのメソッドがどの責務を持っているのか不明
  • テストが書きにくい
  • 修正する時に影響範囲がわからない
  • 500行を超えて、スクロール地獄

レビューで「これ、全部分割してください」と言われました。
単一責任の原則を知らなかったんです。

どう直したか

// 改善後: 責務ごとにクラスを分割

// バリデーション
class UserValidator
{
    public function validateRegistration(array $data)
    {
        if (empty($data['email'])) {
            throw new ValidationException('Email is required');
        }
        
        if (User::where('email', $data['email'])->exists()) {
            throw new ValidationException('Email already exists');
        }
    }
}

// メール送信
class UserMailService
{
    public function sendWelcomeMail(User $user)
    {
        Mail::to($user->email)->send(new WelcomeMail($user));
    }
    
    public function sendPasswordResetMail(User $user, string $token)
    {
        Mail::to($user->email)->send(new PasswordResetMail($user, $token));
    }
}

// ユーザー登録
class UserRegistrationService
{
    protected $validator;
    protected $mailService;
    
    public function __construct(
        UserValidator $validator,
        UserMailService $mailService
    ) {
        $this->validator = $validator;
        $this->mailService = $mailService;
    }
    
    public function register(array $data)
    {
        $this->validator->validateRegistration($data);
        
        $data['password'] = Hash::make($data['password']);
        $user = User::create($data);
        
        $this->mailService->sendWelcomeMail($user);
        
        Log::info('User registered: ' . $user->id);
        
        return $user;
    }
}

// ユーザー更新
class UserUpdateService
{
    // 更新に関する責務だけを持つ
}

// ユーザー削除
class UserDeletionService
{
    // 削除に関する責務だけを持つ
}

// プロフィール画像管理
class UserAvatarService
{
    // 画像アップロードに関する責務だけを持つ
}

各クラスが1つの責務だけを持つようになり、理解しやすく、テストしやすく、修正しやすくなりました。

学んだこと

この4つのやらかしを通じて、多くのことを学びました。

1. 基本原則の大切さ

SOLID原則、中でも特に

  • Single Responsibility Principle（単一責任の原則）：1つのクラスは1つの責務だけを持つ
  • Open/Closed Principle（開放閉鎖の原則）：拡張に対して開いていて、修正に対して閉じている

DRY原則（Don't Repeat Yourself）：同じコードを繰り返さない

KISS原則（Keep It Simple, Stupid）：シンプルに保つ

これらの原則は、理由があって存在しています。先人の知恵です。

2. コードレビューのありがたみ

当時は「レビューで指摘されるの嫌だな」と思っていました。

でも今思えば、レビューで指摘してもらえることは本当にありがたいです。

  • 本番でバグになる前に気づける
  • より良い書き方を学べる
  • チーム全体のコード品質が上がる

レビュアーの方々、本当にありがとうございました。

3. 可読性は正義

コードは書く時間より、読む時間の方が圧倒的に長いです。

  • 1ヶ月後の自分
  • 半年後の自分
  • 他のチームメンバー
  • 将来のメンテナンス担当者

すべての人のために、読みやすいコードを書く。これがプロフェッショナルの責任です。

後輩に伝えたいこと

最初はみんな書く、恥ずかしくない

私も1年目、こんなコードを書いていました。
先輩エンジニアも、最初はこんなコードを書いていたはずです(たぶん…)

失敗は成長の糧です。恥ずかしがる必要はありません。

でも、学んで改善していこう

大事なのは：

  • 指摘を素直に受け入れること
  • 「なぜダメなのか」を理解すること
  • 同じ失敗を繰り返さないこと
  • 学んだことを次に活かすこと

コードレビューを活用しよう

  • 遠慮せず、レビューを依頼してください
  • 指摘されたら、理由を聞いてください
  • わからないことは、質問してください

レビューは学びの場です。積極的に活用しましょう。

基本原則を学ぼう

  • SOLID原則
  • DRY原則
  • KISS原則
  • クリーンコード
  • リファクタリング

これらは一朝一夕では身につきません。
でも、意識して実践していけば、必ず身につきます。

最後に

4年前の自分に言いたい。

「そのコード、1ヶ月後の自分が泣くよ」

でも、あの失敗があったから、今の自分があります。
失敗から学び、少しは成長できました。

この記事を読んでいる新人エンジニアの皆さん。
失敗を恐れず、でも学ぶことを忘れず、一緒に成長していきましょう！！！

※本記事は個人の経験に基づく見解であり、所属組織の公式見解ではありません。
※コード例は説明のために簡略化しており、実際のプロダクションコードではありません。

8
1
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
8
1

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?