How to refactor a long function?

Question

I have a function that prepares a receipt output. But since it has all kinds of conditions it ends up being very long and difficult to understand..

How would one go about refactoring this? Any ideas?

If I split this into 100 small functions would that really be better?

public static function prepare_receipt($printer)
{
    if (self::hasItems($printer['id']))
    {
        $output = '';

        if ($_POST['pre_receipt'])
        {
            $output .= "======== Pre receipt =======\n\n\n";
        }

        /**
         * Time and table
         */
        if ($_POST['isTakeaway'] || $_POST["isDeliveryGuys"] || $_POST["isBolt"]) {
            $output .= "Table: " . $_POST['table'] . "\n";
            $output .= "Floor: " . $_POST['floor'] . "\n";
            $output .= "Time: " . $_POST['takeawayTime'] . "\n";

            if ($_POST['order_comment']) {
                $output .= "Comment: " . removeSpecialChars($_POST['order_comment']) . "\n";
            }
        } else {
            $output .= "Table: " . $_POST['table'] . "\n\n";
            $output .= "Floor: " . $_POST['floor'] . "\n\n";

            if ($_POST['order_comment']) {
                $output .= "Comment: " . removeSpecialChars($_POST['order_comment']) . "\n";
            }
        }

        $output .= "------------------------\n";


        /**
         * Food items
         */
        foreach ($_POST['orderedItems'] as $orderedItem)
        {
            $has_unprinted_quantity = false;

            if (isset($orderedItem['last_printed_quantity'])) {
                $unprinted_quantity_count = intval($orderedItem['is_printed_quantity']) - intval($orderedItem['last_printed_quantity']);

                if ($unprinted_quantity_count > 0) {
                    $has_unprinted_quantity = true;
                }
            }


            if ( ($orderedItem['should_print'] &&
                 !$orderedItem['is_printed'] &&
                  $orderedItem['is_visible']) ||
                  $_POST['pre_receipt'] ||
                  $has_unprinted_quantity)
            {
                if (is_array($orderedItem['printers'])) {
                    $in_printer = in_array($printer['id'], $orderedItem['printers']);
                } else {
                    $in_printer = in_array($printer['id'], json_decode($orderedItem['printers'], true));
                }

                if (  $in_printer || $_POST['pre_receipt'] )
                {
                    if ($orderedItem['is_sidedish'] && !$_POST['pre_receipt']) {
                        continue;
                    }

                    if ($has_unprinted_quantity) {
                        $output .= $unprinted_quantity_count . 'x ';
                    } else {
                        $output .= $orderedItem['quantity'] . 'x ';
                    }

                    // We ned to split it for multiple lines...
                    $itemDescriptionParts = self::split($orderedItem['description']);

                    foreach ($itemDescriptionParts as $itemDescription) {
                        $itemDescriptionClean = removeSpecialChars($itemDescription);
                        $output .= $itemDescriptionClean;
                    }

                    // Add price for pre receipt
                    if ($_POST['pre_receipt']) {
                        $output .= " - " . number_format($orderedItem['price_with_discount'], 2, '.', ',');
                    }
                    
                    if (!$_POST['pre_receipt']) {
                        if ($orderedItem['comments'] != '') {
                            $output .= "   > " . removeSpecialChars(substr($orderedItem['comments'], 0, 27)) . "\n";
                        }
                    }

                    /** Side dishes */
                    if (isset($orderedItem['side_dishes']) && !$_POST['pre_receipt'])
                    {
                        foreach ($orderedItem['side_dishes'] as $side_dish) {
                            $output .= "\n   + " . removeSpecialChars(substr($side_dish['description'], 0, 27)) . "\n";
                        }
                    }

                    $output .= "\n";
                }
            }
        }

        /**
         * Sums
         */


        /**
         * Footer
         */
        $output .= "------------------------\n";

        if ($_POST['pre_receipt'])
        {
            $output .= "\nSubtotal: " . number_format($_POST['order']['subtotal'], 2, '.', ',') . "\n";
            $output .= "Discount: " . number_format($_POST['order']['discount'], 2, '.', ',') . "\n";
            $output .= "Total: " . number_format($_POST['order']['total'], 2, '.', ',') . "\n\n";
        }

        $output .= "Time: " . getTime() . "\n";

        return $output;
    }
    else
    {
        return 'EMPTY';
    }
}

Any pointers in the right direction would be much appreciated.

score 3 · Answer 1 · answered Sep 18 '20 at 10:04

3

Refactoring works often well, if it follows semantics. In your case: You made already comments for different sections. This is often a sign for a function on it's own.

Just to give you an idea: How it may look like afterwards:

$output .= create_headline(...);
$output .= create_time_table(...);
$output .= create_separator();
foreach ($_POST['orderedItems'] as $orderedItem) {
  $output .= create_food_item($orderedItem, $_POST['pre_receipt'], ...);
}
$output .= create_separator();
$output .= create_footer(...);

This will save time when searching for a bug in a certain area of the receipt.

answered Sep 18 '20 at 10:04

leun4m

560
5
24

I think creating seperate functions for headline, footer and creating a function just for separator is inefficient because these are just few lines of code which does not justify separate functions of their own. – blacksheep_trident Sep 18 '20 at 10:27
Well, the seperator should be at least a constant since it is used multiple times already and it might easy enough me used a third time. If you want to change the amount of `-` have fun doing it on multiple places. Surely, you can use search and replace, but ... no. – leun4m Sep 18 '20 at 12:06

Sergio Rinaudo · Answer 2 · 2020-09-18T12:02:35.830

I would advice https://en.wikipedia.org/wiki/Divide-and-conquer_algorithm, and your function already has comment that indicates how this function can be divided in multiple that has a single responsability.

I would also advice not to use $_POST directly, all input data must always be validated and possibly filtered. Data from input should be passed as dependency, to adhere dependency injection, see https://phptherightway.com/ for other good practices.

I would also avoid the use of string concatenation, store all the parts within an array and then join/implode them using a separator.

score 0 · Answer 3 · answered Sep 18 '20 at 10:21

Looking at your code smart use of ternary operators and shifting the orderitem loop into different function can drastically reduce your code length by half. There is no need to create function for each operation like printing head, printing tail etc as the logic in those print is pretty simple and your code can prettry muddy and difficult to navigate if there are large number of unwanted functions. You can do something like below. Also note using . (dot) operator for string concatenation make your string less readable hence prefer printing variables with {} operator.

    <?php

    public static function prepare_receipt($printer)
    {
        if (self::hasItems($printer['id']))
        {
            $output = isset($_POST['pre_receipt']) ? "======== Pre receipt =======\n\n\n" : "" ;
            //addiing time table 
            $output .= "Table: {$_POST['table']}. \n Floor: {$_POST['floor']}. \n";
            //adding time if it is takeaway or isDeliveryGuys or isBolt
            $output .= ($_POST['isTakeaway'] || $_POST["isDeliveryGuys"] || $_POST["isBolt"]) ? "Time: {$_POST['takeawayTime']}. \n" : "" ; 
            //adding order comment
            $output .= $_POST['order_comment']) ? "Comment: {removeSpecialChars($_POST['order_comment'])} \n" : "" ;
            
            //print order items
            this->getOrderItems($_POST[orderedItems], &$output);

            // footer
            $output .= "------------------------\n";
            if ($_POST['pre_receipt'])
                $output .= "\nSubtotal: {number_format($_POST['order']['subtotal'], 2, '.', ',')} \n Discount: { number_format($_POST['order']['discount'], 2, '.', ',') } \n Total: {number_format($_POST['order']['total'], 2, '.', ',')} \n\n";
            $output .= "Time: " . getTime() . "\n";

            return $output;
        }
        else
        {
            return 'EMPTY';
        }
    }

    ?>

.

You shortened lines but IMO it doesn't really add much to readability (if you don't take the comments into account). Have a look at https://stackoverflow.com/questions/209015/what-is-self-documenting-code-and-can-it-replace-well-documented-code — leun4m, Sep 18 '20 at 11:57
Yeah I do agree with your opinion but with usage and experience ternary operators can feel quite natural. — blacksheep_trident, Sep 18 '20 at 12:45

How to refactor a long function?

3 Answers3