JS Система домов (без системы гаража) (Js, Njs, Vue 2)

lazzarevv

Junior Developer
Скриптер
Приветствую.
Сделал систему домов с возможностью:
- Покупки / продажи
- Открытия / закрытия дверей дома


p.s Кнопки помимо выше упомянутых не функциональнs и сделаны с заделом на будущее.
 

Lev Angel

Developer
Команда форума
Скриптер
Красава Спасибо что поделился. Продолжение будет?)
 

Lev Angel

Developer
Команда форума
Скриптер
Не против если я сделаю ревью кода? Может смогу тебе что-то полезное подсказать)))
 

Lev Angel

Developer
Команда форума
Скриптер
Короче глянул только серверную часть. Больше не успел.

Ниже напишу свое ИМХО. Главное не воспринимай как личную критику, код ревью не про это. Оно про то, чтобы научиться чему-то новому.

Сразу респект что есть дамп таблицы + какие-то демо данные (y)

Теперь по коду.

1. На серверной стороне не нужен файл packages/index.js. Сервер его даже не читает. Это на клиентке он нужен.

2. Вот к этому ивенту два вопроса
JavaScript:
mp.events.add('playerReady', (player) => {
    player.position = new mp.Vector3(-1189.982, 291.911, 69.897)
    DB.query('SELECT id FROM houses', [], (err, res) => {
        if (err) return console.log(err)
        for (let i = 0; i < res.length; i++) {
            ids.push(res[i].id)
        }
        player.call('loadHousesObjects::CLIENT', [houses])
        player.call('loadInHouseObjects::CLIENT', [houses, ids])
    })
})
2.1 Если мы уже при загрузке сервера выгрузили все дома включая их id в houses, то зачем нам еще раз делать выгрузку при подключении игрока? Ид мы можем из массива получить в любой момент.
2.2 В конце отправляются два события на клиент. Я бы объединил их в одно, тем более что там одинаковые данные отправляются. Мне кажется лучше на клиенте (если есть необходимость) в ивенте вызвать две разные функции, чем делать два разных ивента. Так будут меньше накладные расходы для сервера.

3. Вот такая конструкция
JavaScript:
DB.query('SELECT * FROM ...', [id], (err, res) => {
    if (err) {
        console.log(err)
    } else {
        // КОД      
        // КОД  
        // КОД
        // КОД
        // КОД
        // КОД
    }
}
Есть такая тема. Называется ранний возврат из функции. Она позволяет улучшить читаемость кода. Плюс когда у тебя в кода много if else и они вложенные друг в друга, то потом становится сложно понимать такой код. У тебя в принципе простой код и это не вызывает проблем, но я думаю может быть такая идея полезна на будущее :) Короче лучше делать вот так.
JavaScript:
DB.query('SELECT * FROM ...', [id], (err, res) => {
    if (err) return console.log(err)

    // КОД      
    // КОД  
    // КОД
    // КОД
    // КОД
    // КОД
}

4. Тут же в sendHouseInfo::SERVER у тебя встречается res[0]. Во первых не проверяешь есть ли у тебя там что-то, ведь запрос может вернуть и пустой результат. Во-вторых дальше она во многих местах юзается. Не очень информативно называется и всякие эти магические числа тягать за собой))) Можно создать локальную переменную с норм именем. На этом нет смысла экономить, если оно заметно улучшит читаемость.

5. При любых изменениях ты по новой вытаскиваешь данные из базы о доме. По сути у тебя любые взаимодействия крутятся вокруг бд. Я бы тут советовал ориентироваться больше на данные в памяти. Мы же при загрузке сервера все дома выгрузили в houses. Можем работать теперь только с этим массивом. Продали дом - нашли в массиве - поменяли статус.
В бд тоже конечно нужно сохранять изменения. Но у тебя будет вместо SELECT + UPDATE, только UPDATE. Экономия на лицо))

6. В enterHouse::SERVER там у тебя switch и в каждом case по сути одинаковый код. Отличается только housesPos[Х]. Такое лучше выносить в отдельную функцию. Во первых чем меньше кода тем лучше. Во вторых меньше вероятность ошибиться, когда будешь там что-то менять (то ли дело в одном месте поменять, то ли в трех).
JavaScript:
function playerEnterHouse(player, houseCoords){
    player.position = new mp.Vector3(houseCoords.x, houseCoords.y, houseCoords.z)
    player.dimension = id + 10
    player.call('houseMenuToggle::CLIENT', [false])
    player.call('chooseGarageToggle::CLIENT', [false])
    player.call('unbindEkey::CLIENT')  
}

switch (res[0].class) {
    case 'high' :
        playerEnterHouse(player, housesPos[0][0]);
        break;
   
    case 'medium' :
        playerEnterHouse(player, housesPos[1][0]);
        break;
   
    case 'low' :
        playerEnterHouse(player, housesPos[2][0]);
        break;
}
 

lazzarevv

Junior Developer
Скриптер
Короче глянул только серверную часть. Больше не успел.

Ниже напишу свое ИМХО. Главное не воспринимай как личную критику, код ревью не про это. Оно про то, чтобы научиться чему-то новому.

Сразу респект что есть дамп таблицы + какие-то демо данные (y)

Теперь по коду.

1. На серверной стороне не нужен файл packages/index.js. Сервер его даже не читает. Это на клиентке он нужен.

2. Вот к этому ивенту два вопроса
JavaScript:
mp.events.add('playerReady', (player) => {
    player.position = new mp.Vector3(-1189.982, 291.911, 69.897)
    DB.query('SELECT id FROM houses', [], (err, res) => {
        if (err) return console.log(err)
        for (let i = 0; i < res.length; i++) {
            ids.push(res[i].id)
        }
        player.call('loadHousesObjects::CLIENT', [houses])
        player.call('loadInHouseObjects::CLIENT', [houses, ids])
    })
})
2.1 Если мы уже при загрузке сервера выгрузили все дома включая их id в houses, то зачем нам еще раз делать выгрузку при подключении игрока? Ид мы можем из массива получить в любой момент.
2.2 В конце отправляются два события на клиент. Я бы объединил их в одно, тем более что там одинаковые данные отправляются. Мне кажется лучше на клиенте (если есть необходимость) в ивенте вызвать две разные функции, чем делать два разных ивента. Так будут меньше накладные расходы для сервера.

3. Вот такая конструкция
JavaScript:
DB.query('SELECT * FROM ...', [id], (err, res) => {
    if (err) {
        console.log(err)
    } else {
        // КОД     
        // КОД 
        // КОД
        // КОД
        // КОД
        // КОД
    }
}
Есть такая тема. Называется ранний возврат из функции. Она позволяет улучшить читаемость кода. Плюс когда у тебя в кода много if else и они вложенные друг в друга, то потом становится сложно понимать такой код. У тебя в принципе простой код и это не вызывает проблем, но я думаю может быть такая идея полезна на будущее :) Короче лучше делать вот так.
JavaScript:
DB.query('SELECT * FROM ...', [id], (err, res) => {
    if (err) return console.log(err)

    // КОД     
    // КОД 
    // КОД
    // КОД
    // КОД
    // КОД
}

4. Тут же в sendHouseInfo::SERVER у тебя встречается res[0]. Во первых не проверяешь есть ли у тебя там что-то, ведь запрос может вернуть и пустой результат. Во-вторых дальше она во многих местах юзается. Не очень информативно называется и всякие эти магические числа тягать за собой))) Можно создать локальную переменную с норм именем. На этом нет смысла экономить, если оно заметно улучшит читаемость.

5. При любых изменениях ты по новой вытаскиваешь данные из базы о доме. По сути у тебя любые взаимодействия крутятся вокруг бд. Я бы тут советовал ориентироваться больше на данные в памяти. Мы же при загрузке сервера все дома выгрузили в houses. Можем работать теперь только с этим массивом. Продали дом - нашли в массиве - поменяли статус.
В бд тоже конечно нужно сохранять изменения. Но у тебя будет вместо SELECT + UPDATE, только UPDATE. Экономия на лицо))

6. В enterHouse::SERVER там у тебя switch и в каждом case по сути одинаковый код. Отличается только housesPos[Х]. Такое лучше выносить в отдельную функцию. Во первых чем меньше кода тем лучше. Во вторых меньше вероятность ошибиться, когда будешь там что-то менять (то ли дело в одном месте поменять, то ли в трех).
JavaScript:
function playerEnterHouse(player, houseCoords){
    player.position = new mp.Vector3(houseCoords.x, houseCoords.y, houseCoords.z)
    player.dimension = id + 10
    player.call('houseMenuToggle::CLIENT', [false])
    player.call('chooseGarageToggle::CLIENT', [false])
    player.call('unbindEkey::CLIENT') 
}

switch (res[0].class) {
    case 'high' :
        playerEnterHouse(player, housesPos[0][0]);
        break;
  
    case 'medium' :
        playerEnterHouse(player, housesPos[1][0]);
        break;
  
    case 'low' :
        playerEnterHouse(player, housesPos[2][0]);
        break;
}
Огромное спасибо. В дальнейшем буду учитывать твои замечания
 
Яндекс.Метрика
Верх