Nick #14

grf14003 · 2020-04-22T19:22:06Z

No description provided.

grf14003

Hey Nick,

Great job on this code! You have made some excellent progress. My comments were mostly formatting things so no big deal there. I did have one major suggestion. The writeLog function should also print to stdout. I talk about it more in my line comments but its probably my fault. I should have been more clear when I was describing the purpose of this function to you. Anyways, let me know if you have any questions and push your changes when you make the edits I suggested. Don't merge yet though!

Best regards,
Greg

grf14003 · 2020-04-22T20:26:42Z

bfs/CMakeSettings.json

-          "type": "PATH"
-        }
-      ]
+      "variables": []


Get rid of this change, you should not need to edit this file.

grf14003 · 2020-04-22T20:27:17Z

bfs/include/Logger.hpp

+public:
+	string filename;
+	string filePath;
+	FILE* fp;


fp, filename, and filepath should all be private

grf14003 · 2020-04-22T20:32:17Z

bfs/src/comms/AlgorithmServer.cpp

-		write_log("Listening for clients...\n", 1, logger->filename, logger);
-		fprintf(stderr, "Listening for clients...\n");
+		logger->writeLog("Listening for clients...\n");
+		logger->writeLog("%s", logger->filePath);


You can just combine this line into the previous one right?

grf14003 · 2020-04-22T20:32:40Z

bfs/src/comms/AlgorithmServer.cpp

-		write_log("Hello new client!\n", 1, logger->filename, logger);
-		fprintf(stderr, "Hello new client!\n");
+		logger->writeLog("Hello new client!\n");
+		printf("Hello new client!\n");


Get rid of this printf

grf14003 · 2020-04-22T20:33:38Z

bfs/include/Logger.hpp

+	~Logger();
+	void writeLog(const char* fmt, ...);
+	static Logger* getLogger(Logger* logger);
+	string getPath();


Some of these functions can be private I think

grf14003 · 2020-04-22T20:50:45Z

bfs/src/logging/Logger.cpp

+
+void Logger::writeLog(const char* fmt, ...) {
+	this->fp = fopen(this->filePath.c_str(), "a");
+	//if (this->logging_Level <= msg_level) {


Get rid of all the commented out code in this function

grf14003 · 2020-04-22T20:51:25Z

bfs/src/logging/Logger.cpp

+	fprintf(this->fp, ctime(&now));
+	fprintf(this->fp, "\n");
+	fprintf(this->fp, fmt);
+	fprintf(this->fp, "\n");


All these fprintf statements can be compacted into one line

grf14003 · 2020-04-22T20:51:43Z

bfs/src/logging/Logger.cpp

+Logger* Logger::getLogger(Logger* logger) {
+	if (logger == NULL) {
+		logger = new Logger(1);
+


Get rid of this space

Still need to do this

grf14003 · 2020-04-22T20:55:03Z

bfs/src/logging/Logger.cpp

+	struct tm * now = localtime( & t );
+	char buffer [80];
+	strftime (buffer,80,"%Y-%m-%d-%Hhr%Mm%Ss",now);
+	std::string str(buffer);


You can get rid of all the std:: in this file because you have "using namespace std" in your header

grf14003 · 2020-04-22T20:56:27Z

bfs/include/Logger.hpp

+
+using namespace std;
+
+


Too much space here, I know it seems kinda picky but there should just be one.

grf14003

Hey Nick, Still a couple more things to fix. I tried to explain it better this time because maybe I was a little vague in my last set of comments. You also need to rebase your branch to dev before you cal merge the pull request. Also, don't merge until a approve it.

Best regards,
Greg

grf14003 · 2020-04-30T17:29:21Z

bfs/include/Logger.hpp

+
+};
+
+static Logger* logger = logger->getLogger(logger);


Get rid of this global, if we need to use the logger in the code, we can just call getLogger(). Instead, add it as a private static pointer in the class initialized to null at first. The first time getLogger is called, this private static variable will be null, so initialize it. Every other call to getLogger (after the first) will just return the pointer and NOT reinitialize it

grf14003 · 2020-04-30T17:30:23Z

bfs/src/logging/Logger.cpp

+
+string Logger::getPath() {
+	char buffer[MAX_PATH];
+    GetModuleFileName(NULL, buffer, MAX_PATH); // Get current working directory


You still didn't fix the indenting here

grf14003 · 2020-04-30T17:31:04Z

bfs/include/Logger.hpp

+	~Logger();
+	void log(const char* fmt, ...);
+	static Logger* getLogger(Logger* logger);
+	string getPath();


This getPath function should be static

grf14003 · 2020-04-30T17:31:40Z

bfs/src/logging/Logger.cpp

+	fclose(this->fp);
+}
+
+Logger* Logger::getLogger(Logger* logger) {


You don't have to pass the logger pointer here, just use the global

grf14003 · 2020-04-30T17:33:27Z

bfs/src/logging/Logger.cpp

+
+void Logger::log(const char* fmt, ...) {
+	this->fp = fopen(this->filePath.c_str(), "a");
+	auto now = chrono::system_clock::to_time_t(chrono::system_clock::now());


If you are going to use auto here, add a comment to say what the type is that is returning

Still need to do this!

grf14003 · 2020-04-30T17:33:44Z

bfs/CMakeSettings.json

-          "type": "PATH"
-        }
-      ]
+      "variables": []


You still need to get rid of this change

grf14003

Hey Nick,

Good job fixing some things. Still a couple of small things to fix. There are still some globals to remove. Rather that using global variables in the data sync thread and algorithm server, just call getLogger any time you need it in the code rather than referencing the global.

There are also some conflicts so you have to rebase to the latest dev before you can merge. If you have questions about this, ask on WA or ask on WA if I am available to chat on discord.

There are also some things that it looks like you missed in my last review so make sure you address those this time. Hopefully we can get this merged by EOD today.

Best regards and happy editing,
Greg

grf14003 · 2020-05-08T17:14:27Z

bfs/include/Logger.hpp

+
+};
+
+//static Logger* logger = logger->getLogger(logger);


Get rid of this comment

grf14003 · 2020-05-08T17:14:40Z

bfs/include/Logger.hpp

+	string filename;
+	string filePath;
+	FILE* fp;
+


Get rid of this space

grf14003 · 2020-05-08T17:15:10Z

bfs/src/comms/AlgorithmServer.cpp

@@ -1,8 +1,9 @@

 #include "AlgorithmServer.hpp"
-#include <Logger.h> 
+#include "Logger.hpp"


This should be included in the algorithm server hpp file not the cpp file.

grf14003 · 2020-05-08T17:15:32Z

bfs/src/comms/AlgorithmServer.cpp



+Logger* server_Logger = server_Logger->getLogger();


This is a global variable when located right here and should be removed

grf14003 · 2020-05-08T17:16:05Z

bfs/src/comms/AlgorithmServer.cpp

 	vector<Attribute> newAttribs;
+	//vector<Attribute>* newAttribs = new vector<Attribute>;


Still need to get rid of this

grf14003 · 2020-05-08T17:16:51Z

bfs/src/comms/DataSyncThread.cpp


+Logger* thread_Logger = thread_Logger->getLogger();


Same as above. Global variables are bad! Get rid of this

grf14003 · 2020-05-08T17:17:56Z

bfs/src/logging/Logger.cpp

+Logger::Logger(int l_Level) {
+	this->logging_Level = l_Level;
+	time_t t = time(0);   // get time now
+	struct tm *now = localtime( & t );


Get rid of the spacing in the &t at the end of this line

grf14003 · 2020-05-08T17:18:37Z

bfs/src/logging/Logger.cpp

+	time_t t = time(0);   // get time now
+	struct tm *now = localtime( & t );
+	char buffer [80];
+	strftime (buffer, 80, "%Y-%m-%d-%Hhr%Mm%Ss", now);


This time format string: "%Y-%m-%d-%Hhr%Mm%Ss" should be a private const variable in the logger class

grf14003 · 2020-05-08T17:18:59Z

bfs/src/logging/Logger.cpp

+
+void Logger::log(const char* fmt, ...) {
+	this->fp = fopen(this->filePath.c_str(), "a");
+	auto now = chrono::system_clock::to_time_t(chrono::system_clock::now());


Still need to do this!

grf14003 · 2020-05-08T17:19:23Z

bfs/src/logging/Logger.cpp

+Logger* Logger::getLogger(Logger* logger) {
+	if (logger == NULL) {
+		logger = new Logger(1);
+


Still need to do this

ncc14003 added 12 commits February 20, 2020 13:22

Test log

b3936ca

Logger changes

c0b68e9

Reupload logger changes

810ff1b

Removing old logger file

daa4ff9

CMake

e0b4037

Rebase part 1

f8c705d

Logger changes

7f6854f

REBASE 3/5

d43c958

Removing old logger file

ba311d5

CMake

cabb7d4

Merging

710a12f

Logger fixes

9de731e

grf14003 assigned ncc14003 Apr 22, 2020

grf14003 commented Apr 22, 2020

View reviewed changes

Logging polishing

6f9b08e

grf14003 commented Apr 30, 2020

View reviewed changes

ncc14003 added 2 commits May 8, 2020 12:29

CMake revert

afceba1

Final edits

edc74e0

grf14003 commented May 8, 2020

View reviewed changes

ncc14003 added 2 commits May 8, 2020 20:24

Final edits

2d08d00

Final edits

b1393ff

Nick #14

Nick #14

grf14003 commented Apr 22, 2020

grf14003 left a comment

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 May 8, 2020

grf14003 Apr 22, 2020

grf14003 Apr 22, 2020

grf14003 left a comment

grf14003 Apr 30, 2020

grf14003 Apr 30, 2020

grf14003 Apr 30, 2020

grf14003 Apr 30, 2020

grf14003 Apr 30, 2020

grf14003 May 8, 2020

grf14003 Apr 30, 2020

grf14003 left a comment

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

		vector<Attribute> newAttribs;
		//vector<Attribute>* newAttribs = new vector<Attribute>;

Nick #14

Are you sure you want to change the base?

Nick #14

Conversation

grf14003 commented Apr 22, 2020

grf14003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grf14003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grf14003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment