loading...

How to write readable code?

suckup_de profile image Lars Moelleken ・4 min read

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

Posted on by:

suckup_de profile

Lars Moelleken

@suckup_de

"Es gibt nichts Gutes, außer man tut es" - Erich Kästner

Discussion

markdown guide
 

There's a weird thing in readability that you learn to quickly read the form that you use/read most. This means that if you have multiple repositories full of another format then your "ideal" format will be more difficult. Standardize your code format for the team.

If ternaries aren't used elsewhere then it can take longer to remember how to read it than just having the if/else.

Things like right side open VS left side open can be acceptable either way if you are accustomed to it.

 

Be consistent! Yes, but this cannot be an argument for bad code style.

e.g.:

bad:

.example_1,.example_2{background: url(images/example.png) center center no-repeat, url(images/example-2.png) top left repeat, url(images/example-3.png) top right no-repeat;}

not that bad:

.example_1, 
.example_2 {     
 background: url(images/example.png) center center no-repeat,                            
             url(images/example-2.png) top left repeat,                            
             url(images/example-3.png) top right no-repeat;
}

-> dev.to/suckup_de/do-not-fear-the-w...