Почему драйвер Java MongoDB использует генератор условных чисел в условных выражениях?

211

В этом коммите я видел следующий код для драйвера Java-подключения MongoDB , и на первый взгляд он кажется какой-то шуткой. Что делает следующий код?

if (!((_ok) ? true : (Math.random() > 0.1))) {
    return res;
}

(РЕДАКТИРОВАТЬ: код был обновлен с момента публикации этого вопроса)

Monstieur
источник
13
Какая часть вас смущает?
Оливер Чарльзуорт
4
Я думаю, что это сбивает с толку. этот код выполняется в блоке catch!
Proviste
11
@MarkoTopolnik: это? Это может быть написано гораздо более ясно как if (!ok || Math.random() < 0.1)(или что-то подобное).
Оливер Чарльзуорт
5
github.com/mongodb/mongo-java-driver/commit/… вы не первый, смотрите комментарий к этой строке
msangel
3
@msangel Эти парни, кажется, критикуют логику, а не стиль кодирования.
Марко Топольник

Ответы:

279

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

  1. Эта линия совершенно запутана. Общая форма

    a? true : b

    для boolean a, bэквивалентно простому

    a || b
  2. Окружающее отрицание и чрезмерные скобки еще больше усложняют ситуацию. Принимая во внимание законы Де Моргана, это тривиальное наблюдение, что этот кусок кода составляет

    if (!_ok && Math.random() <= 0.1)
      return res;
    
  3. Коммит, который изначально ввел эту логику, имел

    if (_ok == true) {
      _logger.log( Level.WARNING , "Server seen down: " + _addr, e );
    } else if (Math.random() < 0.1) {
      _logger.log( Level.WARNING , "Server seen down: " + _addr );
    }
    

    - еще один пример некомпетентного кодирования, но обратите внимание на обратную логику : здесь событие регистрируется, если либо _okв одном, либо в 10% других случаев, тогда как код в 2. возвращает 10% случаев и регистрирует 90% случаев. Так что последующий коммит разрушил не только ясность, но и саму корректность.

    Я думаю, что в коде, который вы опубликовали, мы действительно можем увидеть, как автор намеревался if-thenкаким-то образом буквально преобразовать оригинал в отрицание, необходимое для раннего returnсостояния. Но затем он испортил и вставил эффективный «двойной негатив», поменяв знак неравенства.

  4. Помимо проблем со стилем кодирования, стохастическое ведение журнала само по себе является довольно сомнительной практикой, тем более что запись в журнале не документирует свое собственное специфическое поведение. Очевидно, что намерение состоит в том, чтобы уменьшить количество повторений того же факта: сервер в данный момент не работает. Подходящим решением является регистрация только изменений состояния сервера, а не каждого его наблюдения, не говоря уже о случайном отборе 10% таких наблюдений. Да, это займет немного больше усилий, поэтому давайте посмотрим на некоторые.

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

Марко Топольник
источник
26
Кроме того, насколько я могу судить, это официальный драйвер Java 10gen для MongoDB, поэтому, помимо мнения о драйвере Java, я думаю, что он дает мне мнение о коде MongoDB
Крис Траверс,
5
Отличный анализ всего лишь нескольких строк кода, я мог бы просто превратить его в вопрос для интервью! Ваш четвертый пункт - реальный ключ, почему с этим проектом что-то принципиально не так (остальные могут быть отклонены как ошибки неудачного программиста).
Авель
1
@ChrisTravers Это является официальным драйвером Монго Java для Монго.
assylias
17

https://github.com/mongodb/mongo-java-driver/commit/d51b3648a8e1bf1a7b7886b7ceb343064c9e2225#commitcomment-3315694

11 часов назад от gareth-rees:

Предположительно, идея состоит в том, чтобы регистрировать только около 1/10 сбоев сервера (и, таким образом, избегать массового рассылки спама в журнале) без затрат на обслуживание счетчика или таймера. (Но, конечно, поддержание таймера будет доступным?)

msangel
источник
13
Не для того, чтобы придираться, но: 1/10 времени он будет возвращать res, поэтому он будет регистрировать остальные 9/10 раз.
Supericy
23
@Supericy Это точно не придирки. Это просто еще одно свидетельство ужасной практики кодирования этого человека.
Анов
7

Добавьте члена класса, инициализированный к отрицательному 1:

  private int logit = -1;

В блоке try выполните тест:

 if( !ok && (logit = (logit + 1 ) % 10)  == 0 ) { //log error

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

Если вы хотите, чтобы первая и десятая из всех ошибок, независимо от соединения, делали класс logit статическим, а не членом.

Как было отмечено, это должно быть поточно-ориентированным:

private synchronized int getLogit() {
   return (logit = (logit + 1 ) % 10);
}

В блоке try выполните тест:

 if( !ok && getLogit() == 0 ) { //log error

Примечание: я не думаю, что выбрасывать 90% ошибок - это хорошая идея.

tpdi
источник
1

Я видел подобные вещи раньше.

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

Так что иногда появлялись ранее невидимые новые «вопросы», и они появлялись в пакете, как 100 из них подряд.

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

Таким образом, решение состояло в том, чтобы регистрировать неизвестные вопросы, но, как оказалось, было тысячи разных. Журналы стали слишком большими, и их ускорение не принесло пользы, поскольку у них не было очевидных ответов. Но время от времени появлялась куча вопросов, на которые можно было бы ответить.

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

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

Таким образом, если произойдет неизвестное событие, в случайном количестве этих случаев оно будет зарегистрировано.

Я думаю, что это похоже на то, что вы видите здесь.

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

Дженс Тиммерман
источник
3
За исключением того, что мы говорим о драйвере базы данных здесь ... неправильное место проблемы, IMO!
Стивен Шланскер
@ StevenSchlansker Я никогда не говорил, что это хорошая практика. Я удалил этот кусок кода и просто записал эти сообщения в другой файл.
Дженс Тиммерман