Анализ одного рефакторинга

    В данном крохотном посте речь пойдет об одной из глав, книги «Принципы, паттерны и методики гибкой разработки на языке C#», с названием «Рефакторинг». Глава полностью посвящена рефакторингу. На примере одного большого метода, автор последовательно модифицирует код, попутно объясняя почему он делает те или иные модификации. После каждого этапа, код прогоняется через тесты.

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

    Статья не преследует цели дискредитации автора, просто показалось интересным выложить свои наблюдения и услышать мнение сообщества по этому поводу.

    Приступим. Первоначальный метод, который автор подверг изменениям:
    /// <remark>
    /// Этот класс генерирует простые числа, не превышающие заданного
    /// пользователем порога. В качестве алгоритма используется решето
    /// Эратосфена.
    ///
    /// Эратосфен Киренский, 276 г. до н. э., Кирена, Ливия --
    /// 194 г. до н. э., Александрия. Впервые измерил окружность Земли.
    /// Известен также работами по составлению календаря с високосными
    /// годами. Работал в Александрийской библиотеке.
    ///
    /// Сам алгоритм прост. Пусть дан массив целых чисел, начинающийся
    /// с 2. Вычеркиваем все кратные 2. Находим первое невычеркнутое
    /// число и вычеркиваем все его кратные. Повторяем, пока не
    /// дойдем до квадратного корня из максимального значения.
    ///
    /// Написал Роберт К. Мартин 9.12.1999 на языке Java
    /// Перевел на C# Мика Мартин 12.01.2005.
    ///</remark>
    
    using System;
    
    /// <summary>
    /// автор: Роберт К. Мартин
    /// </summary>
    
    public class GeneratePrimes
    {
    	///<summary>
    	/// Генерирует массив простых чисел.
    	///</summary>
    	///
    	/// <param name=”maxValue”>Верхний порог.</param>
    	public static int[] GeneratePrimeNumbers(int maxValue)
    	{
    		if (maxValue >= 2) // единственный допустимый случай
    		{
    			// объявления
    			int s = maxValue + 1; // размер массива
    			bool[] f = new bool[s];
    			int i;
    
    			// инициализировать элементы массива значением true.
    			for (i = 0; i < s; i++)
    				f[i] = true;
    
    			// исключить заведомо не простые числа
    			f[0] = f[1] = false;
    
    			// решето
    			int j;
    			for (i = 2; i < Math.Sqrt(s) + 1; i++)
    			{
    				if (f[i]) // если i не вычеркнуто, вычеркнуть его кратные.
    				{
    					for (j = 2 * i; j < s; j += i)
    						f[j] = false; // кратное – не простое число
    				}
    			}
    
    			// сколько оказалось простых чисел?
    			int count = 0;
    			for (i = 0; i < s; i++)
    			{
    				if (f[i])
    					count++; // увеличить счетчик
    			}
    
    			int[] primes = new int[count];
    
    			// поместить простые числа в результирующий массив
    			for (i = 0, j = 0; i < s; i++)
    			{
    				if (f[i]) // если простое
    					primes[j++] = i;
    			}
    
    			return primes; // вернуть простые числа
    		}
    		else // maxValue < 2
    			return new int[0]; // если входные данные не корректны,
    		// возвращаем пустой массив
    	}
    }
    


    Конечный вариант, на котором заканчивается статья, но автор упоминает, что теоретически, можно еще продолжить:
    ///<remark>
    /// Этот класс генерирует простые числа, не превышающие заданного
    /// пользователем порога. В качестве алгоритма используется решето
    /// Эратосфена.
    /// Пусть дан массив целых чисел, начинающийся с 2:
    /// Находим первое невычеркнутое число и вычеркиваем все его
    /// кратные. Повторяем, пока в массиве не останется кратных.
    ///</remark>
    
    using System;
    
    public class PrimeGenerator
    {
    	private static bool[] crossedOut;
    	private static int[] result;
    
    	public static int[] GeneratePrimeNumbers(int maxValue)
    	{
    		if (maxValue < 2)
    			return new int[0];
    		else
    		{
    			UncrossIntegersUpTo(maxValue);
    
    			CrossOutMultiples();
    			PutUncrossedIntegersIntoResult();
    
    			return result;
    		}
    	}
    
    	private static void UncrossIntegersUpTo(int maxValue)
    	{
    		crossedOut = new bool[maxValue + 1];
    
    		for (int i = 2; i < crossedOut.Length; i++)
    			crossedOut[i] = false;
    	}
    
    	private static void PutUncrossedIntegersIntoResult()
    	{
    		result = new int[NumberOfUncrossedIntegers()];
    
    		for (int j = 0, i = 2; i < crossedOut.Length; i++)
    		{
    			if (NotCrossed(i))
    				result[j++] = i;
    		}
    	}
    
    	private static int NumberOfUncrossedIntegers()
    	{
    		int count = 0;
    
    		for (int i = 2; i < crossedOut.Length; i++)
    		{
    			if (NotCrossed(i))
    				count++; // увеличить счетчик
    		}
    
    		return count;
    	}
    
    	private static void CrossOutMultiples()
    	{
    		int limit = DetermineIterationLimit();
    
    		for (int i = 2; i <= limit; i++)
    		{
    			if (NotCrossed(i))
    				CrossOutputMultiplesOf(i);
    		}
    	}
    
    	private static int DetermineIterationLimit()
    	{
    		// У каждого составного числа в этом массиве есть простой
    		// множитель, меньший или равный квадратному корню из размера
    		// массива, поэтому необязательно вычеркивать кратные, большие
    		// корня.
    		double iterationLimit = Math.Sqrt(crossedOut.Length);
    
    		return (int)iterationLimit;
    	}
    
    	private static void CrossOutputMultiplesOf(int i)
    	{
    		for (int multiple = 2 * i;
    			multiple < crossedOut.Length;
    			multiple += i)
    
    			crossedOut[multiple] = true;
    	}
    
    	private static bool NotCrossed(int i)
    	{
    		return crossedOut[i] == false;
    	}
    }
    


    Беглый анализ

    На мой взгляд, в первом примере вообще рефакторинг не нужен, достаточно просто удалить лишние комментарии, но тогда глава была бы не интересная (смайл). В конечном варианте меня смущает только подряд идущие методы, которые не принимают и не возвращают параметры. По моему ощущению, понять логику такой цепочки сложнее, так как не за что зацепиться.

    Подробный анализ

    Итак, первое что мы видим, то что метод является чистой функцией и не имеет побочных эффектов. Отсюда вытекает его детерминированность — при одних и тех же входных параметрах мы получаем одни и те же выходные параметры. После модификации, автор вынес локальные переменные из метода в класс, тем самым добавив состояние классу и добавил побочные эффекты нашему методу. Чем это плохо? Плохо это тем, что раньше наш метод был потокобезопасным, а после этого стал потокоопасным. Получается автор (или мы), невольно добавили потенциальный баг в метод, который рефакторили, при условии использования в многопоточной среде.
    Во вторых, так как переменные все же являются локальными для метода (методов) и не отражают состояние класса, то в дальнейшем, при добавлении методов или состояния, будет сложнее разбираться, чьи же это переменные и зачем они нужны.

    Вместо заключения

    Рефакторинг это прекрасно! Но часто, мы сталкиваемся с проблемой яйца и курицы: чтобы понять как работает код, его необходимо отрефакторить, а чтобы отрефакторить, нужно понимать как он работает.

    Всем спасибо за внимание, жду ваших комментариев.
    AdBlock has stolen the banner, but banners are not teeth — they will be back

    More
    Ads

    Comments 10

      +11
      Это отвратительно, черт побери! Не хватает метода PutCrossedFingers();
        +5
        Некоторые советы в этой книге Боба Мартина довольно спорные, на самом деле. Вот хороший критический отзыв на нее, там в том числе и этот пример с PrimeGenerator бегло проанализирован.
          +5
          С моей точки зрения метод слишком длинный => требует рефакторинга. Выведение всех полей в статики нехорошо не только по причине потокобезопасности, но и потому, что делает неочевидным входы и выходы каждого подметода. Я бы передавал/возвращал масивы явно.

            –3
            Автор все сделал верно — он демонстрировал, как писать понятный код, а это много важнее того, что пропали вторичные качества (потокобезопасность и прочая редко нужная ерунда — интересно кто этот метод будет выполнять из разных потоков? Да и потом как вы себе представляете ускоренную работу этого метода если его целиков выполнять в разных потоках, а разделяя метод, как раз и возникает возможность распаралелизации, а потокобезпасность допелить потом не проблема ). Конечно пример утрирован, но очевидно показывает, что алгоритм надо писатьн не сплошным текстом, а разбивая его на логичные части.
              0
              Рефакторинг — это изменение кода без изменения его функциональности. После этого «рефакторинга» нельзя запустить функцию параллельно для двух разных аргументов так как данные теперь общие и статические.
                –1
                Может теперь вообще все писать гавно-кодом в одной функции? Функциональность не изменилась, уберете static (который почти всегда лишний и все станет на места) — просто не закончен рефакторинг.
                  +1
                  Никто не говорит, что рефакторинг длинных методов не нужен, но если уж показывать пример, то как минимум нужно сказать о потенциальных негативыных последствиях.

                  Ведь никто не отменял правила из Framework Design Guidelines, которые говорят, что все статические методы должны быть потокобезопасными. Причем сделать это довольно просто, достаточно перенести статическое состояние во вновь выделенный объект, а старый метод сделать фасадным методом. Так бы автор показал, что он знает и уважает принципы той платформы, на которой он пишет примеры, да еще бы и один дополнительный паттерн показал бы в придачу.
                    0
                    Думаю в конкретном случае можно было бы пометить эти переменные ThreadStatic атрибутом.
                    0
                    1. habrahabr.ru/post/231325/?reply_to=7820487#comment_7817957
                    2. Это потребует также заменить методы на нестатические и добавить фасадный статик метод как сказали ниже. Вы книжку про рефакторинг читали? Видели сколько там усилий предпринимается, чтобы ничего не сломать?
                0
                Похоже на рефакторинг замена метода объектом методов, но только с какой-то кривой реализацией на статических переменных.
                Данный рефакторинг имеет смысл, если у вас есть длинный метод, в котором локальные переменные так сильно переплетены, что это делает невозможным применение извлечения метода, то есть когда все настолько запущено, что другого выхода практически нет.

                Only users with full accounts can post comments. Log in, please.