4

Интересно, как грамотнее писать проверку на то, что коллекция не пустая, следуя принципу чистого кода?

if(!col.isEmpty())

Или

if(col.size() > 0)

Или

if(col.get(0) != null)

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

Так, как все-таки лучше?

10
  • 2
    Коварный восклицательный знак можно не заметить - ты программируешь без восклицательных знаков вообще?) 14 ноя 2017 в 16:15
  • 1
    Нет, я их, конечно, использую, например if(!stoped), но в случаях когда If(!getBlah().blah().blah().blah().blah().blah().isEmpty()) его и правда можно не заметить
    – user194625
    14 ноя 2017 в 16:17
  • 4
    if(!col.isEmpty()) только так 14 ноя 2017 в 16:22
  • 1
    If(!getBlah().blah().blah().blah().blah().blah().isEmpty()) его и правда можно не заметить - в таком случае надо программисту руки отрывать, чтоб так не писал 14 ноя 2017 в 16:23
  • 2
    @SergeyGornostaev, строго говоря, они не обязаны. Думаю, существуют коллекции, которые не знают своего размера, пока их не спросят, потому что это сложно.
    – vp_arth
    14 ноя 2017 в 16:36

3 ответа 3

7

if(col.size() > 0)
и
if(col.get(0) != null)

вообще не вариант.

if(!col.isEmpty()) больше подходит, но как вы верно заметили оператор "!" часто можно не заметить, потому что наш мозг плохо работает с отрицанием. Из-за этого иногда трудно будет найти ошибку в коде.

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

Например,

if(!children.isEmpty()) {
    // do something
}

!children.isEmpty() ивлекаю в метод hasChildren().

if (hasChildren()) {
    // do something
}

private boolean hasChildren() {
    return !children.isEmpty();
}

Такой код лучше читается и не путает другого программиста который будет работать с вашим кодам.

Вот еще варианты:
!orders.isEmpty() -> hasOrders(). Если заказы пусты, значит ничего не заказывал(а).
!playlist.isEmpty() -> hasPlaylist(). Если пользователь вообще ничего не добавлял(а) в плейлист, значит у него (нее) нет плейлиста.
и т.д.

4

Во-первых, вы не указали еще вариант

if (col.isEmpty()) {
   return ... // результат если коллекция пуста
} else {
   ... // результат если не пуста
}

Во-вторых, if(col.size() > 0) довольно некрасивый на мой взгляд, тем более что для строк и кастомных классов он не всегда подходит. К тому же кастомные коллекции могут быстро отдавать ответ isEmpty() и долго size(). Иметь один вариант для всех случаев предпочтительнее.

6
  • При наличии return, else is redundant
    – vp_arth
    14 ноя 2017 в 16:37
  • 3
    ... а col.get(0) вообще должно упасть на пустой коллекции
    – VladD
    14 ноя 2017 в 16:40
  • "При наличии return, else is redundant" - а это дело вкуса и стиля, по-мне так читается лучше, так как сразу видно что это два разных взаимоисключающих варианта. А если в первой ветке много кода, то нужно искать глазами return, чтобы понять это взаимоисключающие ветки или ветка дополняющая другую. Для компилятора пофиг, а для человека есть разница. Плюс есть гарантия что в будущем если добавят if'ов в первой ветке и return станет не безусловным, то компилятор ругнется, а не выполнит 2 ветку 14 ноя 2017 в 16:44
  • а это дело вкуса и стиля - мне кажется весь вопрос дело вкуса и стиля... и вообще решаемой задачи 14 ноя 2017 в 16:45
  • @VladD, разве? Вернётся просто null
    – user194625
    16 ноя 2017 в 7:02
0

По уму нужно использовать col.isEmpty().
Плюсы тут не в производительности. Если заглянуть в исходники метода, то, скорее всего, вы сможете увидеть return size == 0;

Все обстоит немного иначе…
Начнем с того, что если писать код правильно и грамотно, то все классы предметной области должны взаимодействовать через их интерфейсы.
Это дает много плюсов, в т.ч. гибкость, ведь по сути, если класс А имеет дело с интерфейсом класса В, то класс А вообще ничего не знает о существовании класса В, вы можете с легкостью даже полностью заменить класс В, главное, чтобы он имплементировал те же интерфейсы.
Именно по этой причине, создавая новый экземпляр класса любой коллекции, мы приводим ее к интерфейсу Map map = new HashMap(). И вот тут самое интересное…

Теперь, вызывая isEmpty(), вы вообще-то обращаетесь к интерфейсу и вообще не привязаны к непосредственной реализации этого метода в классе, либо к каким-либо другим методам. Посему работает все вышесказанное.

4
  • Читаемость этого ответа не соответствует best practice guideline =)
    – vp_arth
    14 ноя 2017 в 16:45
  • ну , простите,объяснил как умею, я не препод)) 14 ноя 2017 в 16:52
  • size() и isEmpty() оба методы интерфейса Collection :) 14 ноя 2017 в 16:57
  • а в чем, собственно, противоречие 14 ноя 2017 в 16:58

Ваш ответ

By clicking “Отправить ответ”, you agree to our terms of service and acknowledge you have read our privacy policy.