Рефакторинг кода и исправление багов?

Question

[ramazan22]

Необходимо провести рефакторинг, исправить баги, указать уязвимиости и продокументировать в стиле PHPDoc.

Пока вижу, что подключение к базе идет в цикле (что, понятно, неверно) и закрывается соединение с базой перед ретурном. Мне кажется, надо после (не прав?).

function load_users_data($user_ids) {
    $user_ids = explode(',', $user_ids);
    foreach ($user_ids as $user_id) {
        $db = mysqli_connect("localhost", "root", "123123", "database");
        $sql = mysqli_query($db, "SELECT * FROM users WHERE id=$user_id");
        while($obj = $sql->fetch_object()){
            $data[$user_id] = $obj->name;
        }
        mysqli_close($db);
    }
    return $data;
}
// Как правило, в $_GET['user_ids'] должна приходить строка
// с номерами пользователей через запятую, например: 1,2,17,48
$data = load_users_data($_GET['user_ids']);
foreach ($data as $user_id=>$name) {
    echo "<a href=\"/show_user.php?id=$user_id\">$name</a>";
}

Comments

[Максим Тимофеев]

кто-то тестовое задание дал?

[ramazan22]

Максим Тимофеев, конечно. да

[Максим Тимофеев]

ramazan22, а смысл тогда тут спрашивать? Одно дело Вы сделали и показываете типа "что я упустил", другое если за Вас сделают. Вы же дальше провалитесь. Пустить пыли в глаза и немного поднять свой скил выше реального можно, но всему же есть предел?
Такие задания показывают не только заказчику, но и Вам в первую очередь, готовы и Вы это выполнять.
Так что Вы задание сделайте, а потом сюда кидайте, что бы помогли дошлифовать.

Answer 1

[Lander]

function load_users_data(Array $user_ids) 
{
    $data = [];
    
    $user_ids = array_filter($user_ids, function($id) { 
        return is_numeric($id); 
    });
    
    if ($db = mysqli_connect("localhost", "root", "123123", "database")) {
        $sql = "SELECT * FROM `users` WHERE id IN (" . implode(',', $user_ids) . ")";
        $query = mysqli_query($db, $sql);
    
        while($obj = $query->fetch_object()) {
            $data[$obj->id] = $obj->name;
        }

        mysqli_close($db);
    } else {
        throw new Exception('Ошибка соединения с базой данных.');
    }
    return $data;
}

$data = load_users_data(explode(',', $_GET['user_ids']));
foreach ($data as $user_id=>$name) {
    echo "<a href=\"/show_user.php?id=$user_id\">$name</a>";
}

В самом первом приближении лучше сделать вот так

Comments

[Lander]

Беда только в том, что если вы ни строчки не поймёте из этого кода, то я оказал вам медвежью услугу. Согласен с Максим Тимофеев, делать нужно самому. Просто я две недели на больничном и не смог удержаться. :)

[ramazan22]

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

Вам большое спасибо за подсказку

[Lander]

ramazan22,

Просто, как по мне, уж лучше приложить хоть какие-то усилия, чем просто сказать - я не знаю. Скажу - я не знал, но спросил

Возможно вы и правы.

[Danbka]

Я бы немного улучшил code-style вот этого куска:

$db = mysqli_connect("localhost", "root", "123123", "database");

if (!$db) {
	throw new Exception('Ошибка соединения с базой данных.');
}

$sql = "SELECT * FROM `users` WHERE id IN (" . implode(',', $user_ids) . ")";
$query = mysqli_query($db, $sql);

while($obj = $query->fetch_object()) {
	$data[$obj->id] = $obj->name;
}

mysqli_close($db);

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

Lander, я бы лучше вот так переписал это:

function load_users_data(\mysqli $connection, int $userIds): iterable
{
    $sql = \sprintf('SELECT * FROM `users` WHERE id IN (%s)', \implode(',', userIds));
    $query = \mysqli_query($db, $sql);
    
    while($obj = $query->fetch_object()) {
        yield $obj->id => $obj->name;
    }
}

1) Ибо соединение с БД - это сайдэффект функции
2) Лишний тайпкастинг через фильтрацию можно решить нативными средствами
3) Запрос без конкатенации читаемее.
4) Зачем набивать массив, если можно просто вернуть итератор?

М?