Вот мой проект с открытым исходным кодом: https://github.com/Skewjo/SysLat_Software

Сегодня я хотел бы попытаться очистить мой класс «SysLatData».

Заголовочный файл здесь: https://github.com/Skewjo/SysLat_Software/blob/master/SysLatData.h

Реализация здесь: https://github.com/Skewjo/SysLat_Software/blob/master/SysLatData.cpp

Определение и использование здесь: https://github.com/Skewjo/SysLat_Software/blob/master/SysLat_SoftwareDlg.cpp

Я думаю, что первая пара вещей, которые мне нужно атаковать (после очистки typedef и моих включенных охранников):

• Перенос некоторых функций из этого класса в основную программу (в частности, UpdateSLD()) и создание сеттеров.
• Перемещение функций экспорта в новую статическую универсальную библиотеку (в частности, CreateJSONSLD() и ExportData())
• Перенос несвязанных переменных в основную программу (например, m_targetApp, m_RTSSVersion, и m_boxAnchor)

Но я действительно не знаю, что мне делать, и поэтому я здесь!

#pragma once
#ifndef SYSLATDATA_H
#define SYSLATDATA_H

//#define MAX_TESTS                                     500
#define MOVING_AVERAGE                                  100
#define EVR_MIN                                         3
#define EVR_MAX                                         100

//putting all of this in a struct has got to be unbelievably stupid
typedef struct SYSLAT_DATA {
    vector<int>         m_allResults;
    vector<string>      m_v_strRTSSWindow;
    vector<string>      m_v_strActiveWindow;
    int                 m_counter = 0;
    int                 m_systemLatencyTotal = 0;
    double              m_systemLatencyAverage = 0;
    int                 m_counterEVR = 0;
    int                 m_systemLatencyTotalEVR = 0; //EVR stands for expected value range
    double              m_systemLatencyAverageEVR = 0;
    int                 m_a_MovingAverage[MOVING_AVERAGE] = { 0 };//if I end up using a "total tests" var, then this should probably just be split into 2 pointers pointing at the "head" of the totalTests var and head-100 or something...
    int                 m_systemLatencyMedian = 0;
    int                 m_systemLatencyMedianEVR = 0;
    int                 m_systemLatencyMax = 0;
    int                 m_systemLatencyMin = 0;
    int                 m_systemLatencyMaxEVR = 0;
    int                 m_systemLatencyMinEVR = 0;
}SYSLAT_DATA;

class CSysLatData
{
protected: 
    SYSLAT_DATA         sld;
    CString             m_strSysLatResultsComplete;
    HANDLE              m_Mutex = NULL;
    CString             m_strError;
    time_t              m_startTime, m_endTime;
    char                m_startDate[26], m_endDate[26];

public:
    CSysLatData(); //this constructor only opens a mutex... does the destructor need to close the mutex? Also, do I need to init the struct in the constructor? It should init to all 0's off the bat...
    
    Json::Value         jsonSLD; //this is basically a second copy of the data... will probably eat up a BOATLOAD of memory for no reason. There's got to be a better way...
    
    //using getters and setters for all of these seems stupid...
    int                 GetCounter();
    int                 GetTotal();
    double              GetAverage();
    int                 GetCounterEVR();
    int                 GetTotalEVR();
    double              GetAverageEVR();
    CString             GetStringResult();

    int                 GetMedian();
    int                 GetMedianEVR();
    int                 GetMax();
    int                 GetMin();
    int                 GetMaxEVR();
    int                 GetMinEVR();
    //int*              GetMovingAverage(); //????

    string              m_targetApp = "Unknown";
    string              m_RTSSVersion = "0.0.0";
    string              m_boxAnchor = "Unknown";
    struct tm*          startTimeUTC;
    double              m_testDuration;

    //double            CalculateMovingAverage(); //this function would be for calculating it from scratch...
    //double            UpdateMovingAverage(); //this function would be used if I'm updating the moving average every time I get a new value
    void                SetEndTime();
    
    void                UpdateSLD(unsigned int loopCounter, const CString& sysLatResults, string RTSSWindow, string activeWindow, DWORD fgPID, DWORD rtssPID);
    
    //mutex functions
    void                CheckSLDMutex();
    BOOL                AcquireSLDMutex();
    void                ReleaseSLDMutex();
    void                CloseSLDMutex();

    void                AppendError(const CString& error);

    // I think I need to make the following 2 functions return BOOLs or INTs based on whether or not they failed.
    void                CreateJSONSLD();
    void                ExportData(int testNumber, string path = ".\SysLat_Logs", int totalLogs = 10000);

    bool                dataExported = false;
    bool                dataUploaded = false;
};
#endif

Мы очень ценим любую помощь, которую вы можете оказать.

Избегайте макросов

Вы должны объявить константы как constexpr переменные вместо как #defines, вот так:

constexpr std::size_t MOVING_AVERAGE = 100;

Использовать std::size_t для размеров, количества и индексов массива

An int может быть недостаточно большим для представления всех возможных размеров массивов, а отрицательные значения также не имеют смысла для размеров и количества. Правильный тип — std::size_t.

Если вы не имеете дело с массивами или счетчиками циклов, а просто нуждаетесь в переменной для подсчета событий, вы также можете использовать std::size_t для этого вместо int, хотя было бы лучше подумать о том, насколько большим может быть количество событий, и использовать целочисленный тип с явным размером.

Использовать double последовательно для измерения задержки

Я рекомендую вам использовать double для всех переменных, в которых хранятся задержки, будь то единичное измерение или среднее значение. Вы явно вовлекаете doubles в какой-то момент, и хотя могут возникнуть некоторые накладные расходы на использование doubles повсюду, вы избавляетесь от накладных расходов на преобразование между int и double.

Будьте последовательны при именовании переменных

Вы, кажется, используете Венгерская нотация только для нескольких переменных, например m_v_strRTSWindow и m_strSysLatResultsComplete, но у многих других переменных их типы не закодированы в имени. Если вы не делаете это постоянно, нет смысла использовать венгерскую нотацию, и я настоятельно рекомендую вам прекратить использовать ее везде.

Кроме того, вы используете m_ префикс для переменных-членов почти везде, но у некоторых переменных его нет, например sld, jsonSLD, startTimeUTC, dataExported и dataUploaded.

Нет необходимости typedef structs в C ++

Вместо написания:

typedef struct SYSLAT_DATA {
    ...
} SYSLAT_DATA:

Вы можете просто написать:

struct SYSLAT_DATA {
    ...
};

В отличие от C, в C ++ вы можете просто использовать тип SYSLAT_DATA без необходимости писать struct перед ней.

Избегайте повторения имен

В struct SYSLAT_DATA, есть много переменных-членов с именами m_systemLatencySomething. Поскольку struct Сама по себе задержка системы уже упоминается, вам не нужно это повторять. Это значительно сокращает имена переменных.

Использовать std::chrono типы для всего, что связано со временем

Я вижу, вы используете различные типы C для хранения времени, в том числе time_t и struct tm, и у вас даже есть char массивы для хранения дат. Начиная с C ++ 11, есть отличные std::chrono средства работы с моментами времени и продолжительностью. Я настоятельно рекомендую вам начать использовать их постоянно.

Использовать std::mutex вместо специфичных для платформы функций

Начиная с C ++ 11, потоки, мьютексы и другие примитивы синхронизации стали стандартизированными. Я рекомендую вам использовать стандартные функции вместо специфичных для Windows. Типы и функции из STL гораздо более естественны для использования в программе на C ++, и это упростит вам задачу, если вы когда-нибудь захотите перенести свой проект на другие операционные системы.

Например, если вы используете std::mutex за m_Mutex, вам не нужны конструктор и деструктор в class CSysLatData больше.

Кто отвечает за блокировку мьютекса?

Я вижу, у вас есть функции-члены вроде AcquireSLDMutex() и ReleaseSLDMutex(). Это сильно намекает на то, что class CSysLatData сам по себе не блокирует и не освобождает мьютекс, но требует, чтобы это делали другие части вашей программы. В таком случае, наверное, логичнее переместить мьютекс из class CSysLatData, Итак class полностью независим от потоков.

Держите ваши structпесок classтощий

//putting all of this in a struct has got to be unbelievably stupid

Наверное. Для чего нужны все эти переменные? Неужели их все нужно хранить? Вы должны сохранять только те переменные, которые хранят данные, которые вам понадобятся позже. Временные переменные, используемые только внутри одной функции, скорее всего, не нужно помещать в struct, и если это не связано с производительностью, вам не нужно хранить что-либо, что может быть получено из других переменных.

Геттеры и сеттеры

//using getters and setters for all of these seems stupid...

У геттеров и сеттеров есть свое место, но на самом деле необходимость писать десятки геттеров кажется глупой. В вашем случае вы можете заменить большинство геттеров одним, который выглядит так:

const SYSLAT_DATA &GetData() {
    return sld;
}

Это возвращает const ссылка на sld. Компилятор будет обеспечивать, чтобы вызывающий абонент не мог писать в sld, только читай с него. Итак, вместо того, чтобы писать звонящий:

CSysLatData syslat;
...
std::size_t counter = syslat.GetCounter();
double total = syslat.GetTotal();
...

Теперь он может писать:

CSysLatData syslat;
...
auto &data = syslat.GetData();
std::size_t counter = data.m_counter;
double total = data.m_total;
...

Дублирование переменных в struct SYSLAT_DATA

Помимо векторов в начале, кажется, есть две копии каждой переменной, одна из которых называется m_something, и один назван m_somethingEVR. Может, имеет смысл переписать его примерно так:

struct SYSLAT_DATA {
    struct Statistics {
         std::size_t counter;
         double total;
         double average;
         double median;
         ...
    };

    std::vector<...> ...;
    ...

    Statistics m_statistics;
    Statistics m_statisticsEVR;
};