Refactoring: The Art and Craft of Code Improvement

Luka Peharda

What

What it is not

Insult

What it is not

Excuse

What is Refactoring?

The art of restructuring existing computer code without changing its external behavior

What is Refactoring?

It is a code makeover, but without the need to hire Mirjana Mikulec

What is Refactoring?

"Refactoring is a series of small behavior preserving transformations. Each transformation does little, but a sequence of these transformations can produce a significant restructuring."

Martin Fowler

Why should you refactor your code?

Enhance readability

Why should you refactor your code?

Enhance maintainability

Why should you refactor your code?

Improve extensibility

Why should you refactor your code?

Increase understanding of unfamiliar code

Why should you refactor your code?

Increase performance

Why should you refactor your code?

Reduce technical debt

Why should you refactor your code?

1 + 1 = 3

Let's start

It's a warm summer evening in ancient Greece…

Course / Lesson / Video & Comments

class Course
{
	protected array $lessons;

	public function statement(): string
	{
		$count = count($this->lessons);
		$duration = 0;
		$commentsCount = 0;

		// Loop through lessons and calculate duration and comments count
		foreach ($this->lessons as $lesson) {
			if ($lesson->video !== null) {
				$duration += floor($lesson->video->duration / 60);
			} else {
				$duration += floor(
					str_word_count(
						strip_tags($lesson->text)
						) / 200 * 60
					);
			}

			$commentsCount += count($lesson->comments);
		}

		return sprintf(
			'This course has %d lessons with %d comments and will take %d minutes to complete.',
			$count,
			$commentsCount,
			$duration
		);
	}
}

public function statement(): string
{
	$count = count($this->lessons);
	$duration = 0;
	$commentsCount = 0;

	// Loop through lessons and calculate duration and comments count
	foreach ($this->lessons as $lesson) {
		$duration += $this->getLessonDuration($lesson);
		$commentsCount += count($lesson->comments);
	}

	return sprintf(
		'This course has %d lessons with %d comments and will take %d minutes to complete.',
		$count,
		$commentsCount,
		$duration
	);
}

protected function getLessonDuration(Lesson $lesson): int
{
	if ($lesson->video !== null) {
		return floor($lesson->video->duration / 60);
	} else {
		return floor(
			str_word_count(
				strip_tags($lesson->text)
			) / 200 * 60
		);
	}
}

class Lesson
{
	public ?Video $video = null;
	public ?string $text = null;
	public array $comments;

	public function getDuration(): int
	{
		if ($this->video !== null) {
			return floor($this->video->duration / 60);
		} else {
			return floor(
				str_word_count(
					strip_tags($this->text)
				) / 200 * 60
			);
		}
	}

	public function getCommentsCount(): int
	{
		return count($this->comments);
	}
}

public function statement(): string
{
	$count = count($this->lessons);
	$duration = $commentsCount = 0;

	// Loop through lessons and calculate duration and comments count
	foreach ($this->lessons as $lesson) {
		$duration += $lesson->getDuration();
		$commentsCount += $lesson->getCommentsCount();
	}

	return sprintf(
		'This course has %d lessons with %d comments and will take %d minutes to complete.',
		$count,
		$commentsCount,
		$duration
	);
}

class Course
{
	protected array $lessons;

	protected function calculateDuration(array $lessons): int
	{
		return array_reduce(
			$lessons,
			fn($acc, $lesson) => $acc + $lesson->getDuration()
		, 0);
	}

	protected function commentsCount(array $lessons): int
	{
		return array_reduce(
			$lessons,
			fn($acc, $lesson) => $acc + $lesson->getCommentsCount()
		, 0);
	}

	public function statement(): string
	{
		return sprintf(
			'This course has %d lessons with %d comments and will take %d minutes to complete.',
			count($this->lessons),
			$this->calculateDuration($this->lessons),
			$this->commentsCount($this->lessons)
		);
	}
}

Let's extract all lessons duration calculation in its own method. I'm using reduce here which makes this method short but it could make it less readable especially if you have a lot of juniors in your team. So this is something you need to consider when refactoring.

We do the same for course comments count

"statement" method now looks really short. We've inlined some temp variables as well

Some of you may notice that instead of one loop, we now have two. And this could have performance penalty.

In this particular case this isn't a problem but you need to consider this on each individual case. Sometimes performance would be severly degraded so this kind of refactoring would not be possible and we'd skip this last step

So why do it? We can re-use this methods in other places and don't have to duplicate code.

When to refactor?

Rule of three

When to refactor?

When you add a new feature

When to refactor?

When you fix a bug

When to refactor?

When implementing new feature using a new technique

When to refactor?

During code review

When to refactor?

When procrastinating
or needing a small win to get you started

When to refactor?

If it stinks, change it

Code Smells

Long Method

Long Class

Code Smells

Comments

Code Smells

Comments

// $commentable is a lesson so it should only be made searchable if lesson is not published
if ($commentable::class === Lesson::class && ! $commentable->isPublished()) {
	// ...
}

class Lesson
{
	public function shouldBePublished(): bool
	{
		return $this->isPublished();
	}
}

if ($commentable->shouldBePublished()) {
	// ...
}

Code Smells

Comments

Code Smells

Primitive Obsession

Code Smells

Primitive Obsession

class Order
{
	public function pay(float $amount, string $currency = 'USD'): void
	{
		// ...
	}
}

Code Smells

Primitive Obsession

class Price
{
	public function __construct(
		protected float $amount,
		protected string $currency = 'USD'
	) {
		// ...
	}
}

class Order
{
	public function pay(Price $price): void
	{
		// ...
	}
}

Code Smells

Primitive Obsession

$item = [
	'name' => 'Beer',
	'price' => 2.0,
	'currency' => 'USD'
];

$item = new OrderItem(
	name: 'Beer',
	price: 2.0,
	currency: 'USD'
);

Code Smells

Long Parameter List

Code Smells

Long Parameter List

class MemberService
{
	public function createMember(
		Website $website,
		string $email,
		string $firstName,
		string $lastName,
		bool $sendNotification = false
	): Member {
		$member = Member::create([
			'website_id' => $website->id,
			'email' => $request->email,
			'first_name' => $request->firstName,
			'last_name' => $request->lastName
		]);

		if ($sendNotification) {
			dispatch(SendNotification($website, $member));
		}

		return $member;
	}
}

Code Smells

Long Parameter List

class MemberService
{
	public function createMember(
		Website $website,
		CreateMemberRequest $request,
		bool $sendNotification = false
	): Member {
		$member = Member::create([
			'website_id' => $website->id,
			'email' => $request->email,
			'first_name' => $request->firstName,
			'last_name' => $request->lastName
		]);

		if ($sendNotification) {
			dispatch(SendNotification($website, $member));
		}

		return $member;
	}
}

Code Smells

Long Parameter List

class MemberService
{
	public function createMember(
		Website $website,
		CreateMemberRequest $request
	): Member {
		return Member::create([
			'website_id' => $website->id,
			'email' => $request->email,
			'first_name' => $request->firstName,
			'last_name' => $request->lastName
		]);
	}

	public function createMemberAndSendNotification(
		Website $website,
		CreateMemberRequest $request
	): Member {
		$member = $this->createMember($website, $request);

		dispatch(SendNotification($website, $member));

		return $member;
	}
}

Code Smells

Shotgun Surgery

Code Smells

Shotgun Surgery

class MakeCommentSearchable
{
	public function shouldBeSearchable(): bool
	{
		if ($comment->created_at < now()->subHours(3)) {
			return false;
		}

		return true;
	}
}

class ShowComments
{
	public function shouldBeDisplayed(): bool
	{
		if ($comment->created_at < now()->subHours(3)) {
			return false;
		}

		return true;
	}
}

Code Smells

Shotgun Surgery

class Comment {
	public function wasCreatedRecently(): bool
	{
		if ($this->created_at < now()->subHours(3)) {
			return false;
		}

		return true;
	}
}

public function shouldBeSearchable(): bool
{
	return $comment->wasCreatedRecently();
}

public function shouldBeDisplayed(): bool
{
	return $comment->wasCreatedRecently();
}

Code Smells

Inappropriate Intimacy

Code Smells

Inappropriate Intimacy

class Member
{
	public function getFullAddress(): string
	{
		$output = $this->address->street . ' ' . $this->houseNumber;
		$output .= ', ' . $this->address->city . ' ' . $this->address->zipCode;

		if ($this->address->state) {
			$output .= ', ' . $this->address->state;
		}

		$output .= ', ' . $this->address->country;

		return $output;
	}
}

Code Smells

Inappropriate Intimacy

class Address
{
	public function getFullAddressAsString(): string
	{
		$output = $this->street . ' ' . $this->houseNumber;
		$output .= ', ' . $this->city . ' ' . $this->zipCode;

		if ($this->state) {
			$output .= ', ' . $this->state;
		}

		$output .= ', ' . $this->country;

		return $output;
	}
}

public function getFullAddress(): string
{
	return $this->address->getFullAddress();
}

Code Smells

Feature Envy

Code Smells

Dead Code

Duplicate Code

Techniques

Extract Method / Class

Move Method

Replace Temp with Query

Techniques

Replace Data Value with Object

Introduce Parameter Object

Replace Array with Object

Techniques

Introduce Explaining Variable

Techniques

Introduce Explaining Variable

if (($platform->toUpperCase()->indexOf("MAC") > -1)
&& ($browser->toUpperCase()->indexOf("IE") > -1)
&& $this->wasInitialized()) {
	// ...
}

$isMacOs = $platform->toUpperCase()->indexOf("MAC") > -1;
$isIE = $browser->toUpperCase()->indexOf("IE")  > -1;

if ($isMacOs && $isIE && $this->wasInitialized()) {
	// ...
}

Techniques

Hide Delegate

Techniques

Hide Delegate

$lesson->getVideo()->getDuration();

$lesson->getVideoDuration();

Techniques

Remove Middle Man

Techniques

Remove Middle Man

$lesson->getVideoType();

$lesson->getVideo()->getType();

Techniques

Introduce Foreign Method

Techniques

Introduce Foreign Method

class Report {
	public function sendReport()
	{
		$previousReportDate = clone $this->previousReportDate;
		$paymentDate = $previousReportDate->modify("+7 days");
		// ...
	}
}

Techniques

Introduce Foreign Method

class Report {
	public function sendReport()
	{
		$paymentDate = $this->nextWeek($this->previousReportDate);
		// ...
	}

	/**
	 * Foreign method. Should be in Date.
	*/
	private function nextWeek(DateTime $date)
	{
		$previousReportDate = clone $date;

		return $previousReportDate->modify("+7 days");
	}
}

Techniques

Introduce Local Extension

Techniques

Replace Magic Number with Symbolic Constant

Techniques

Replace Magic Number with Symbolic Constant

class CommentService
{
	public function getPublishedComments()
	{
		return Comment::where('status', 2)->get();
	}

	public function getPendingComments()
	{
		return Comment::where('status', 1)->get();
	}
}

public function getPublishedComments()
{
	return Comment::where('status', 'published')->get();
}

Techniques

Replace Magic Number with Symbolic Constant

class Comment
{
	const STATUS_PUBLISHED = 2;
	const STATUS_PENDING = 1;
}

public function getPublishedComments()
{
	return Comment::where('status', Comment::STATUS_PENDING)->get();
}

Techniques

Decompose Conditional

Techniques

Decompose Conditional

class Order
{
	public function calculateTotal()
	{
		if ($this->taxSettings->includeVat === true && $this->taxSettings->chargeVat === true && $this->taxSettings->vatPercentage > 0 && $this->customer->vatValid === false) {
			$price = $this->price * (1 + $this->taxSettings->vatPercentage / 100);
			$total = $price * $quantity;
		} else {
			$price = $this->price;
			$total = $price * $quantity;
		}

		return $total;
	}
}

Techniques

Decompose Conditional

class Order
{
	public function calculateTotal()
	{
		if (
			$this->taxSettings->includeVat === true
			&& $this->taxSettings->chargeVat === true
			&& $this->taxSettings->vatPercentage > 0
			&& $this->customer->vatValid === false
		) {
			$price = $this->price * (1 + $this->taxSettings->vatPercentage / 100);
			$total = $price * $this->quantity;
		} else {
			$price = $this->price;
			$total = $price * $this->quantity;
		}

		return $total;
	}
}

Techniques

Decompose Conditional

protected function shouldChargeVat(TaxSettings $taxSettings, Customer $customer)
{
	return $taxSettings->includeVat === true && $taxSettings->chargeVat === true
		&& $taxSettings->vatPercentage > 0 && $customer->vatValid === false;
}

protected function calculateTotalVatIncluded()
{
	return $this->price
		* (1 + $this->taxSettings->vatPercentage / 100)
		* $this->quantity;
}

protected function calculateTotalWithoutVat()
{
	return $this->price * $this->quantity;
}

public function calculateTotal()
{
	if ($this->shouldChargeVat($this->taxSettings, $this->customer)) {
		return $this->calculateTotalVatIncluded();
	}

	return $this->calculateTotalWithoutVat();
}

More than one way to skin the cat

Hide delegate VS Remove middle man

Extract method VS Inline method

Add parameter VS Remove parameter

Parametrize method VS Replace parameter with explicit methods

Replace error code with exception VS Replace exception with test

Next Steps

Refactoring by Michael Fowler

https://refactoring.com

Next Steps

https://refactoring.guru

Next Steps

https://kata-log.rocks/refactoring

Next Steps

Mikado Method

Philosophy of Code Refactoring

Leave the code in better shape than you found it

Questions

Class is an extensible program-code-template for creating objects, providing initial values for state (member variables) and implementations of behavior (member functions or methods)

Object is called an instance of the class, and the member variables specific to the object are called instance variables

Facade is a structural design pattern that provides a simple interface to a complex subsystem which contains lots of moving parts