HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Validating lines in a file using certain specifications

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filevalidatingusingspecificationscertainlines

Problem

I have written a program which basically reads a file named "data.txt" line-by-line. As a line is read, it validates the line with a certain specification. If the specification is met, it will output the data within a file named data.txt. If the line does not meet the validation criteria, then it will output that specific line to a separate file named error.txt.

I have finished the code and was wondering if anyone could look at it and see if they could help me with a few points in making the code look more compact and tidy. I feel I have done the same code over and over again for no reason. I have added comments so the code should make sense.

```
#include //library including standard input and output functions
#include //library including exit and system functions used below
#include //library including string functions used

struct packet{
int source; // 1 - 1024 range (int)
int destination; // 1 - 1024 range (int)
int type; // 0 - 10 range (int) // Varibles for the structure
int port; // 1 = 1024 (int)
char data[50]; // 1 - 50 range (char)

};

int main()
{

char filename[32] = { '\0' } ; // variables which declare the I/O stream and the filename structure
char DataLine[71]; // Reads the file one line at a time
char ErrorLine[71]; // This is the varible that deals with the validation error
char TempStorage[5]; // Stores data to be validated
char TempData[50]; // Stores the data which will be validated
int TempS, TempD, TempT, TempP; // Stores the integer derived from the input file
int Flag = 0; // This is the Flag that indicates a Line has not passed validation
int Count = 0; // This is the Flag that indicated a line has passed validation
int Ecount = 0; // This counts the number of errors

struct packet *DataRe

Solution

Readability

This code is very hard to read and review, for many reasons:

  • Messy indenting: some code blocks are correctly indented (the struct), but most of the rest is not, the main function has a mix of various inexplicable indents



  • Very long lines: avoid long lines. Try to stay within ~80 characters when possible



  • As a general rule, comments should be on their own line. This will immediately make the code more readable, as I won't have to scroll to the far right to read something and then scroll back to the left to see the code



Naming

Use lowercase names for variables. For example, dataFile instead of DataFile.

Since you use TempS, TempD, ... are intended to be used at some point as fields of the packet struct, it would be better to name them after those fields in the first place:

  • tempSource instead of TempS for the source field



  • tempDestination instead of TempD for the destination field



  • ... and so on



The traditional loop variable is i, it would be just more natural to use that instead of ii.

Magic numbers

Some numbers appear multiple times. Their purpose is unclear, and if you ever change it in one place, you'll have to remember to change everywhere.
To prevent possible errors, it's better to define constants for them.
For example for the number 71.

Security

In general, strcpy is not safe, because it may overwrite beyond the end of the destination array. This line is immediately suspicious:

strcpy(ErrorLine, DataLine);


On closer look, I see that both ErrorLine and DataLine have the same size,
so it should be ok, but it would be better if this was obvious without thinking, by using strncpy, and using a named constant as the size parameter, the same constant used in the declaration of the destination array (as I pointed out in the previous point).

Code Snippets

strcpy(ErrorLine, DataLine);

Context

StackExchange Code Review Q#64104, answer score: 5

Revisions (0)

No revisions yet.