Вечерний рефакторинг с Дженной Ортегой*

На простом примере показываю как можно сделать вашу программу «снова великой».

Исходный код отрефакторенной версии JPC выложен на Github.

Задача

Допустим у вас есть некий софт, написанный еще «при царе Горохе» и созданный неким гордым и очень упертым одиночкой , которого с тех пор никто не видел.

Софт живой, у него есть пользователи, он приносит прибыль и его надо как-то развивать и поддерживать.

В попытке расширить команду разработки, вы начинаете нанимать новых разработчиков, но раз за разом происходит одна и та же ситуация:

Кто-то при увольнении намекает на причины такого поступка, в диапазоне от «ваш проект попахивает» до «срочно все переписать к х#ям».

После десятого сбежавшего программиста, вы наконец начинаете задумываться о том, что с проектом действительно что-то не так и наверное даже стоит провести этот самый «рефакторинг».

The "образец"

В качестве образца для этой статьи был взят один интересный но малоизвестный широкой публике проект JPC:

Да это именно оно:

самый настоящий эмулятор старого x86 компьютера, реализованный без всяких нативных частей — на одной чистой Java!

Как-то так это выглядит у автора(ов):

Проект старый и интересный:

Read more about JPC - since it's launch at JavaOne 2007, JPC can now boot many more operating systems (including graphical linuxes) and it's much faster.

Не очень большой (~6500 файлов), но имеет стадию «внутренней генерации», т.е часть исходного кода создана не вручную, а путем запуска кодогенератора по метаданным.

Что типично и характерно для больших проектов с историей.

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

Текущее состояние

После переезда проекта на Github, список коммитов выглядит следующим образом:

Никаких тестов в проекте нет, по всей видимости тестировалось все вручную с помощью молитвы, еще судя по исходному коду — далеко не весь функционал вообще рабочий.

Что также характерно для проектов в «пред-рефакторинговом» состоянии.

Собирается этот чудо-проект с помощью Makefile, что для мира Java является грязным извращением:

make application

Примерно как собирать проект на QT с помощью Gradle или (еще лучше) — sbt.

Предложите как-нибудь коллегам и посмотрите на реакцию, некоторые точно перестанут с вами здороваться.

Для сборки необходимо иметь путь к JDK в переменной PATH, причем сборка проходит успешно даже с последними версиями (автор использовал OpenJDK 21).

Готовое запускаемое приложение:

JPCApplication.jar

появится в корне проекта после завершения сборки.

На этом хорошие новости заканчиваются и начинается обычный треш:

Собранное приложение цинично отказывается запускаться, причина — восстание роботов изменение поведения URLClassloader при загрузке ресурсов в новых версиях Java.

Но мы это исправим чуть ниже.

Рефакторинг. Стадия 1: Новый скелет

Первым делом, как и в реальном боевом проекте, необходимо избавиться от любого «самопала», задействованного при сборке.

Причина, почему этот шаг критически важен на самом деле не так очевидна:

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

Поэтому я сделал стандартный для своей практики «финт ушами»:

перевел сборку проекта на Apache Maven, максимально широко поддерживаемый средствами анализа кода, CI-системами и средами разработки.

Реализовать такую миграцию в данном случае оказалось очень просто, поскольку проект JPC совсем не использует внешние библиотеки.

Все что я сделал это раскидал ресурсы и исходный код в стандартную для Maven структуру каталогов:

Исходный код был перенесен из каталога src в src/main/java, ресурсы — в src/main/resources.

Также я добавил такой очень простой pom.xml, описывающий шаги сборки проекта:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" 
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 
                             http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <groupId>com.x0x08.samples</groupId>
    <artifactId>jpc-refactored</artifactId>
    <version>1.0-SNAPSHOT</version>
    <packaging>jar</packaging>
    <properties>
        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
        <maven.compiler.source>17</maven.compiler.source>
        <maven.compiler.target>17</maven.compiler.target>
    </properties>
    <build>
        <plugins>
             <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
             <version>3.13.0</version>
        </plugin>
            
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-jar-plugin</artifactId>
                <version>3.4.2</version>
                <configuration>
                    <archive>
                        <manifest>
                            <mainClass>
                                org.jpc.j2se.JPCApplication
                            </mainClass>
                        </manifest>
                    </archive>
                </configuration>
            </plugin>
        </plugins>
    </build>
</project>

Как видите тут все сильно проще чем в оригинальном Makefile, на что есть причины, раскрытые в следующем абзаце.

Также обратите внимание на куда более упрощенную генерацию манифеста:

<manifest>
         <mainClass>
               org.jpc.j2se.JPCApplication
         </mainClass>
</manifest>

Указывается только стартовый класс, используемый для запуска приложения, а дата сборки и название будут проставляться автоматически самим Maven.

В оригинальной сборке происходит добавление еще и атрибута, отвечающего за параметры запуска «по-умолчанию»:

	echo "Default-Args: -fda mem:resources/images/floppy.img -hda mem:resources/images/dosgames.img -boot fda" >> jpc.manifest

Я убрал эту дичь, поскольку точно такой же параметр запуска зашит еще и в код:

Наследие былых времен, не иначе.

Рефакторинг. Стадия 2: Отделение ненужного

Как и в любом долгоживущем проекте, в JPC есть свои «внутренние утилиты» — отдельные программы, написанные для задач автоматизации.

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

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

И то и другое я просто не стал переносить в новую версию.

Также я убрал часть исходного кода эмулятора, отвечающего за отладку (пакет org.jpc.debugger) — по тем же самым причинам.

Что потребовало правок в классе org.jpc.emulator.PC.java:

был убран импорт класса org.jpc.debugger.LinearMemoryViewer а используемая статичная функция translateLinearAddressToInt с особым цинизмом перенесена в класс PC.

Все эти действия позволили сократить кодовую базу проекта практически в двое, что сильно упростило следующий шаг рефакторинга.

Рефакторинг. Стадия 3: Критические проблемы

Вот оно — начало.

(ц) из х\ф «Властелин Колец».

Наконец не прошло и года мы подошли непосредственно к самому рефакторингу, который я буду проводить с помощью очень мощного инструмента "Code Inspector" из среды разработки Intellij Idea.

Открываем pom.xml в среде разработки и Idea автоматически создаст проект. Первый же запуск анализатора дает следующий результат:

24 критических ошибки и 37 тысяч предупреждений — не так плохо, как временами бывает.

Смотрим глубже и видим что все «24 errors» — вообщем-то самые критичные, поскольку это 24 места, из-за которых проект может перестать собираться в самом ближайшем будущем:

Так что именно эти места необходимо рефакторить в первую очередь.

Есть и хорошая новость:

Как видно на скриншоте выше, большая часть ошибок сосредоточена в классе JPCApplet, который используется для запуска приложения в режиме Java-апплета — ныне устаревшей технологии, когда-то работавшей через плагин для браузера непосредственно на веб-странице.

Нам оно в современных реалиях "нах#й не нужно", так что класс можно удалить.

Но все несколько сложнее - класс имеет вложенные статичные классы, один из которых используется снаружи (org.jpc.j2se.JPCApplication):

JPCApplet.PlayPausePanel pp = new JPCApplet.PlayPausePanel(this);

Также как и в прошлый раз, я просто перенес этот статичный класс по месту использования, что позволило удалить JPCApplet из проекта целиком.

Получилось минус 13 критических ошибок за раз.

Что весьма неплохо.

Но едем дальше.

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

Дальше смотрим класс org.jpc.emulator.peripheral.Mixer, он забит предупреждениями от среды разработки буквально через строчку, но массово править пока не стоит — «всемогущая» Idea временами ошибается и это именно такой случай.

Смотрим вот этот метод:

private void ShowVolume(String name,FloatRef vol0,FloatRef vol1) {
   System.out.printf("%-8s %3.0f:%-3.0f  %+3.2f:%-+3.2f \n",new Object[] {name,
            new Float(vol0.value*100),new Float(vol1.value*100),
            new Float(20*Math.log(vol0.value)/Math.log(10.0f)),new Float(20*Math.log(vol1.value)/Math.log(10.0f))}
   );
}

Анализатор ругается (в первую очередь) на конструктор new Float(), поскольку его прямое использование объявлено устаревшим и в новых версиях Java стоит использовать Float.valueOf() в качестве замены.

Конечный вариант выглядит вот так:

  System.out.printf("%-8s %3.0f:%-3.0f  %+3.2f:%-+3.2f \n",
                name,
                vol0.value * 100, vol1.value * 100,
                20*Math.log(vol0.value)/Math.log(10.0f),
                20*Math.log(vol1.value)/Math.log(10.0f));

Оберток Float нет вообще, поскольку в новых версиях Java их использование объявляется как:

Unnecessary boxing

Как видите я также убрал обертку new Object[] {} , поскольку сработало предупреждение:

Redundant array creation for calling varargs method

Следующий класс для изучения org.jpc.j2se.PCMonitorFrame, в котором валидатор ругается на два места (в плане критических ошибок).

Первое очень характерно для старых проектов и будет встречаться вам не раз и не два:

catch (AccessControlException e)
        {
LOGGING.log(Level.WARNING, "Not able to add some components to frame.", e);
        }

Дело в том что класс ошибки, которая тут ловится объявлен устаревшим и должен быть удален в новых версиях:

'java. security. AccessControlException' is deprecated since version 17 and marked for removal

Прежде этот класс был частью sandbox системы, встроенной в JVM, которая позволяла ограничивать функционал платформы, но поскольку в новых версиях Java решили отказаться от самой концепции такого sandbox — этот класс также помечен как устаревший.

Само исключение AccessControlException давно не выбрасывается, поэтому весь блок try-catch можно спокойно удалить.

Следующая остановка, вот такой код в этом же классе:

if (runner.isAlive())
            {
                try 
                {
                    runner.stop();
                } 
                catch (SecurityException e) {}
           }

Ругань идет на метод stop(), который был отмечен как устаревший еще до того как я начал писать на Java:

'stop()' is deprecated since version 1.2 and marked for removal

Примерно до версии 1.8 использование этого метода еще было хоть как-то оправданным из-за гор говнокода и различных поделок на тему многопоточности, но в нынешних реалиях этот метод (фактически реализующий kill -9) просто и банально не работает.

Вот вам официальное объяснение ситуации:

Why is Thread.stop deprecated?

Because it is inherently unsafe. Stopping a thread causes it to unlock all the monitors that it has locked. (The monitors are unlocked as the ThreadDeath exception propagates up the stack.) If any of the objects previously protected by these monitors were in an inconsistent state, other threads may now view these objects in an inconsistent state. Such objects are said to be damaged. When threads operate on damaged objects, arbitrary behavior can result. This behavior may be subtle and difficult to detect, or it may be pronounced. Unlike other unchecked exceptions, ThreadDeath kills threads silently; thus, the user has no warning that his program may be corrupted. The corruption can manifest itself at any time after the actual damage occurs, even hours or days in the future.

Так что этот метод может быть легко заменен на стандартный .interrupt() :

if (runner.isAlive())
            {
                 runner.interrupt();
            }

Блок try-catch также можно спокойно убрать, поскольку SecurityException не выбрасывается в новых версиях Java при попытке остановки потока — это все та же часть sandbox системы.

На этом все критические проблемы в проекте решены и получен минимальный практический смысл от всей затеи:

Рефакторинг. Стадия 4: Ошибки рантайма

Пришло время наконец попробовать собрать и запустить нашего «франкенштейна».

Сборка разумеется завершится успешно, но при запуске будет все та же ошибка поиска ресурсов:

Пришло время радикальных решений:

в оригинальном проекте часть ресурсов (образ биоса) загружалось только из jar-файла, часть (образы дисков) — снаружи, из каталога resources.

Сию х#йню необходимо пресечь и сделать по-нормальному, т.е. как это реализовано в движке знаменитого Quake:

За чтение образа биоса отвечает вот такой метод в классе org.jpc.emulator.motherboard.Bios:

 private static final byte[] getBiosData(String image) throws IOException {
        InputStream in = Bios.class.getResourceAsStream(image);
        if (in == null) {
            throw new IOException("resource not found: " + image);
        }
        try {
            ByteArrayOutputStream bout = new ByteArrayOutputStream();

            while (true) {
                int ch = in.read();
                if (ch < 0) {
                    break;
                }
                bout.write((byte) ch);
            }

            return bout.toByteArray();
        } finally {
            try {
                in.close();
            } catch (IOException e) {
            }
        }
    }
}

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

Конечный вариант после всех чисток выглядит вот так:

private static byte[] getBiosData(String image) throws IOException {

       File f = new File(image);
        if (f.exists() && f.isFile() && f.canRead())
            return Files.readAllBytes(f.toPath());

        f = new File("resources",image);
        if (f.exists() && f.isFile() && f.canRead())
            return Files.readAllBytes(f.toPath());

        final URL u = Bios.class.getResource(image);
        if (u == null)
            throw new IOException("resource (bios) not found: %s".formatted(image));

        try (InputStream in = u.openStream()) {
            return in.readAllBytes();
        }
    }

Логика переделана на возможности современных JDK, поэтому кода стало сильно меньше.

Также я сделал три последовательные проверки на наличие ресурса:

• по полному пути,
• по частичному (предполагается что файл находится в каталоге resources),
• поиск внутри jar приложения.

Но при попытке запуска получаем следующее падение, уже в другом месте:

Причиной является вот такая искусственная проверка:

 if (!(cl instanceof URLClassLoader))
            throw new IllegalStateException();       

Когда-то давно системный класслоадер действительно наследовался от URLClassLoader, но те времена прошли.

Подобные искусственные проверки (фактически это Assert) часто встречаются в отечественных проектах и служат вообщем-то хорошей цели раннего предупреждения о проблеме.

Но временами авторы перебарщивают.

Однако одним лишь удалением проверки дело не ограничивается и необходимо еще зачистить очередной блок говнокода, реализующий «закат солнца вручную»:

    private static final Iterator<String> getResources(String directory)
    {
        ClassLoader context = Thread.currentThread().getContextClassLoader();

        List<String> resources = new ArrayList<String>();

        ClassLoader cl = JPCApplication.class.getClassLoader();
        if (!(cl instanceof URLClassLoader))
            throw new IllegalStateException();
        URL[] urls = ((URLClassLoader) cl).getURLs();
        
        int slash = directory.lastIndexOf("/");
        String dir = directory.substring(0, slash + 1);
        for (int i=0; i<urls.length; i++)
        {
            if (!urls[i].toString().endsWith(".jar"))
                continue;
            try
            {
                JarInputStream jarStream = new JarInputStream(urls[i].openStream());
                while (true)
                {
                    ZipEntry entry = jarStream.getNextEntry();
                    if (entry == null)
                        break;
                    if (entry.isDirectory())
                        continue;

                    String name = entry.getName();
                    slash = name.lastIndexOf("/");
                    String thisDir = "";
                    if (slash >= 0)
                        thisDir = name.substring(0, slash + 1);

                    if (!dir.equals(thisDir))
                        continue;
                    resources.add(name);
                }

                jarStream.close();
            }
            catch (IOException e) { e.printStackTrace();}
        }
        InputStream stream = context.getResourceAsStream(directory);
        try
        {
            if (stream != null)
            {
                Reader r = new InputStreamReader(stream);
                StringBuilder sb = new StringBuilder();
                char[] buffer = new char[1024];
                try
                {
                    while (true)
                    {
                        int length = r.read(buffer);
                        if (length < 0)
                        {
                            break;
                        }
                        sb.append(buffer, 0, length);
                    }
                } finally
                {
                    r.close();
                }

                for (String s : sb.toString().split("\n"))
                {
                    if (context.getResource(directory + s) != null)
                    {
                        resources.add(s);
                    }
                }
            }
        }
        catch (IOException e)
        {
            LOGGING.log(Level.INFO, "Exception reading images directory stream", e);
        }

        return resources.iterator();
    }

Тут происходит поиск доступных образов дисков путем последовательного перебора всех файлов внутри .jar с приложением.

С учетом того что .class файлов ~6500 — такое решение мягко говоря «не оптимально».

Вообще говоря абсолютно любой поиск ресурсов через перебор во время работы приложения является медленным — это и есть основная причина медленного запуска любого приложения на (например) Spring Boot.

Поскольку в проекте используется очень небольшое количество тестовых образов, я просто зашил их имена в код:

 private static final String[] IMAGES = {
            "images/dosgames.img",
            "images/floppy.img",
            "images/linux.img",
            "images/odin070.img",
    };

    private static Iterator<String> getResources(String directory)
    {
        final List<String> resources = new ArrayList<String>
                                  (Arrays.stream(IMAGES).toList());
        final File f = new File(directory);

        if (!f.exists() || !f.isDirectory()) {
            return  resources.iterator();
        }

        final File[] files = f.listFiles();

        if (files == null) {
            return  resources.iterator();
        }
        for (File ff: files) {
            resources.add(directory + ff.getName());
        }

        return resources.iterator();
    }

Суть кода выше — показать список доступных образов дисков для загрузки через меню приложения, все внутренние образы (зашитые в .jar) просто добавляются в этот список сразу.

Но и этих правок оказалось недостаточно и мы едем дальше, следующая остановка — класс org.jpc.support.ArrayBackedSeekableIODevice, который как нетрудно догадаться из названия пакета и самого класса, играет ключевую роль в проекте.

Метод configure отвечает непосредственно за загрузку образов дисков и дискет:

  public void configure(String spec) throws IOException
    {
        resource = spec;
        imageOffset = 0;

        InputStream in = ArrayBackedSeekableIODevice.class.getClassLoader().getResourceAsStream(resource);
        if (in == null) {
            LOGGING.log(Level.SEVERE, "resource not found: {0}", resource);
            throw new IOException("resource not found: " + resource);
        }
        try {
            byte[] buffer = new byte[1024];
            ExposedByteArrayOutputStream bout = new ExposedByteArrayOutputStream(32*1024);

            while (true) {
                int read = in.read(buffer);
                if (read < 0)
                    break;
                bout.write(buffer, 0, read);
            }

            imageData = bout.getBuffer();
            length = bout.getPosition();
        } catch (IOException e) {
            LOGGING.log(Level.SEVERE, "could not load file", e);
            throw e;
        } finally {
            try {
                in.close();
            } catch (IOException e) {
            }
        }
    }

Переделываем с учетом современных реалий:

 public void configure(String spec) throws IOException {
        resource = spec;
        imageOffset = 0;

        File f = new File(spec);
        if (f.exists() && f.isFile() && f.canRead()) {
            imageData = Files.readAllBytes(f.toPath());
            length = imageData.length;
            return;
        }

        f = new File("resources",spec);
        if (f.exists() && f.isFile() && f.canRead()) {
            imageData =  Files.readAllBytes(f.toPath());
            length = imageData.length;
            return;
        }
        final URL u = ArrayBackedSeekableIODevice.class.getResource(spec);
        if (u == null)
             throw new IOException("resource (image) not found: %s"
             .formatted(spec));

        try (InputStream in = u.openStream()) {
                imageData = in.readAllBytes();
                length = imageData.length;
        }
    }

Тут тоже три последовательные проверки на поиск образа диска и использование современного API для минимизации кодовой базы.

После всех этих правок, наконец можно успешно запустить эмулятор, например так:

 java -jar target/jpc-refactored-1.0-SNAPSHOT.jar -hda src/main/resources/images/linux.img 

Запустится Qemu Linux Test Distribution:

Или так:

 java -jar target/jpc-refactored-1.0-SNAPSHOT.jar -fda mem:images/odin070.img 

и запустится FreeDOS ODIN:

Этой стадией была проведена самая настоящая «коммерческая оптимизация», т. е. доведен до ума функционал актуальный конечным пользователям.

На этой стадии рефакторинг помимо технического, получил еще и коммерческий смысл — т. е. есть четкая и актуальная задача, которая была решена.

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

Так что вас за такое скотство, проведенное с рабочим проектом уже не уволят ;)

Рефакторинг. Стадия 5: Массовые правки

Все описанное выше — обязательные базовые части, без которых рефакторинг вообще не может состояться.

Но можно зайти дальше — в действительно рисковую зону, где ваши действия могут иметь не всегда предсказуемые последствия:

массовые и сквозные правки исходного кода, во всем проекте целиком

Рабочая область выглядит как-то так:

Собственно все желтенькое на скриншоте ниже — места для рефакторинга, заботливо подсказанные средой разработки:

К сожалению все несколько сложнее чем подсказывает Idea и просто нажимать «Alt + Shift + Enter» на каждую подсказку не стоит:

Дело в том что в проекте активно используется Reflection API для загрузки и обращения к методам класса через жопу нестандартными способами:

 try {
         Method valueOf = type.getMethod("valueOf", String.class);
         return valueOf.invoke(type, value);
      } catch (NoSuchMethodException e) {
                    System.err.println(type + " :No suitable method");
      }

Что конечно же сводит анализаторы исходного кода с ума, поэтому примерно половина методов в проекте десктоп-приложения, не использующего никакие IoC-контейнеры отмечено как неиспользуемые:

Скотство?

Конечно скотство, но и в реальных больших и старых проектах такая хня тоже будет в обязательном порядке — когда-то использование Reflection API считалось модным и молодежным.

Что делать со всем этим говном предков?

Разумеется убирать, заменяя рефлексию на нормальные вызовы — в большинстве мест это является возможным.

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

Лямбды появились еще в Java 8 и с тех пор уже нет никакого смысла указывать полное имя класса с конструктором.

Почищенная версия выглядит вот так:

В любом legacy-проекте, особенно если это десктоп-приложение такого будет очень и очень много:

И все это вам придется вычистить, хотя-бы ради уменьшения объема кода (одна из ключевых задач рефакторинга).

Следующий предмет для массового рефакторинга — прямой результат ручной разработки, без использования средств проверки кода:

Хороший пример, на котором сработал этот валидатор:

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

Следующие две проблемы — также частые гости любого legacy-проекта:

Точно также как и с ненужными модификаторами в интерфейсе, всего лишь занимают место и увеличивают объем кода.

Хотя если честно, пример выше это совсем уж старый код, поскольку метод Arrays.asList () появился еще в Java 7 — былинные времена далекого прошлого.