Wątek przeniesiony 2018-07-17 08:13 z Newbie przez kq.

Prośba o code review

Odpowiedz Nowy wątek
2018-07-17 10:13

Rejestracja: 1 rok temu

Ostatnio: 3 tygodnie temu

0

Witam. Mógłby ktoś spojrzeć na mój kod? Wszelkie uwagi dotyczące zarówno działania, jak i stylu pisania kodu mile widziane:). W klasie SQLConnection mam zakomentowany pewien fragment, ponieważ w takiej formie nie działał. Zapytanie wykonywało się tylko wtedy, gdy zamiast '?' wpisywałam konkretną nazwę kolumny. Jeśli ktoś z Was będzie miał pomysł dlaczego nie działa to również prosiłabym o pomoc.
https://github.com/quark001/Calendar

Z gory serdecznie dziękuję.

edytowany 1x, ostatnio: quark001, 2018-07-17 10:13
Przydałby się kod. - kq 2018-07-17 10:13
@kq jest link do github - MarekR22 2018-07-17 11:16
W momencie pisania komentarza nie było. - kq 2018-07-17 11:16
@kq jest po prostu zbyt szybki:). Nie mogłam wstawić linka w pierwszym poście, musiałam go edytować, co zajęło mi dosłownie jakieś 5 sekund. - quark001 2018-07-17 17:46

Pozostało 580 znaków

2018-07-17 11:13

Rejestracja: 12 lat temu

Ostatnio: 45 sekund temu

5
  • using namespace std; w nagłówku!
  • Płaska organizacja projektu (wszystko w głównym katalogu).
  • brak pliku projektu! jakiś cmake, make lub przynajmniej coś charakterystycznego dla IDE jakiego używasz.
  • if (cos) return true; else return false zamiast return cos;.
  • komentarze przy prywatnych metodach a brak komentarzy przy publicznych (co mnie obchodzą prywatne metody, gdy używam klasy)
  • brak użycia/wykorzystania forward declaration, np w SQLConnection.h powinno być tak:
#ifndef CALENDAR2_SQLCONNECTION_H
#define CALENDAR2_SQLCONNECTION_H

#include <string>

// do przeniesienia do pliku cpp
// #include <cppconn/driver.h>
// #include <cppconn/connection.h>
// #include <cppconn/resultset.h>
// #include <cppconn/statement.h>
// #include <cppconn/exception.h>

#include <vector>
// #include "SQLConnection.h" - plik załącza sam siebie !!!

// do przniesienia do cpp
// #include "Activity.h"
// #include "CalendarEntry.h"

// forward declarations
namesapce sql {
    class Driver;
    class Connection;
    class Statement;
    class ResultSet;
    class PreparedStatement;
}

class Activity;
class CalendarEntry;

class SQLConnection
{
public:
    SQLConnection ();
    ~SQLConnection ();
    void connect ();
    void closeConnection ();
    void readAllActivitiesToVector ( std::vector < Activity > & );
    void readCalendarEntriesToVector ( std::vector < CalendarEntry > &, string );
    void deleteActivity ( Activity & );
    void addActivity ( Activity & );
    void addCalendarEntry ( CalendarEntry & );
    void updateCalendarEntry ( CalendarEntry &, int );

private:
    sql::Driver *driverPtr;
    sql::Connection *connectionPtr;
    sql::Statement *statementPtr;
    sql::ResultSet *resultSetPtr;
    sql::PreparedStatement *preparedStatementPtr;
};

#endif //CALENDAR2_SQLCONNECTION_H
  • surowe wskaźniki to anachronizm, używaj std::unique_ptr lub std::shared_ptr
  • własna kulawa implementacja czasu MTime oraz Date- używaj std::chrono lub ctime, względnie użyj MTime do opakowania jednego z tych rozwiązań.
  • jest jeszcze parę rzeczy, ale na razie sobie daruję

Żeby nie tylko zrzędzić, chwali się, że

  • jesteś konsekwentny w formatowaniu kodu
  • piszesz małe funkcje
  • jak na początkującego to całkiem nieźle

Jeśli chcesz pomocy, NIE pisz na priva, ale zadaj dobre pytanie na forum.
edytowany 8x, ostatnio: MarekR22, 2018-07-17 16:40
@MarekR22: Bardzo dziękuję za uwagi. Wiem, że z nicka i awataru to nie wynika, ale jestem kobietą, więc mogę być co najwyżej "konsekwentnA" :) - quark001 2018-07-17 17:50

Pozostało 580 znaków

2018-07-17 18:01

Rejestracja: 1 rok temu

Ostatnio: 1 rok temu

1
void Menu::help ()
{
    cout
          << "**********\n";
    cout
          << "***HELP***\n";
    cout
          << "**********\n";
    cout
          << "ENTER:\n";
    cout
          << "0 - to quit\n";
    cout
          << "1 - for help\n";
    cout
          << "2 - to mark calendar entry as done\n";
    cout
          << "3 - to add a new calendar entry\n";
    cout
          << "4 - to print calendar entries\n";
    cout
          << "5 - to add a new activity\n";
    cout
          << "6 - to remove a activity\n";
    cout
          << "7 - to print activities\n"
          << endl;

}

Można to zrobić tak:

void Menu::help()
{
    std::cout <<
    "**********\n"
    "***HELP***\n"
    "**********\n"
    "ENTER:\n"
    "0 - to quit\n"
    "1 - for help\n"
    "2 - to mark calendar entry as done\n"
    "3 - to add a new calendar entry\n"
    "4 - to print calendar entries\n"
    "5 - to add a new activity\n"
    "6 - to remove a activity\n"
    "7 - to print activities\n\n";
}
Albo nawet raw string literala można użyć ;​) - kq 2018-07-17 21:09

Pozostało 580 znaków

2018-07-18 06:48

Rejestracja: 6 lat temu

Ostatnio: 7 godzin temu

0

Poniższe hardkodowanie poświadczeń do bazy danych też nie jest super:
https://github.com/quark001/C[...]blob/master/SQLConnection.cpp

Najlepiej wrzucić to w jakiś plik konfiguracji, z którego będziesz odczytywać. To jest aplikacja desktopowa więc każdy klient będzie miał u siebie, na komputerze jawne dane odnośnie logowania do bazy, w dodatku jako root. Taki plik należałoby wiec odpowiednio zabezpieczyć vide zaszyfrować i w ogóle łączyć się z bazą na dedykowanym użytkowniku, a nie na koncie root'a.

Pozostało 580 znaków

Odpowiedz

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0