Skocz do zawartości
  • 1

Code Review.


rozumek29

Pytanie

Witam, jeśli ktoś ma chwilę wolnego czasu, nie pogardziłbym Code Review. Proszę, ponieważ mam wrażenie że nie do końca optymalnie korzystam z bazy danych.

https://github.com/Rozumek29/StoreCraftCore

Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/
Udostępnij na innych stronach

16 odpowiedzi na to pytanie

Rekomendowane odpowiedzi

  • 0

Jeśli chodzi o bazę danych to wszystkie zapytania do bazy danych powinieneś wykonywać asynchronicznie (w osobnym wątku). Bo w Twoim kodzie wszystko wykonujesz w głównym wątku serwera, przez co przy większych zapytaniach/większej ilości zapytań serwer będzie lagował.

Edytowane przez GoblicPL
Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-316830
Udostępnij na innych stronach

  • 0

1. Powinieneś użyć tzn. connection pool, czyli połączyć się z bazą x razy (najlepiej do ustawienia w configu). I później używać połączeń naprzemiennie, zapisując je w liście. Odciąży to "jedno połączenie" i dzięki temu, jeżeli będziesz wykonywał duże zapytanie, albo pare małych to zawsze zostanie wybrane połączenie, które nie jest zablokowane dla tego zapytania. 

2. Używasz #prepareStatement, po czym nie używasz najważniejszej funkcji tej metody, czyli parametrów. W zapytaniu używasz znaków zapytania (?), każdy znak zapytania jest indeksowany od 1 w górę. Później dla każdego parametru używasz metody PreparedStatement#setString(1, uuid) analogicznie #setInt(2, kills), #setLong(3, lastLogin) itd..

3. Konwencja nazewnictwa w sql (w pytaniu jest mysql, ale dotyczy to też sqlite i wielu innych języków sql) - https://stackoverflow.com/a/62706057

4. Co do nazewnictwa to, niektóre nazwy atrybutów masz w liczbie mnogiej, a inne w pojedynczej - warto się zdecydować (Kills/BlockPlaced)

5. Dobrą rzeczą jest używanie cache. Z tego co widzę to jest w porządku całkiem zakodzony, trochę dziwnie, ale jeżeli działa to spoko. 

 

Ogólnie rzecz ujmując masz pojęcie i całkiem dobrze sobie radzisz. Jeżeli są to Twoje początki to prawdopodobnie tak wyglądałby kod osoby, która naczytała się poradników i oglądnęła jakieś kursy także jesteś na dobrej ścieżce rozwoju. Nic tylko życzyć powodzenia. 

 

ps co do 2 to warto sobie przygotować metody select/update(String zapytanie, String ... argumenty) i później w pętli setować sobie wartości dla czystej wygody

Edytowane przez LeviBoyPL
Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-316831
Udostępnij na innych stronach

  • 0
11 godzin temu, kerpson napisał:

To ja się przyczepie do tego:

if (event.getBlock() instanceof Block){

w jakim celu to jest, skoro tam nie ma innej możliwości w evencie BlockBreak

Chciałem sprawdzić czy dzięki temu zabiegowi nie będzie mi np zaliczać trawy i kwiatków. Tylko w celach testowych, nie będzie tego w finalnej wersji.

Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-316899
Udostępnij na innych stronach

  • 0
    private Connection connection = SQLiteManager.getConnection();
    private PreparedStatement ps;
    private ResultSet rs;

No to chyba to jest największym problemem i przyczyną dlaczego to nie działa, trzy z tych rzeczy automatycznie się zamykają przy try-with-resources i średnio bezpieczne jest przypisywanie jednego pola prepared statement pod 10 różnych zapytań. Lepiej w każdej metodzie osobno w parametrach try pobierać połączenie na nowo i tworzyć w nich odpowiednią zmienną prepared statement, wtedy będziesz miał pewność, że nie będzie jakichś niespodziewanych timeoutów lub zamkniętych połączeń przed ich użyciem.

Abstrahując od samego stacktrace, popełniasz ogromny błąd nie escape'ując tych zmiennych w zapytaniach, narażasz się na SQL Injection. Poniżej jest przykładowy kod który miałby to, o czym wspomniałem wyżej. 

    public void increaseDeaths(){
        this.deaths++;
        try {
            ps = connection.prepareStatement("UPDATE 'players' SET Deaths="+this.deaths+" WHERE UUID LIKE '"+this.uuid+"';");
            ps.execute();
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }

Zamienić na np.:

    public void increaseDeaths(){
        this.deaths++;
        try (Connection connection = SQLiteManager.getConnection();
             PreparedStatement statement = connection.preparedStatement("UPDATE 'players' SET Deaths=? WHERE UUID LIKE ?;") {
	    statement.setInteger(1, this.deaths);
            statement.setString(2, this.uuid.toString());
            statement.executeUpdate();
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }

Nie mam pojęcia czy to połączenie tutaj się nie wykrzaczy z tego powodu, że getConnection() zwraca w tym przypadku zainicjowane połączenie które nawet nie tyka Hikari w żaden sposób, ale na SQLite zbytnio się nie znam więc to takie gdybanie.

Moim zdaniem powinieneś na pewno odrobinę popracować nad strukturą tego "modelu". StorePlayer mógłby być zwykłym POJO i zawierać podstawowe pola oraz gettery takie jak znajdują się powyżej, nie łącząc tego w jednej klasie z pewnego rodzaju repozytorium. Zamiast inkrementować w taki sposób śmierci w jakiejś metodzie increaseDeaths(), mógłbyś zrobić sobie klasę która by obrała za argument uuid gracza i na jego podstawie zwiększać liczbę.

Edytowane przez zrdzn
Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-317017
Udostępnij na innych stronach

  • 0

Na Githubie byłą stara wersja kodu, sorki mój błąd. Oprócz tych escapów o których pisałeś cała klasa StoreCraftPlayer wygląda tak. Poleciał też commit i push na githuba, więc jeśli ci wygodniej to tam możesz spojrzeć na kod. 

Update Kodu nic nie zmienił, nadal wyskakuję błąd podany wyżej

 

https://github.com/Rozumek29/StoreCraftCore

 

Edytowane przez rozumek29
Edycja zaimportowanego codu.
Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-317019
Udostępnij na innych stronach

  • 0

Nadal nie poprawiłeś kodu według naszych instrukcji 

2. Używasz #prepareStatement, po czym nie używasz najważniejszej funkcji tej metody, czyli parametrów. W zapytaniu używasz znaków zapytania (?), każdy znak zapytania jest indeksowany od 1 w górę. Później dla każdego parametru używasz metody PreparedStatement#setString(1, uuid) analogicznie #setInt(2, kills), #setLong(3, lastLogin) itd..

Poza tym poczytaj sobie https://github.com/RainbowDashLabs/DataSourceSample#why-use-try-with-resources to powinno rozwiązać Twój problem. 

Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-317051
Udostępnij na innych stronach

  • 0
 public static Connection getConnection() throws SQLException {
     return ds.getConnection();
 }

Zamiast zwracać połączenie które może być automatycznie zamknięte, lepiej zwracać całe HikariDataSource, a połączenie pobierać za każdym razem przy zapytaniach. Prawdopodobnie to jest przyczyną błędu.

        Bukkit.getScheduler().runTaskAsynchronously(plugin, new Runnable() {
            @Override
            public void run() {

                HikariConfig config = new HikariConfig();
                config.setPoolName("StoreCraftPool");
                config.setDataSourceClassName("org.sqlite.SQLiteDataSource");
                config.setJdbcUrl("jdbc:sqlite:"+file.getAbsolutePath());
                config.setPoolName("StoreCraftDataBasePool");
                config.setMaximumPoolSize(20);
                config.setMaxLifetime(60000); // 60 Sec
                config.setIdleTimeout(30000); // 30 Sec

                ds = new HikariDataSource(config);
            }
        });

Uruchamianie tego asynchronicznie jest absolutnie niepotrzebne, instancję HikariDataSource inicjujesz tylko raz.

    static {

    }

Puste, można wywalić. Swoją drogą za dużo w tym kodzie statycznych rzeczy jest, w większości jest on źle użyty, powinieneś zdecydowanie poczytać o obiektowości, konstruktorach itp.

Edytowane przez zrdzn
Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-317102
Udostępnij na innych stronach

  • 0

ogólnie używanie sqlite jest cringe - po to masz flat/json

@edit

a jak masz ustawione sqlite na testy to radzę zrobić darmową bazę i przerzucić się na mysql 😃 

Edytowane przez Queito
Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-317115
Udostępnij na innych stronach

  • 0
W dniu 17.01.2022 o 22:48, Queito napisał:

ogólnie używanie sqlite jest cringe - po to masz flat/json

@edit

a jak masz ustawione sqlite na testy to radzę zrobić darmową bazę i przerzucić się na mysql 😃 

Właściciel lubi bardzo tego SQLite, gdyby to ode mnie zależało to już dawno cały serwer byłby na Mysql. 

Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-317229
Udostępnij na innych stronach

  • 0
W dniu 17.01.2022 o 22:48, Queito napisał:

ogólnie używanie sqlite jest cringe - po to masz flat/json

@edit

a jak masz ustawione sqlite na testy to radzę zrobić darmową bazę i przerzucić się na mysql 😃 

Po uzgodnieniu z właścicielem, udało się przejść na MySQL i wszystkie problemy znikły jak ręką odjął. Dzięki wszystkim za porady, wydaje mi się że prawię wszystkie udało mi się zrealicować.

Temat można zamknąć.

Odnośnik do komentarza
https://skript.pl/temat/51548-code-review/#findComment-317300
Udostępnij na innych stronach

Dołącz do dyskusji

Możesz dodać zawartość już teraz a zarejestrować się później. Jeśli posiadasz już konto, zaloguj się aby dodać zawartość za jego pomocą.

Nieaktywny
Odpowiedz na pytanie...

×   Wklejono zawartość z formatowaniem.   Usuń formatowanie

  Dozwolonych jest tylko 75 emoji.

×   Odnośnik został automatycznie osadzony.   Przywróć wyświetlanie jako odnośnik

×   Przywrócono poprzednią zawartość.   Wyczyść edytor

×   Nie możesz bezpośrednio wkleić grafiki. Dodaj lub załącz grafiki z adresu URL.

  • Ostatnio przeglądający   0 użytkowników

    • Brak zarejestrowanych użytkowników przeglądających tę stronę.
×
×
  • Dodaj nową pozycję...