How do I actually spot a "smell" when the code is still working fine?

That's the trickiest part about legacy code. Everything is passing its tests (if you have any), the users aren't complaining, and the site is live. A "code smell" isn't a bug; it's a hint that the code is fragile. I usually look for what I call "The God Object." This is a class that does everything. If you open a file called User.php and you see methods for calculating taxes, sending emails, validating passwords, and updating a database record all in one place, that's a massive smell.

Here is a classic example of a "God Method" I encountered a few years ago:

public function processOrder($orderId) {
    // 1. Fetch order from DB
    $order = $this->db->query("SELECT * FROM orders WHERE id = $orderId");
    
    // 2. Calculate totals
    $total = 0;
    foreach($order->items as $item) { $total += $item->price; }
    
    // 3. Process payment via CURL to a third-party API
    $ch = curl_init("https://payment-gateway.com/pay");
    curl_setopt($ch, CURLOPT_POSTFIELDS, ['amount' => $total]);
    $response = curl_exec($ch);
    
    // 4. Send confirmation email
    mail($order->email, "Order Confirmed", "Your total is $total");
    
    // 5. Update DB status
    $this->db->query("UPDATE orders SET status = 'paid' WHERE id = $orderId");
}

It works, sure. But it's a nightmare to maintain. If the payment API changes, you're editing the same file where the database logic lives. One typo in the email string could potentially crash the entire payment flow. When you see a method that handles three or four different "responsibilities," that's your red flag.

What are the specific "PHP-isms" I should be worried about?

PHP has evolved a lot, and legacy code often carries baggage from the PHP 4 or 5 eras. One of the biggest smells I look for is the overuse of the global keyword or the $_SESSION superglobal being accessed deep inside business logic. When a function relies on a global variable, it's essentially "lying" about its dependencies. You can't tell what the function needs just by looking at its arguments.

Another big one is "Spaghetti Logic"—where HTML is interleaved with SQL queries. If you see this in a file, run:

<div class="user-profile">
    <?php 
        $res = mysqli_query($conn, "SELECT name FROM users WHERE id = " . $_GET['id']); 
        $user = mysqli_fetch_assoc($res);
        echo "<h1>" . $user['name'] . "</h1>";
    ?php 
    >
</div>

This is a smell because it violates the Separation of Concerns. It makes the code nearly impossible to unit test because you can't trigger the logic without also triggering the HTML output. I've spent weeks debugging "white screen of death" errors only to find a single stray echo statement hidden inside a database helper class.

Is every long method a problem, or am I just overthinking it?

I'll be honest: not every long method is a disaster. Some complex business rules just require a bit of length. However, there is a difference between "complex" and "cluttered." If you find yourself scrolling for three pages to find the end of a function, you're dealing with high cognitive load. I usually ask myself: "Can I explain what this function does in one sentence without using the word 'and'?"

If your answer is "It fetches the user, AND validates the token, AND checks the permissions, AND updates the last-login timestamp," then you have a smell. The "And" is the clue. Each "and" is a candidate for a new, smaller method. I prefer having five 10-line methods with clear names over one 50-line method that requires a map and a compass to navigate.

How do I start cleaning this up without breaking everything?

This is where most developers panic. You see the mess, you want to rewrite the whole thing, and then you accidentally break the checkout page on a Friday afternoon. Don't do that. I follow the "Boy Scout Rule": leave the code slightly cleaner than you found it.

Instead of a full rewrite, use the "Extract Method" technique. Find a chunk of code inside a smelly method that does one specific thing—like sending that email in the example above—and move it into its own protected method. You haven't changed the behavior, but you've started to organize the chaos. Once the logic is isolated, it becomes much easier to move that logic into a separate class entirely without risking a system-wide collapse.

📋 Practical Task

Refactoring the "Everything" Order Class

You have inherited a legacy class called OrderManager. It currently contains a single method, completeOrder(), that handles database updates, API calls for shipping, and logging. This is a classic "God Method" smell.

Your Task: Refactor the provided code by extracting the "Shipping API" logic and the "Logging" logic into their own private methods within the same class.

Constraints:

• Do not change the external behavior of completeOrder().
• Ensure the main completeOrder() method now reads like a high-level summary of the process, delegating the details to your new methods.
• Give your new methods descriptive names (e.g., sendShippingRequest()).

class OrderManager {
    public function completeOrder($orderId) {
        // Database logic
        echo "Updating order $orderId to completed in database...\n";
        
        // Shipping API Logic (SMELL: This should be extracted)
        echo "Connecting to ShipStation API...\n";
        echo "Sending package dimensions and address...\n";
        echo "Receiving tracking number: TRK12345...\n";
        
        // Logging Logic (SMELL: This should be extracted)
        echo "Opening log file...\n";
        echo "Writing: Order $orderId was completed at " . date('Y-m-d H:i:s') . "\n";
        echo "Closing log file...\n";
        
        return true;
    }
}