Govnokodu.Net

Пример рефакторинга «извлечение класса» в коде поиска подходящего ACL

Исходный код

for ($i=0; $i < $this->acl_count; $i++)
{
	$acl =& $this->acl[$i];
	if (strcasecmp( $aco_section_value, $acl[0] ) == 0) {
		if (strcasecmp( $aco_value, $acl[1] ) == 0) {
			if (strcasecmp( $aro_section_value, $acl[2] ) == 0) {
				if (strcasecmp( $aro_value, $acl[3] ) == 0) {
					if ($axo_section_value && $acl[4]) {
						if (strcasecmp( $axo_section_value, $acl[4] ) == 0) {
							if (strcasecmp( $axo_value, $acl[5] ) == 0) {
								$acl_result = @$acl[6] ? $acl[6] : 1;
								break;
							}
						}
					} else {
						$acl_result = @$acl[6] ? $acl[6] : 1;
						break;
					}
				}
			}
		}
	}
}

Что не так в исходном коде

Первое, что бросается в глаза — это, конечно, адская лесенка из операторов if, хотя она здесь не причина, а следствие других проблем с кодом.

Второй момент, не столь очевидный — это то, как устроена запись ACL: это массив с пронумерованными элементами, в котором у каждого элемента есть собственный смысл. По сути это адская замена структуры с именованными полями на массив с нумерованными элементами. По моему опыту такие выверты никогда ни к чему хорошему не приводят.

Третье — утечка знания. Знания о том, как устроена каждая запись ACL и как выполнять сравнения записи с внешними значениями не инкапсулированы сущность «ACL», а реализуются на вышестоящем уровне, в коде, который работает с записями ACL.

Четвёртый момент — в коде смешаны две разных ответственности, два разных знания: как найти подходящую запись ACL и как сформировать из найденной записи нужный результат.

Пятое — тут продублирована реализация получения результата поиска: @$acl[6] ? $acl[6] : 1.

Шестое — ни о чём не говорящее название acl_result. Название должно указывать на то, что такое есть значение, которое лежит в этой переменной, каков его смысл.

Как лучше всего отрефакторить исходный код

Я приведу идеальный пример рефакторинга, который труднее реализовать, так как он затронет, вероятно, много другого кода.

Мне категорически не нравится, что запись контроля доступа так убого реализована даже не в виде структуры, а в виде массива, поэтому первое, что я сделаю — это добавлю новый класс. Следом я раскидаю исходный код по методам нового класса: проверки отдельно, получение результата отдельно. Состав проверок не обязательно должен быть именно таким: он в значительной степени зависит от смысла полей и параметров. Я исходил из баланса количества проверок и количества аргументов методов. Это рефакторинг «извлечение класса».

class AccessControlRecord {
	public function isAcoMatched($aco_section_value, $aco_value) : boolean {
		return strcasecmp( $aco_section_value, $this->aco_section_value ) == 0 && 
			strcasecmp( $aco_value, $this->aco_value ) == 0;
	}

	public function isAroMatched($aro_section_value, $aro_value) : boolean {
		return strcasecmp( $aro_section_value, $this->aro_section_value ) == 0 && 
			strcasecmp( $aro_value, $this->aro_value ) == 0;
	}
	
	public function isAxoMatched($axo_section_value, $axo_value) : boolean {
		if ($axo_section_value && $this->axo_section_value) {
			return strcasecmp( $axo_section_value, $this->axo_section_value ) == 0 && 
				strcasecmp( $axo_value, $this->axo_value ) == 0;
		}
		return true;
	}

	public function controlFlag() {
		return $this->control_flag ?? 1;
	}
	
	private $aco_section_value;
	private $aco_value;
	private $aro_section_value;
	private $aro_value;
	private $axo_section_value;
	private $axo_value;
}

Теперь я хочу убрать с глаз долой поиск подходящей записи — это рефакторинг «извлечение метода»:

public function matchedRecord($aco_section_value, $aco_value, $aro_section_value, $aro_value, $axo_section_value, $axo_value): ?AccessControlRecord {
	for ($i=0; $i < $this->acl_count; $i++) {
		$acr =& $this->acl[$i];
		if ($acr->isAcoMatched($aco_section_value, $aco_value) && 
				$acr->isAroMatched($aro_section_value, $aro_value) && 
				$acr->isAxoMatched($axo_section_value, $axo_value)) {
			return $acr;
		}
	}
	return null;
}

После всего исходный код станет совсем простым:

$matched_acr = $this->matchedRecord($aco_section_value, $aco_value, $aro_section_value, $aro_value, $axo_section_value, $axo_value);
if ($matched_acr) {
	$flag = $acr->controlFlag();
}

Обрати внимание, что исходный код теперь ничего не знает о том, как искать подходящую запись, как сравнивать поля записи, и как получить из записи нужный конечный результат.

Теория

Сергей Галанин