[R.....y]

#programowanie #cpp
mogę prosić kogoś o code review?
https://github.com/redve-dev/Steam-market-monitor

[R.....y]

@RedveKoronny: jakby ktoś próbował odpalić, to jednak program defaultowo nie szuka pliku 'items' tylko w--------a segmentation fault

[R.....y]

@RedveKoronny: dobra, naprawione

[zrakiep]

@RedveKoronny: na szybko to Update nie powinno pisać na ekran. Powinieneś mieć oddzielnie logikę gdzie pobierasz dane i je przetwarzasz i oddzielnie logikę gdzie z nią coś robisz - czy to na ekran czy może do .csv by se w excelu wykresy porobić. Mapa walut też niepotrzebna - jak masz 3 wartości to switch w konstruktorze i zapisanie sobie stringa nazwy waluty - jako bonus jak walute pomylisz to wywali

[zrakiep]

@RedveKoronny: o, i jeszcze nie ma potrzeby by wszystko miało swój katalog - tj te Itemy i InputData. Moze luze w src leżeć, łatwiej przeglądać będzie

[Whiskeyjack29]

@zrakiep: to mnie zawsze zastanawiało, w cpp wszystko trzymają w src luzem, ewentualnie 1 katalog wgłąb na sub-moduł/bibliotekę potrzebną w projekcie. W innych javach czy c# tysiące katalogów ( ͡° ͜ʖ ͡°)

[M.....f]

@RedveKoronny:

1. Używasz std::map https://www.wykop.pl/wpis/66670015/z-czego-wynika-popularnosc-std-map-aka-drzewo-czer/
2. Pola w klasie nie powinny być "const"
3. items.pushback(Item(name, currency)); można zrobić emplaceback
4. Nie wiem czy to jest zamierzone, ale pola w InputData są publiczne.
5. Używasz std::endl

[R.....y]

Mapa walut też niepotrzebna - jak masz 3 wartości to switch w konstruktorze i zapisanie sobie stringa nazwy waluty

@zrakiep: jest po to żeby móc bardzo łatwo dodać kolejne waluty w przyszłości, i żeby zachować zgodność z dokumentacją steama

Te generowanie linka też może być w konstruktorze, nic się tam nie dzieje takiego by potrzebna była oddzielna

[M.....f]

@RedveKoronny:
6. bool IsNum jest const
7. Nie ma testów
8. Nie ma namespaców
9. Funktory śmierdzą
10. Mam wrażenie, że jest za dużo includów

[R.....y]

1. Używasz std::map

@Malkof: wyjaśniłem komentarz wyżej, dodatkowo to nie jest etap programu gdzie wydajność jest kluczowa

2. Pola w klasie nie powinny być

[R.....y]

@RedveKoronny: 10. wywaliłem tylko iostream z main.cpp które zostało po debugowaniu

[R.....y]

o, i jeszcze nie ma potrzeby by wszystko miało swój katalog - tj te Itemy i InputData. Moze luze w src leżeć, łatwiej przeglądać będzie

@zrakiep: zrobiłem sobie skrypt który tak mi tworzy klasy. Wiem że zadziała i bez tego, ale moim zdaniem jest sporo czyściej

[M.....f]

@RedveKoronny:

dodatkowo to nie jest etap programu gdzie wydajność jest kluczowa

[M.....f]

@RedveKoronny:

11. znaleść

[R.....y]

@Malkof:

1. Używasz std::map

różnica w wydajności tutaj będzie

[MamCieNaHita]

@RedveKoronny: na szybko, bo kto robi dobrze
1. ten statyczny std map najlepiej zamienić na free function ze switch casem, a jak tak bardzo się upierasz, to zrób go globalnego(statici w funkcji zawsze sprawdzają czy są zainicjalizowane z każdym zawołaniem funkcji)
2. curl request nie używa raii
3. te wszelkie predykaty powinny być free function
4. hextochar też, i te strumienie są okropnie wolne
5. to zastępowanie reserved

[M.....f]

@RedveKoronny:

różnica w wydajności tutaj będzie marginalna

Tak jak wszystko w twoim programie, ale to pokazuje twoje niechlujne

[maisterwihaister]

@RedveKoronny: explicit nic nie robi kiedy konstruktor ma więcej niż 1 parametr

[M.....f]

@maisterwihaister: Oczywiście, że robi.

[maisterwihaister]

@Malkof: rzeczywiście, zapobiega też copy-list-initialization. Tu przykład jakby ktoś jeszcze nie wiedział: https://en.cppreference.com/w/cpp/language/explicit

[j.....0]

mogę prosić kogoś o code review?

@RedveKoronny: widzę lubisz krytykę, tą sensowną i tą niezbyt skoro tutaj piszesz xD
Ale proszę bardzo:

1) Skoro i tak już wciągasz bibliotekę do parsowania json'ów to jest jakieś uzasadnienie, aby ta konfiguracja (plik items) była w takiej dziwnej formie? Nie dałoby się z tego zrobić