Плохое дизайнерское решение выкинуть исключение из аксессора?

Question

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

public class App {
  static class Test {       
    private List<String> strings;

    public Test() {
    }

    public List<String> getStrings() throws Exception {
        if (this.strings == null)
            throw new Exception();

        return strings;
    }

    public void setStrings(List<String> strings) {
        this.strings = strings;
    }
}

public static void main(String[] args) {
    Test t = new Test();

    List<String> s = null;
    try {
        s = t.getStrings();
    } catch (Exception e) {
        // TODO: do something more specific
    }
  }
}

getStrings() выбрасывает Exception, если strings не было установлено. Будет ли лучше справиться с этой ситуацией с помощью метода?

Answer 1

Это действительно зависит от того, что вы делаете со списком, который вы получаете. Я думаю, что более элегантным решением было бы вернуть Colletions.EMPTY_LIST или, как предполагает @Nathan, пустое ArrayList. Затем код, использующий список, может проверить, является ли он пустым, если он хочет, или просто работать с ним, как с любым другим списком.

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

Answer 2

Да, это плохо.Я предлагаю инициализировать строковую переменную в объявлении, например:

private List<String> strings = new ArrayList<String>();

, и вы могли бы заменить сеттер на что-то вроде

public void addToList(String s) {
     strings.add(s);
}

Так что нет никакой возможности NPE.

(В качестве альтернативы не инициализируйте строковую переменную в объявлении, добавьте инициализацию к методу addToList и попросите код, использующий его, проверить getStrings (), чтобы увидеть, получает ли он нулевое значение.)

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

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

Answer 3

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

public List<String> getStrings() {
    if (strings == null)
        throw new IllegalStateException("strings has not been initialized");

    return strings;
}

Быть исключение исключено, вы не обязаны объявлять его в методе, поэтому вы не загрязняете клиентский код большим количеством попыток "поймать / поймать".Кроме того, поскольку этого условия можно избежать (т. Е. Клиентский код может гарантировать, что список был инициализирован), это ошибка программирования , и поэтому непроверенное исключение является приемлемым и предпочтительным.

Answer 4

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

Ответ - да, вы должны работать, чтобы избежать тривиальных исключений.

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

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

В этом примере у вас есть две проблемы:

1. Ваш пример не безопасен для потоков. Эта проверка на нулевое значение может быть успешной (то есть обнаружить, что объект является нулевым) в двух потоках одновременно. Затем ваш экземпляр создает два разных списка строк. Произойдет недетерминированное поведение.

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

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

try {
    // I don't understand why this throws an exception.  Ignore.
    t.getStrings();
} catch (Exception e) {
    // Ignore and hope things are fine.
}

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

public class App {   
    static class Test {            
        // Empty list
        public static final List<String> NULL_OBJECT = new ArrayList<String>();

        private List<String> strings = NULL_OBJECT;

        public synchronized List<String> getStrings() {
            return strings;
        }      

        public synchronized void setStrings(List<String> strings) {         
            this.strings = strings;     
        } 
    }  

    public static void main(String[] args) {     
        Test t = new Test();      

        List<String> s = t.getStrings();

        if (s == Test.NULL_OBJECT) {
            // Do whatever is appropriate without worrying about exception 
            // handling.

            // A possible example below:
            s = new ArrayList<String>();
            t.setStrings(s);
        }

        // At this point, s is a useful reference and 
        // t is in a consistent state.
    }
}

Answer 5

В общем, это запах проекта.

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

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

for (String s: t.getStrings()) {
  // no need to check for null
}

Это может сильно упростить клиентский код.

РЕДАКТИРОВАТЬ: Хорошо, я простовидел, что вы сериализуете из JSON, поэтому вы не можете удалить конструктор.Поэтому вам нужно либо:

1. выбросить исключение IllegalArgumentException из получателя
2. , если строка имеет значение null, вернуть Collections.EMPTY_LIST.
3. или лучше: добавитьпроверка правильности сразу после десериализации, где вы специально проверяете значение строк и генерируете разумное исключение.

Answer 6

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

Answer 7

Я бы обратился к спецификации Java Beans по этому вопросу. Вероятно, было бы лучше не бросать java.lang.Exception, а вместо этого использовать java.beans.PropertyVetoException, и ваш бин регистрируется как java.beans.VetoableChangeListener и запускает соответствующее событие. Это немного излишне, но правильно определит, что вы пытаетесь сделать.

Answer 8

Я бы предложил выбросить исключение из метода установки. Есть ли какая-то особая причина, почему бы не сделать это лучше?

public List<String> getStrings() throws Exception {
    return strings;
}

public void setStrings(List<String> strings) {
    if (strings == null)
        throw new Exception();

    this.strings = strings;
}

Кроме того, в зависимости от конкретного контекста, нельзя ли просто передать List<String> strings напрямую конструктору? Таким образом, возможно, вам даже не понадобится метод установки.