How to write readable code?

first published here: http://suckup.de/2020/07/how-to-write-readable-code/

Why do we write unreadable code?

I think we are mostly lazy, and so we use names from underlying structures like APIs or the databases, instead of using names that describe the current situation.

For example, the method “Cart->getUser(): User“: There is a “user_id“ in the underlying database table, so the developer chooses the name “getUser()“ where the User will be returned. The problem is “getX“ and “setX“ are terrible names for methods because they did not say you anything about the context of these methods. Maybe something like “Cart->orderUser(): OrderUser“ and at some point “Cart->orderApprovalUser(): OrderApprovalUser“ are better names. But keep in mind that this depends on your use cases. And please rename also e.g. your “user_id“ in the cart database table, so that you keep the code consistent and readable.

In Eric Evans’ book Domain-Driven Design he introduces the concept of a “Ubiquitous Language“ — a shared, common vocabulary that the entire team shares when discussing or writing software. This “entire team” is made up of designers, developers, the product owner and any other domain experts that exist at the organization. And take the note that it is important that your team have a domain expert! Mostly we are not the expert in the field we are writing software for, so we need some professional input. If you build an invoice-system you will need an expert in finance-questions and -laws.

if ()

BAD:

if ($userExists == 1) { }

BETTER: use "==="

if ($userExists === 1) {}

BETTER: use the correct type

if ($userExists === true) {}

BETTER: use something readable

if ($this->userExists()) {}

BAD:

$foo = 3;
if ($lall === 2) { $foo = 2; }

BETTER: use if-else

if ($lall === 2) { 
    $foo = 2; 
} else {
    $foo = 3;
}

BETTER: only for simple if-else

$foo = ($lall === 2) ? 2 : 3;

BETTER: use something readable

$foo = $this->foo($lall);

BAD:

if ($foo && $foo < 20 && $foo >= 10 || $foo === 100) {}

BETTER: use separate lines

if (
    ($foo && $foo < 20 && $foo >= 10) 
    || 
    ($foo && $foo === 100)
) {}

BETTER: avoid duplications

if (
    $foo
    &&
    (
        ($foo >= 10 && $foo < 20) 
        || 
        $foo === 100
    )
) {}

BETTER: use something readable

if ($this->isFooCorrect($foo)) {}

loop ()

BAD

foreach ($cartArticles as $key => $value)

BETTER: use correct names

foreach ($cartArticles as $cartIndex => $cartArticle)

BETTER: move the loop into a method

cart->getArticles(): Generator<int, Article>

BAD

foreach ($cartArticles as $cartArticle) {
    if ($cartArticle->notActive) {
        $this->removeArticle($cartArticle)
    }
}

BETTER: use continue

foreach ($cartArticles as $cartArticle) {
    if (! $cartArticle->active) {
        continue;
    }

    $this->remove_article($cartArticle)
}

BETTER: use something readable

cart->removeNonActiveArticles(): int;

method ( )

BAD

thisMethodNameIsTooLongOurEyesCanOnlyReadAboutFourCharsAtOnce(bool $pricePerCategory = false): float|float[]

BETTER: use shorter names if possible (long names mostly indicates that the method does more than one thing)

cartNetPriceSum(bool $pricePerCategory = false): float|float[]

BETTER: use less parameter and use one return type

cartNetPriceSum(): float
cartNetPriceSumPerCategory(): float[]

class ()

BAD:

class \shop\InvoicePdfTemplate;

class \shop\InvoicePdfTemplateNew extends \shop\InvoicePdfTemplate;

class \shop\InvoicePdfTemplateSpecial extends \shop\InvoicePdfTemplateNew;

BETTER: use non-generic class names for non-generic classes

class \shop\InvoicePdfTemplate;

class \shop\PdfTemplateCustomerX extends \shop\InvoicePdfTemplate;

class \shop\PdfTemplateActionEaster2020 extends \shop\PdfTemplateCustomerX;

BETTER: do not use multi-inheritance, because of side-effects

class \shop\invoice\PdfTemplateGeneric

class \shop\invoice\PdfTemplate extends \shop\invoice\PdfTemplateGeneric;

class \shop\invoice\PdfTemplateCustomerX extends \shop\invoice\PdfTemplateGeneric;

class \shop\invoice\PdfTemplateActionEaster2020 extends \shop\invoice\PdfTemplateGeneric;

BETTER: use composition via interfaces

interface \shop\invoice\PdfTemplateInterface;

class \shop\invoice\PdfTemplateGeneric implements \shop\invoice\PdfTemplateInterface;

class \shop\invoice\PdfTemplate extends \shop\invoice\PdfTemplateGeneric;

class \shop\invoice\PdfTemplateCustomerX extends \shop\invoice\PdfTemplateGeneric;

class \shop\invoice\PdfTemplateActionEaster2020 implements \shop\invoice\PdfTemplateInterface;

BETTER: use abstract or final

interface \shop\invoice\PdfTemplateInterface;

abstract class \shop\invoice\PdfTemplateGeneric implements \shop\invoice\PdfTemplateInterface;

final class \shop\invoice\PdfTemplate extends \shop\invoice\PdfTemplateGeneric;

final class \shop\invoice\PdfTemplateCustomerX extends \shop\invoice\PdfTemplateGeneric;

final class \shop\invoice\PdfTemplateActionEaster2020 implements \shop\invoice\PdfTemplateInterface;

Real-World-Example

https://github.com/woocommerce/woocommerce/blob/master/includes/class-wc-coupon.php#L505

BAD:

/**
 * Set amount.
 *
 * @since 3.0.0
 * @param float $amount Amount.
 */
public function set_amount( $amount ) {
    $amount = wc_format_decimal( $amount );

    if ( ! is_numeric( $amount ) ) {
        $amount = 0;
    }

    if ( $amount < 0 ) {
        $this->error( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    if ( 'percent' === $this->get_discount_type() && $amount > 100 ) {
        $this->error( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    $this->set_prop( 'amount', $amount );
}

BETTER: fix phpdocs

/**
 * @param float|string $amount Expects either a float or a string with a decimal separator only (no thousands).
 *
 * @since 3.0.0
 */
public function set_amount( $amount ) {
    $amount = wc_format_decimal( $amount );

    if ( ! is_numeric( $amount ) ) {
        $amount = 0;
    }

    if ( $amount < 0 ) {
        $this->error( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    if ( 'percent' === $this->get_discount_type() && $amount > 100 ) {
        $this->error( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    $this->set_prop( 'amount', $amount );
}

BETTER: use php types (WARNING: this is a breaking change because we do not allow string as input anymore)

/**
 * @since 3.0.0
 */
public function set_amount( float $amount ) {
    if ( $amount < 0 ) {
        throw new WC_Data_Exception( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    if (
        $amount > 100
        &&
        'percent' === $this->get_discount_type()
    ) {
        throw new WC_Data_Exception( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    $this->set_prop( 'amount', $amount );
}

BETTER: merge the logic

/**
 * @since 3.0.0
 */
public function set_amount( float $amount ) {
    if (
        $amount < 0
        ||
        (
            $amount > 100
            &&
            'percent' === $this->get_discount_type()
        )
    ) {
        throw new WC_Data_Exception( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    $this->set_prop( 'amount', $amount );
}

BETTER: make the logic readable

/**
 * @since 3.0.0
 */
public function set_amount( float $amount ) {
    if (! is_valid_amount($amount)) {
        throw new WC_Data_Exception( 'coupon_invalid_amount', __( 'Invalid discount amount', 'woocommerce' ) );
    }

    $this->set_prop( 'amount', $amount );
}

private function is_valid_amount( float $amount ): bool {
    if ($amount < 0) {
        return false;
    }

    if (
        $amount > 100
        &&
        'percent' === $this->get_discount_type()
    ) {
        return false;
    }

    return true;
}

Should we write code that everybody can read?

I don’t think that every non-programmer need to read the code, so we do not need to write the code this way. But we should remember that we could write our code, so that every non-programmer could read it.

Summary

There are only two hard things in Computer Science: cache invalidation and naming things.

― Phil Karlton

“Indeed, the ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. …[Therefore,] making it easy to read makes it easier to write.”

― Robert C. Martin, Clean Code: A Handbook of Agile Software Craftsmanship

How to write readable code?

Lars Moelleken

Why do we write unreadable code?

if ()

BAD:

BETTER: use "==="

BETTER: use the correct type

BETTER: use something readable

BAD:

BETTER: use if-else

BETTER: only for simple if-else

BETTER: use something readable

BAD:

BETTER: use separate lines

BETTER: avoid duplications

BETTER: use something readable

loop ()

BAD

BETTER: use correct names

BETTER: move the loop into a method

BAD

BETTER: use continue

BETTER: use something readable

method ( )

BAD

BETTER: use shorter names if possible (long names mostly indicates that the method does more than one thing)

BETTER: use less parameter and use one return type

class ()

BAD:

BETTER: use non-generic class names for non-generic classes

BETTER: do not use multi-inheritance, because of side-effects

BETTER: use composition via interfaces

BETTER: use abstract or final

Real-World-Example

BAD:

BETTER: fix phpdocs

BETTER: use php types (WARNING: this is a breaking change because we do not allow string as input anymore)

BETTER: merge the logic

BETTER: make the logic readable

Should we write code that everybody can read?

Summary

Join Our Newsletter. No Spam, Only the good stuff.

Related