Tuning the data class for the SysLat System Latency Testing hardware for gaming computers - SysLat Cleanup #1

Question

Here's my open source project for testing the latency of gaming computers : https://github.com/Skewjo/SysLat_Software

This software controls the hardware that tests the latency of a gaming computer that can be found at https://syslat.com/. The follow up question to this question can be found here. Since this program is too large to include in a single question here are related questions Cleaning up the USB controller and Cleaning up the Main Dialog.

Today I'd like to attempt to clean up my "SysLatData" class.

Header file here: https://github.com/Skewjo/SysLat_Software/blob/master/SysLatData.h
Implementation here: https://github.com/Skewjo/SysLat_Software/blob/master/SysLatData.cpp
Definition and use here: https://github.com/Skewjo/SysLat_Software/blob/master/SysLat_SoftwareDlg.cpp

I think the first couple things I need to attack (after a cleanup of typedef and my include guards) are:

• Moving some functionality out of this class and into the core program (specifically UpdateSLD()) and creating setters.
• Moving the export functions into a new static, generic library (specifically CreateJSONSLD() and ExportData())
• Moving the unrelated variables into the core program.(such as m_targetApp, m_RTSSVersion, and m_boxAnchor)

But I don't truly know what to do and that's why I'm here!

#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

Any help you can provide is greatly appreciated.

• Toby Speight: It sounds like you already know what can be improved here. You should probably do that first, before asking for a review.
• Skewjo: Well, I've included my best guess, but I'm hesitant to attempt to implement it without seeking some kind of review or head nod from... anyone. I guess I can take your comment as that head nod.
• pacmaninbw: @TobySpeight is correct it seems like you know what needs to be done. We can't offer advice on what needs to be done, we can only make observations about working code that is already complete. We also need all of the code you want reviewed in the question, we can only use linked code repositories as references. Please read How do I ask a good question. One of your options is to break the large class into smaller classes and include those smaller classes in the larger class. SOLID programming
• Toby Speight: Don't interpret my comment as a review - just stating that the code should be finished to the best of your ability before it's ready for review.
• pacmaninbw: You might want to look at the Composite Design Pattern.

Answer 1

Avoid macros

You should declare constants as constexpr variables instead of as #defines, like so:

constexpr std::size_t MOVING_AVERAGE = 100;

Use std::size_t for sizes, counts and array indices

An int might not be large enough to represent all possible sizes of arrays, and negative values don't make sense for sizes and counts either. The proper type is std::size_t.

If you are not dealing with arrays or loop counters, but just need a variable to count events, you might also use std::size_t for that instead of int, although it might be better to think about how large the number of events might be, and use an integer type with an explicit size.

Use double consistently for latency measurements

I recommend you use double for all variables that store latencies, whether it's just a single measurement or an average. You clearly involve doubles at some point, and while there might be some overhead of using doubles everywhere, you get rid of the overhead of converting between int and double.

Be consistent when naming variables

You seem to be using Hungarian notation for only a few of the variables, like m_v_strRTSWindow and m_strSysLatResultsComplete, but many of the other variables do not have their types encoded in the name. If you are not doing this consistently, there is no point in using Hungarian notation, and I strongly recommend you stop using it everywhere.

Furhtermore, you use the m_ prefix for member variables almost everywhere, but some variables don't have it, like sld, jsonSLD, startTimeUTC, dataExported and dataUploaded.

There is no need to typedef structs in C++

Instead of writing:

typedef struct SYSLAT_DATA {
 ...
} SYSLAT_DATA:

You can just write:

struct SYSLAT_DATA {
 ...
};

Unlike C, in C++ you can then just use the type SYSLAT_DATA without having to write struct in front of it.

Avoid repeating names

In struct SYSLAT_DATA, there are many member variables that are named m_systemLatencySomething. Since the struct itself already mentions system latency, you don't have to repeat it. This shortens the variable names considerably.

Use std::chrono types for anything time related

I see you use various C types for storing time, including time_t and struct tm, and you even have some char arrays to hold dates. Since C++11, there are the excellent std::chrono facilities of dealing with timepoints and durations. I strongly recommend you start using those consistently.

Use std::mutex instead of platform-specific functions

Since C++11, threads, mutexes and other synchronisation primitives have become standardized. I recommend you use the standard functions instead of Windows-specific ones. The types and functions from the STL are much more natural to use in a C++ program, and it will make it easier if you ever want to port your project to other operating systems.

For example, if you use a std::mutex for m_Mutex, you don't need a constructor and destructor in class CSysLatData anymore.

Who is responsible for locking the mutex?

I see you have member functions like AcquireSLDMutex() and ReleaseSLDMutex(). This strongly hints that class CSysLatData itself doesn't lock or release the mutex, but requires other parts of your program to do this. In that case, it is probably more logical to move the mutex out of class CSysLatData, so the class is completely thread-agnostic.

Keep your structs and classes lean

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

Probably. What are all these variables for? Do they really need to be all stored? You should only keep those variables that store data that you need later on. Temporary variables used just inside one function most likely do not need to go into a struct, and unless there is a performance reason for doing so, you don't need to store anything that can be derived from other variables.

Getters and setters

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

Getters and setters have their place, but indeed having to write tens of getters seems silly. In your case, you can replace most getters with a single one that looks like this:

const SYSLAT_DATA &GetData() {
 return sld;
}

This returns a const reference to sld. The compiler will enforce that the caller cannot write to sld, only read from it. So instead of the caller writing:

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

It can now write:

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

Duplication of variables in struct SYSLAT_DATA

Apart from the vectors at the start, there seem to be two copies of each variable, one named m_something, and one named m_somethingEVR. Maybe it makes sense to rewrite it to something like:

struct SYSLAT_DATA {
 struct Statistics {
 std::size_t counter;
 double total;
 double average;
 double median;
 ...
 };
 std::vector<...> ...;
 ...
 Statistics m_statistics;
 Statistics m_statisticsEVR;
};

• Skewjo: Awesome. Thank you for all of this. I'm going to look through and try to implement most of these and I'll report back.
• Skewjo: Regarding the int vs. double issue... the hardware is currently only able to accurately measure and send values in milliseconds, so until I do operations on multiple measurements I don't need double. Also, I had some speed inconsistency issues with a few operations that I was doing that caused my USB read times to increase when using doubles, so I'm hesitant to move back towards them.
• Skewjo: I was able to implement basically all of your suggestions. Thank you. github.com/Skewjo/SysLat_Software/commit/…
• Skewjo: I'd toss you an upvote, but I don't have that privilege yet.
• pacmaninbw: @Skewjo you have enough points to up vote.