В чем недостатки такого решения?

Question

[Nikolino]

На техническом собеседовании задали вопрос, что плохого в этом решении.

CREATE TABLE `comments`
( 
  `id` INT(11) NOT NULL AUTO_INCREMENT
  `thread_id` INT(11) NOT NULL,
  `text` VARCHAR(255) NOT NULL, 
  `votes` INT(11) NULL DEFAULT 0,
  PRIMARY KEY(`id`)   
)

public function vote(int $id)
{  
  $repository = App::get('comment'); 
  $comment = $repository->getById($id); 
  ++$comment->votes;   
  $repository->save($comment);  
}

Не смог ответить.
Собеседующий намекнул про одновременные запросы.

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

С блокировками и транзакциями на практике еще не сталкивался. Думаю, что в эту сторону нужно копать.
Как правильно решать такие задачи, когда нам что-то нужно инкрементировать?

Comments

[Uno]

На какую должность устраиваетесь?

[Adamos]

UPDATE `comments` SET `votes` = `votes` + 1 WHERE `id` =

[sim3x]

Какой фреймворк?

[Nikolino]

Noizefan zzz, php разработчик.
А junior или middle не знаю, работодатели обычно не говорят. Но часто говорят, когда требуется именно senior или "старший php разработчик".

[Nikolino]

sim3x, в коде представлен не знаю какой, Yii, полагаю, так как спросили готов ли изучить Yii.

А в laravel, например, такое делать нужно через DB::transaction?
DB::transaction(function () {
// $votes (получаем текущее votes)
DB::table('comments')->update(['votes' => ++$votes]);
});

[sim3x]

Nikolino,
https://www.yiiframework.com/doc/api/2.0/yii-db-ba...

получать текущее значение в пхп не требуется
Нужно отправить в СУБД указание увеличить каунтер

https://www.yiiframework.com/wiki/282/using-counte...

[Uno]

Nikolino, очень понимаю, сам вчера собеседование прошёл) я точно на джуна. Были простенькие задачки, вроде спроектировать бд или рекурсивно массив многомерный вывести.

Кстати, по Вашему вопросу - почему плохо так делать - код сначала скачивает голоса, потом они меняются кем либо, потом код загружает значение. Т.е. - пока он скачал и редачит, +5 голосов будет на комменте, а наш скрипт только +1 учтёт и запишет.

Update: решение у ребят ниже, vote = vote+1, лучше ничего не придумать, проверно годами)

[Developer]

А какой вопрос-то задали?

Answer 1

[Кирилл Несмеянов]

В чем недостатки такого решения?

Помимо отсутствия транзакционности в коде (о чём уже упоминали выше):

1) Нет ключа на thread_id (желательно foreign)
2) Типы в INT, вместо UNSIGNED INT
Т.к. это MySQL, судя по вопросу, то:
3) Нет указания на collation таблицы (желательно utf8mb4)
4) Нет указания на движок таблицы (транзакции есть только в InnoDB и их нет в ISAM, MyISAM, Archive, Memory, etc.)
5) Нет уникальности голосов (т.е. votes), как следствие - таблица денормализованная и подсчитывать так голоса - крайне глупо. В нормальном мире это делается отдельной таблицей с привязкой по comment_id и уникальным ключом на comment_id + user_id.
6) Так как это комментарии, то использование varchar(255) для поля text - глупо. Нужен TEXT или LONGTEXT.
7) Отсутствуют поля даты создания и редактирования, что в будущем не позволит строить статистические данные или расширять функционал (разрешать менять текст комментария только в течении 5ти минут, например). На это надо закладываться заранее.

Это что касается структуры таблицы. Теперь по коду:
8) Сервис-локация (класс App), вместо нормального DI - это жесть. Скрытые зависимости говорят о невозможности тестирования такого кода.
9) "Магическая" константа 'comment' говорит о том, что потом такой код нормально не отрефакторить. Т.е. переименовываешь внутри что-то, а потом получаешь кучу ошибок и придётся лазать по коду и везде выгребать этот "get('comment')". И не дай бог где-то они получаются как-то иначе. Работы ещё на пол дня.
10) Метод getById говорит о том, что сущность должна возвращаться всегда. Однако, может так случиться, что её по указанному id просто не найдётся. Как следствие - нет нормальной обработки ошибок.
11) Мутабельное поле votes у comment (т.е. имеется прямой доступ к нему). Это говорит о том, что сущность очень сильно связана с БД, а значит любой рефакторинг таблицы ведёт к полному перелопачиванию всего кода.
12) Нет обработки ошибок во время сохранения сущности.
13) Метод vote - не лучшее название. Оно переводится и как глагол "голосовать" и как существительное "голос", что может запутать. Я бы переименовал в addVote. Хотя и так тоже норм.

Это всё, что касается вопроса "в чём недостатки такого решения". Ответ - почти на каждой строчке косяк.

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

Comments

[Tvolly]

Столько замечаний, но среди них не заметил основного ответа (как я понял из намека от Собеседующего)
"одновременные запросы":
Если 2 и более пользователей сделают запрос одновременно, то голос может засчитаться за 1.

Необходимо что-то типа:
while ($voteService->isLocked()) {
// просто ждем когда откроется
}

$repository = App::get('comment');
$comment = $repository->getById($id);
++$comment->votes;
$repository->save($comment)

$voteService->unlock();

[Tvolly]

после цикла
while(...) {
}

$voteService->lock();

$repository = App::get('comment');
....

[Кирилл Несмеянов]

Столько замечаний, но среди них не заметил основного ответа

Он в самом первом предложении, предлагаю прочитать его =)

Необходимо что-то типа: while ($voteService->isLocked()) {

Отличное решение! Вместо того, чтобы просто обновить значение в одном запросе - писать while, который может просто грохнуть в 504 весь сайт. Можно ещё добавить sleep на всякий случай, чтобы наверняка! Ведь каждому известно, что медленные сайты производят впечателние того, что он очень крутой и там много всякого кода внутри.

Answer 2

[Дмитрий]

Вообще если идет голосование неплохо бы определять ip голосовавшего, если он не авторизован или user_id, иначе потеря одного голоса будет мелочью по сравнению с тем, что один человек может голосовать несчетное кол-во раз. И если надо запомнить голосовавшего, значит нужна отдельная таблица с полями voter и comment_id

Comments

[Nikolino]

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

[Дмитрий]

Nikolino, Вы не знаете что нужно было ответить. Не надо так лебезить перед каждым интервьюером. Кого волнует что он имел ввиду и что от вас ждал. Это не должно быть вашей заботой. Если вам есть чему научиться - просто научитесь, нет никакого "нужно".

[Nikolino]

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

[Дмитрий]

Nikolino, Слово "лебезил" неудачное, прошу прощения.