jam231 / sia

Stock market server (part of stock market simulation system).
1 stars 0 forks source link

Niezgodność przy obsługiwaniu wyszukiwania najlepszego kupna i najlepszej sprzedaży #22

Closed kaiks closed 11 years ago

kaiks commented 11 years ago

w changeCached... mamy kod

    if(query.first())
    {
        if(query.value(0).isValid() && query.value(1).isValid())
        {
            m_cachedBestSellOrders.insert(stockId,
                                      BestOrder(OrderType::BUY, stockId,
                                                query.value(0).toInt(),
                                               query.value(1).toInt()));
        }
        else
            qDebug() << "[Market] W changeCachedBestBuyOrders"
                     << "zwrócony rekord nie ma dwóch pol.";
    }
    // To znaczy, ze dla danej transakcji nie ma w ogole oferty...
    else
    {
        m_cachedBestSellOrders.remove(stockId);
    }

natomiast to nie jest prawda, że jeśli nie mamy ofert, to nic się nie zwraca

CREATE OR REPLACE FUNCTION najlepsze_kupno(in zasob integer, out integer, out bigint)
    AS $$ SELECT limit1,SUM(ilosc) FROM (SELECT limit1,ilosc,id_zasobu from zlecenie_kupna UNION SELECT 0,0,zasob) t WHERE id_zasobu=zasob GROUP BY limit1 ORDER BY 2 DESC LIMIT 1 $$
LANGUAGE SQL;

zwraca się krotka _,0 (tj. ilość = 0) moja motywacja do tego była taka żeby obsługiwać poprawnie zapytanie proszące o aktualnie najlepsze zlecenie kupna i sprzedaży dla danej akcji (którego chyba jeszcze nie ma). Można to rozwiązać inaczej i jeśli tak chcecie to dla mnie to OK, ale według mnie brak przesyłania jakiegokolwiek potwierdzenia w takim przypadku to nie jest informacja.

zwracam jednocześnie uwagę, że w bestbuyorders jest błąd:

            m_cachedBestSellOrders.insert(...);
        }
        else
           ...
    }
    else
    {
        m_cachedBestBuyOrders.remove(stockId);
     }
jam231 commented 11 years ago

To jest takie inteligetne, że zachowuje sie tak jak ja myslałem, że powinno i tak jak Ty chcesz ;-) Mógłbyś być bardziej precyzyjny i powiedzieć, w którym miejscu jest błąd ?

kaiks commented 11 years ago

Dobra, niech będzie, wywalę te krotki zerowe. Należy jednak pamiętać o odpowiednim sprawdzeniu przy pakiecie informacji o akcji

Błąd jest taki że wstawiasz do SellOrders a wywalasz z BuyOrders. Jak się domyślam w funkcji BestBuyOrders nie interesuje nas hash SellOrders.

jam231 commented 11 years ago

Hah rzeczywiście ^^ Zgadzam się, że powinna być coś wysyłany w przypadku braku transakcji. Proponuje zrobić pakiet NO_ORDER, który dodatkowo mógłby służyć do zaznaczania, że użytkownik dostał już wszystkie swoje zlecenia, zaraz sprawdze czy Marek jakoś sensowniej to zrobił. Mam zrobić merge'a najnowszych dodatków Marka ?

kaiks commented 11 years ago

Marek to zrobił w jednym pakiecie i ja się do tego dostosowałem. (więc nie trzeba no_order)

A co do merge to nie wiem. Niby możesz zrobić, mam tam wątpliwość czy w ilości akcji/zleceń powinien być int, ale to chyba nie jest nic wielkiego. No i wydaje mi się że tam jest wysyłany jakiś spory zbędny pakiet (wysłałem to jako komentarz do jednego z jego commitów) ale nie jestem tego pewien, Marek się do tego nie odniósł

Testowałem to i mam na swoim agentowym repozytorium testera klient_tester który chyba pokazuje że wszystko działa raczej OK poza tym nadmiarowym pakietem.

marimarek commented 11 years ago

Podlinkuj jak możesz o który komentarz Ci chodzi? A o co z tym int'em chodzi?

kaiks commented 11 years ago

Tu: https://github.com/jam231/sia2013/commit/995bfbf601be9b3fe6512853b7b39e75687dbd22#commitcomment-3334219 jest link wklejony w tym komentarzu

a z intem to że niby rozrzutnie, ale to tak naprawdę nie ma znaczenia już obecnie, jak doszedłem do wniosku ze komunikacja sieciowa nie będzie nigdy wąskim gardłem tutaj

marimarek commented 11 years ago

Sprwdzilem to i dodalem/zmienilem troche printow. Teraz jets wiecej informacji, miedzy innymi wiemy dokladnie jaki pakiet wysyla za kazdym razem klasa connection. I wyglada z logow serwera jakby to jednak u Ciebie bylo cos nie tak. Popatrz na to.

kaiks commented 11 years ago

no dobra, przeanalizowałem ten kod

błąd jest taki, że wysyłasz długość jako qint32, a konwencja jest qint16

błąd leży też po stronie @jam231 bo niekonsekwentnie zrobił length jako qint32 zamiast qint16 i potem castował zamiast zrobić od razu 16.

ja jestem za tym żebyś to (tj. funkcję length) zmienił wszędzie na qint16

jam231 commented 11 years ago

@kaiks Nie leży po mojej stronie tylko jest wina waszego wybiórczego przeglądania kodu. Nie ja zrobiłem funkcje length, a Marek. Nie zastanawiało was dlaczego we wszystkich pakietach do tej pory nie używałem funkcji length przy wysyłaniu pakietów ? W wielu miejscach jest nawet slynne "return -1"...

Pokaz mi proszę miejsce w kodzie gdzie castuje length do int16 ?

kaiks commented 11 years ago

@jam231 wybacz, nie wiedziałem, że nie Ty robiłeś funkcje length ;) ale skoro już była to dlaczego jej nie używałeś tylko wyliczałeś z palca?

co do kodu, myślałem że to:

qint32 ListOfStocksMsg::length() const
{
    //8 bajtow na typ + dlugosc + 4 bajty na ilosc akcji
    return 8 + 4 + m_stocks.size() * sizeof(Stock);
}

void ListOfStocksMsg::send(QIODevice* connection)
{
    QDataStream tmpStream(connection);
    tmpStream<<length();
    tmpStream<<static_cast<qint32>(type());
    tmpStream<<m_stocks;
}

było Twoim dziełem

jam231 commented 11 years ago

Liczyłem z palca bo nie miałem pomysłu na inteligetne liczenie długości w bajtach QStringa (tak, zeby się dwa razy nie liczylo) Zdaje sobie sprawe, że to bardzo brzydko i należy to zmienić, ale jakoś nie znalazłem czasu na dogranie tego.

W każdym razie zmieniłem typ zwracany funkcji length na qint16, zeby nikomu glupie pomyslu nie przychodzili do glowy ;-p

A to powyżej to jest legacy code Marka, który zreszta chyba zapomniał o tym implementując GetMyStock i GetMyOrders.

kaiks commented 11 years ago

OK. Raz jeszcze przepraszam za nieporozumienie :D moja bardzo wielka wina