This program reads from serial port, Saves this to a text file, increments file name each time it is run

Question

I want to condense it and get rid of any noob programming errors.

The code works fine but I know it's not as elegant as it could be and I really want to improve. Your advice would be brilliant!

I've talked to other engineers and had some basic help but not a lot.

#include <windows.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <io.h>
#include <time.h>
void serialPort(HANDLE Comms);
int readPort(HANDLE Comms, int testNum);
void parseFile(int fileNum);
int fileExist(void);

    int main(){
    //OPENING SERIAL PORT

    int res = 0;        //variables to be passed to functinos
    int logNum = 0;
    HANDLE hComm;
    hComm = CreateFile( "\\\\.\\COM4",                //port name
                        GENERIC_READ | GENERIC_WRITE, //Read/Write
                        0,                            // No Sharing
                        NULL,                         // No Security
                        OPEN_EXISTING,// Open existing port only
                        0,            // Non Overlapped I/O
                        NULL);        // Null for Comm Devices

      if (hComm == INVALID_HANDLE_VALUE){ //print if COM port wont open
          printf("Error opening port");
          exit(42);
      }
      else {
          printf("Opening of port successful\n");   //Print to know serial port is open
      }

    //Calling necessary functions
    serialPort(hComm);
    res = fileExist();
    logNum = readPort(hComm, res);
    parseFile(logNum);
    CloseHandle(hComm);//Closing the Serial Port
    return 0;   //main returns 0
    }



    void serialPort(HANDLE Comms){
    //CONFIGURING SERIAL PORT
        BOOL Status;
        DCB dcbSerialParams = { 0 }; // Initializing DCB structure
        dcbSerialParams.DCBlength = sizeof(dcbSerialParams);

        dcbSerialParams.BaudRate = CBR_57600;  // Setting BaudRate = 9600
        dcbSerialParams.ByteSize = 8;         // Setting ByteSize = 8
        dcbSerialParams.StopBits = ONESTOPBIT;// Setting StopBits = 1
        dcbSerialParams.Parity   = NOPARITY;  // Setting Parity = None

        SetCommState(Comms, &dcbSerialParams); //Assignin serial settings to comm handle
        Status = EscapeCommFunction(Comms, SETRTS); //RTS Request to Send Signal to esentially reset Serial port (arduino)
        Status = EscapeCommFunction(Comms, SETDTR); //DTR data terminal ready, PC side terminal ready to recieve data

    }


    int fileExist(void){
    //DOES FILENAME ALREADY EXIST

    int result = 0;
    char testNum[10];    
    char fp[80];
    char file[80] = "C:/Users/jgrayjt/Documents/Logs/TestLog_";
    char fileEnd[5] = ".txt";

    for (int i = 1; i < 1000; i++){

        sprintf(testNum, "%d", i); //converting testNum as int to string
        //adding test num to file dir
        strcpy(fp, file);       //adding strings to make file name
        strcat(fp, testNum);
        strcat(fp, fileEnd);
        printf("%s", fp);

            /* Check for existence */
            if((_access(fp, 0 )) != -1 ){           //Function to check if file exists
                printf( "\nFile logs exists\n" );
            }
             else { //if file doesnt exists 
              printf("\nFile doesnt exist\n");
              result = i; //set result to equal current value for i
              break;
             }
        }
    return result;  //send file num to next function
    }


    int readPort(HANDLE Comms, int testNum){
    //READING DATA FROM SERIAL PORT
    //time_t rawtime;
    //struct tm * timeinfo;
    //time (&rawtime);
    //timeinfo = localtime (&rawtime);
    char word[14] = "Test Finished";  //Text to look for
    char SerialBuffer[256];//Buffer for storing Rxed Data
    DWORD NoBytesRead;  //set type for NoBytesRead
    char Num[10];
    sprintf(Num, "%d", testNum);    //convert test num to string
    char str[80];
    //char buf[100];
    char *file = "C:/Users/jgrayjt/Documents/Logs/TestLog_";
    //char tandd[8] = "_%x_%X";
    char fileEnd[5] = ".txt";
    //strftime(buf,64, "_%x_%X", timeinfo);
    strcpy(str, file);      //adding strings to make file name
    strcat(str, Num);
    //strcat(str, buf);
    strcat(str, fileEnd);
    printf("%s", str);

    FILE *fp;    //create file pointer

    fp = fopen(str, "w");   //create file to write serial data to

        do { //start do while loop
            ReadFile(Comms,           //Handle of the Serial port
                     SerialBuffer,       //
                     sizeof(SerialBuffer) -1,//Size of Buffer
                     &NoBytesRead,    //Number of bytes read
                     NULL);

            SerialBuffer[NoBytesRead] = '\0'; //Specify null terminator to stop data rxing strangly

            printf("%s", SerialBuffer); //Print rx'd data to CMD
            fprintf(fp, "%s", SerialBuffer); //Save rx'd data to txt file


            char *fin = strstr(SerialBuffer, word); //Find 'search' in SerialBuffer and assign to 'fin'

            //if fin has data then close Serial and close/save file
            if (fin != NULL){
                printf("Results Successfully logged \n \r \n \r");
                fclose(fp);         //close file
                fp = NULL;          //set file pointer to equal null to break from the loop
            }
        } while (fp != NULL);  //while, from do while loop
    return testNum; //return testNum for use in another function
    }



    void parseFile(int fileNum){
    //FUNCTION TO PARSE DATA FROM LOG FILE TO FIND FAILED TESTS

    char Num[10];   //set Num as char and give extra memory
    sprintf(Num, "%d", fileNum); //Convert filenNum to string
    char dir[80];   //set dir as char and give extra memory
    char *file = "C:/Users/jgrayjt/Documents/Logs/TestLog_";
    char fileEnd[5] = ".txt";
    strcpy(dir, file);      //adding strings to make file name
    strcat(dir, Num);
    strcat(dir, fileEnd);
    printf("%s", dir);
    FILE *fp1;      //create file pointer
    char str[512];  //string to store data read from log file
    const char fail[5] = "FAIL";    //text to look for
    const char testnum[23] = "Test Iteration";     //text to look for
    char *ret;  //create char variables
    char *num;

    fp1 = fopen(dir , "r");  //Open file to read

        if (fp1 == NULL){ //if you cant open file, print 
            printf("File open fail");
        }

        //Loop to find test number and fail strings  
        while( fgets (str, 512, fp1)!=NULL ) { 
            num = strstr(str, testnum); //find test num in string and assign to num
            ret = strstr(str, fail);    //find 'fail' in string and assign to ret

        if (ret != NULL && num != NULL){    //if both variables have data then print num
            printf("\n \r %s\n\r", num);  //num has been given enough mem to include the fail that comes after it
        }
        }

    fclose(fp1);    //close/save fil
    getchar();
    getchar();
    }

Answer 1

Welcome to Code Review, nice first question. Don't Panic, and always take a towel with you.

General Observations
This may be a copy and paste error, but the level of indentation is inconsistent. This makes writing, reading and debugging the program very difficult.

There are too many comments. Code should be self documenting for the most part, only things that won't be apparent by reading the code should be in comments.

Comment blocks are ok at the top of a function to describe what the function does and when it should be called.

Code needs to be maintained, there may be features added or bugs fixed. Since code will change, the comments may have to change or they become irrelevant. For this reason most developers write only absolutely necessary comments indicating strategy or citing requirements.

The only type of programming that would require the level of comments in this program would be assembly code programming.

The exit() Function and Magic Numbers
Generally the exit(int status) function isn't necessary in the main() function, the use of return would be better. The exit() function can be used in subroutines if a none recoverable error occurs in a subroutine but there are also other ways to handle these errors that you may want to look into in the future. Return from the main() function should return one of two values, EXIT_SUCCESS or EXIT_FAILURE. These symbolic constants are available from stdlib.h which is included in this program.

    exit(42);

It's not clear what 42 means unless you're a fan of "Hitchhiker's Guide to the Galaxy". The use of numeric constants rather than symbolic constants in code is sometimes referred to as Magic Numbers. Generally symbolic constants are preferred because they make the code easier to understand and maintain.

In the the C programming language there are 2 methods for defining symbolic constants, the more modern method is

const int StringSize = 80;

The historic method defining symbolic constants is to use a macro

#define SERIAL_BUFFER_SIZE 256

Symbolic constants are very helpful for arrays, not only can one define the array using the symbolic constants, but any loops that access the array can use the symbolic constant as the end of the loop. This allows the programmer to change array sizes with only one edit rather than modifying the constant value everywhere.

The function int readPort(HANDLE Comms, int testNum) has a number of arrays with numeric constants that should be converted.

char word[14] = "Test Finished";  //Text to look for
char SerialBuffer[256];//Buffer for storing Rxed Data
char Num[10];
char str[80];
char *file = "C:/Users/jgrayjt/Documents/Logs/TestLog_";
char fileEnd[5] = ".txt";

If the array SerialBuffer was defined using a symbolic constant then the code to read the serial port doesn't need the sizeof(SerialBuffer) it can use the symbolic constant.

Note: C style strings don't necessarily require an array constant.

    char word[14] = "Test Finished";  //Text to look for

would be easier to define and use as

    char *word = "Test Finished";

The code already uses this method for

    char *file = "C:/Users/jgrayjt/Documents/Logs/TestLog_";

The variable name word might better be SearchString.

It is best to be consistent through out a program as to how something is done, whether it is code indentation or variable declarations

The variable str might better be defined using BUFSIZ which is defined in stdio.h. File names can be at least BUFSIZ, 80 is not a realistic value for a fully defined file spec.

Missing Error Check
The function int readPort(HANDLE Comms, int testNum) Uses fopen() but doesn't check the value to see if the file has actually been opened. The do while loop terminates at the end if the file pointer is NULL, but the loop still executes at least one time and can attempt to write to a file that didn't open. This would cause the program to crash. The loop should not be entered if the file was not opened. The error should at least be reported if not handled.

Use DRY Code
There is a programming principle called Don't Repeat Yourself. There is code in the function int fileExist(void) that creates a filename, this same code is in the function int readPort(HANDLE Comms, int testNum) and void parseFile(int fileNum). It might be better if the code to create the filename was a function itself that was called by all of these function. One of the benefits of this is the code only has to be written and debugged once, and any maintenance can be done in one place rather than two places.

Code Complexity
The function main() has the appropriate complexity, but the functions int fileExist(void), int readPort(HANDLE Comms, int testNum) and void parseFile(int fileNum) are too complex (do too much). Each of these functions could have subroutines that they call and could make the function shorter and perform a single function.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

This programming principle compliments the DRY programming principle.

Declare the Variables When They are Needed
The original definition of the C programming language required all variable to be declared at the top of the function, but this is no longer necessary. Older books and examples misrepresent this.

An example:

int main(){
    HANDLE hComm;
    hComm = CreateFile( "\\\\.\\COM4",  GENERIC_READ | GENERIC_WRITE,  0, NULL, OPEN_EXISTING, 0, NULL);

    if (hComm == INVALID_HANDLE_VALUE){
        printf("Error opening port");
        exit(EXIT_FAILURE);
    }
    else {
        printf("Opening of port successful\n");
    }

    serialPort(hComm);
    int res = fileExist();
    int logNum = readPort(hComm, res);
    parseFile(logNum);

    CloseHandle(hComm);//Closing the Serial Port

    return EXIT_SUCCESS;
}