Code review doAction(): проблемы и рефакторинг

33. Отрефакторить doAction() для разных типов транспорта

Условие задачи:
Дан метод doAction(Object source), который формирует строку для объектов Car, Train и Jet.

Нужно провести code review: найти проблемы с типизацией и приведением типов, оценить текущую цепочку if/else и предложить более безопасный и расширяемый вариант реализации.

Код:

public class Main {

    private record Car(String engine) { }
    private record Train(String engine) { }
    private record Jet (String engine) { }

    private record Book(String author, List<String> names) { }

    public static void main(String[] args) {

        // Задание 1: Проанализировать метод doAction(..), предложить возможный рефакторинг, исправления, оптимизацию.
        String doIt = doAction(new Jet(""Jet""));
        System.out.println(doIt);
        }

        // Например, в данном случае вызов для Jet будет после проверки всех условий (плохо).
        // Какие могут быть еще проблемы в этом коде ?
        private static String doAction(Object source) {
                String doIt = ""Just action "";
                if (source instanceof Car car) {
                        return doIt + car.engine();
                } else if (source instanceof Train train) {
                        return doIt + train.engine();
                } else {
                        return doIt + ((Jet) source).engine();
                }
        }
}

Спойлеры к решению

Подсказки

💡 Метод принимает Object, хотя поддерживает только несколько конкретных типов.
💡 В последней ветке выполняется приведение к Jet без проверки типа.
💡 У Car, Train и Jet есть одинаковое поведение — engine(). Можно ли выразить его общим контрактом?
💡 Сам факт, что Jet проверяется последним, здесь практически не является проблемой производительности.

Решение

Проблемы:

doAction() принимает Object, поэтому в него можно передать объект любого типа;
последняя ветка предполагает, что любой объект, который не Car и не Train, обязательно является Jet;
другой тип приведёт к ClassCastException;
null дойдёт до последней ветки и приведёт к NullPointerException;
при появлении новых типов цепочку условий придётся расширять;
Car, Train и Jet имеют одинаковый метод engine(), но общего контракта нет;
Book к задаче doAction() отношения не имеет и создаёт лишний шум.

Замечание о том, что Jet проверяется последним, несущественно: несколько проверок instanceof практически не являются проблемой производительности. Основная проблема — дизайн метода и небезопасный else.

Как лучше исправить

Поскольку для всех трёх типов действие одинаковое, дополнительный switch вообще не нужен. Достаточно общего интерфейса:

interface Vehicle {
    String engine();
}

record Car(String engine) implements Vehicle {
}

record Train(String engine) implements Vehicle {
}

record Jet(String engine) implements Vehicle {
}

Тогда метод становится простым и типобезопасным:

private static String doAction(Vehicle source) {
    if (source == null) {
        throw new IllegalArgumentException(
                "source must not be null"
        );
    }

    return "Just action " + source.engine();
}

Теперь:

нельзя передать Book, String или другой неподдерживаемый объект;
нет небезопасного cast;
нет цепочки if/else;
новый тип транспорта достаточно реализовать через Vehicle, а doAction() менять не потребуется.

Если же для каждого типа в будущем появится разная логика, можно использовать sealed interface и pattern matching switch. Но в текущем коде это избыточно: все ветки отличаются только способом получить один и тот же engine().

Главное исправление — заменить слабый контракт Object общим типом и использовать полиморфизм вместо проверки конкретных классов.