Code review: DocumentService и DocumentReader

31. Отрефакторить чтение документов разных форматов

Условие задачи:
Дан DocumentService, который выбирает способ чтения документа в зависимости от его типа: PDF, DOCX или XLSX. Для чтения используется самописный DocumentReader.

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

Код:

@Component
public class DocumentService {

    public InputStream readDocument(String type) {
        DocumentReader reader = new DocumentReader();

        if (type.equals(""PDF"")) {
            return reader.readPdf();
        } else if (type.equals(""DOCX"")) {
            return reader.readDocx();
        } else if (type.equals(""XLSX"")) {
            return reader.readXlsx();
        } else {
            return null;
        }
    }
}

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

Подсказки

💡 Что произойдёт при type == null?
💡 DocumentReader создаётся через new, поэтому его сложно подменить в тестах.
💡 Возврат null не объясняет причину отсутствия результата.
💡 Подумай, сколько мест придётся менять при добавлении нового формата.

Решение

Проблемы:

DocumentReader создаётся через new, поэтому сервис жёстко связан с конкретной реализацией;
зависимость невозможно нормально подменить в unit-тесте;
при type == null вызов type.equals(...) приведёт к NullPointerException;
тип документа задаётся строкой, поэтому возможны опечатки и некорректные значения;
для неподдерживаемого типа возвращается null;
цепочка if/else будет разрастаться при добавлении новых форматов;
DocumentService знает конкретные методы чтения каждого формата, поэтому при расширении приходится изменять сам сервис.

Что здесь нормально

Для трёх форматов код простой и легко читается. Также есть единая точка входа для чтения документа. Проблемы появляются прежде всего при тестировании и дальнейшем расширении списка форматов.

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

Минимальный вариант — внедрить DocumentReader, использовать enum и явно обрабатывать неподдерживаемый тип:

public enum DocumentType {
    PDF,
    DOCX,
    XLSX
}

@Component
@RequiredArgsConstructor
public class DocumentService {

    private final DocumentReader reader;

    public InputStream readDocument(DocumentType type) {
        if (type == null) {
            throw new IllegalArgumentException(
                    "Document type must not be null"
            );
        }

        return switch (type) {
            case PDF -> reader.readPdf();
            case DOCX -> reader.readDocx();
            case XLSX -> reader.readXlsx();
        };
    }
}

Это уже устраняет new, строковые значения и null-результат.

Если форматов будет много и они будут регулярно добавляться, лучше перейти к Strategy.

public interface DocumentReader {

    DocumentType getType();

    InputStream read();
}

Отдельная реализация для каждого формата:

@Component
public class PdfDocumentReader implements DocumentReader {

    @Override
    public DocumentType getType() {
        return DocumentType.PDF;
    }

    @Override
    public InputStream read() {
        // чтение PDF
        return ...;
    }
}

Сервис тогда только выбирает подходящую стратегию:

@Component
public class DocumentService {

    private final Map<DocumentType, DocumentReader> readers;

    public DocumentService(
            List<DocumentReader> readers
    ) {
        this.readers = readers.stream()
                .collect(Collectors.toMap(
                        DocumentReader::getType,
                        Function.identity()
                ));
    }

    public InputStream readDocument(DocumentType type) {
        DocumentReader reader = readers.get(type);

        if (reader == null) {
            throw new IllegalArgumentException(
                    "Unsupported document type: " + type
            );
        }

        return reader.read();
    }
}

Теперь при добавлении нового формата достаточно создать новую реализацию DocumentReader: код DocumentService менять не нужно.

Для небольшого фиксированного набора форматов достаточно варианта с enum и switch. Strategy имеет смысл, когда форматов становится много или логика их обработки развивается независимо.