How to I clean up my code? It's a mess! I can't even develop it further.

Pages: 12
I've been working on this project for a while and I've added quite a few features. The only problem is, the more I work on it the messier and more hackish the code gets. Now, it's so messy that I forgot about a function I wrote, I have trouble reading it and I can't develop it further. I've looked up guides for cleaner code but it's still a mess. How do I make my code cleaner and more flexible?
Split things into files, keep naming conventions consistent, consider namespaces and be sure to remove "temporary" or "test" code as soon as you no longer need it - you shouldn't have to prematurely optimize things away, but try to find a more elegant solution that won't bloat the rest of your project.
Last edited on
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
#include<cmath>
#include<cstdio>
#include<cstdlib>
#include<cstdint>
#include<cstring>
#include<iostream>
#include"Portable.h"

FILE* ifile;
FILE* ofile;
int32_t IfileSize = 0;
//Switches
bool saving = 0;

long long int handle_numbers(char NumbHandString[32])
{
unsigned char NumbHandPercent = 0;
long long double NumbHandDec;
long long double NumbHandInt;
NumbHandOut[32];
NumbHandPercent = strchr(NumbHandString[32],'%');
if(NumbHandPercent != NULL)
	{
	strncpy(NumbHandOut,NumbHandString,NumbHandPercent -1);
	NumbHandDec = strtol(NumbHandOut,NULL,0);
	NumbHandDec = NumbHandDec /100;
	NumbHandDec = round(NumbHandDec *IfileSize);
	NumbHandDec = NumbHandInt;
	//NumbHandString = to_string(NumbHandInt);
	}
return strtol(NumbHandString, NULL, 0);
}


int32_t GetIfileSize()
{
int32_t getifilesizesize = 0;
fseek(ifile, 0L, SEEK_SET);
while(fgetc(ifile) != EOF){getifilesizesize = getifilesizesize +1;}
fseek(ifile, 0L, SEEK_SET);
return getifilesizesize;
}

unsigned char load_ifile(char LoadIfileName[32])
{
ifile = fopen(LoadIfileName,"rb");
if(ifile == NULL){printf("%s","ifile does not exist.\n");return 1;}
IfileSize = GetIfileSize();
return 0;
}

unsigned char load_ofile(char LoadOfileName[32])
{
ofile = fopen(LoadOfileName,"wb");
if(ofile == NULL){printf("%s","An error occoured with the primary output file.\n");return 1;}
return 0;
}

void errormsg()
{
printf("%s","Syntax error. Syntax is:\n");
printf("%s","corrupt [switches] <ifile> <ofile> <start> <stop> <every> <operation> <additional>\n");
printf("%s","ifile is the path to the input file.\n");
printf("%s","ofile is the path to the output file.\n");
printf("%s","start is the position in the file to start corrupting.\n");
printf("%s","stop is the position in the file to stop corrupting.\n");
printf("%s","every is the frequency of memory to ignore.\n");
printf("%s","\n");
printf("%s","Operations:\n");
printf("%s","add	Adds current byte to addative\n");
printf("%s","sub	Subtracts current byte from addative\n");
printf("%s","mul	Multiplies current byte by addative\n");
printf("%s","div	Divides current byte by addative\n");
printf("%s","mod	Modular current byte by addative\n");
printf("%s","xor	Xor's current byte by addative\n");
printf("%s","and	And's current byte by addative\n");
printf("%s"," or	Or's current byte by addative\n");
printf("%s","ran	Randomizes current byte\n");
printf("%s","not	Not's current byte\n");
printf("%s","\n");
printf("%s","Switches:\n");
printf("%s","-l [file]	Load corruption from file\n");
printf("%s","-v		Output corruptor version\n");
printf("%s","-s [file]	Save corruption to file\n");

exit(1);
}

char op(char oper[3],char a,char b)
{
if(strcmp(oper, "add") == 0){return a+b;}
else if(strcmp(oper, "sub") == 0){return a-b;}
else if(strcmp(oper, "mul") == 0){return a*b;}
else if(strcmp(oper, "div") == 0){return a/b;}
else if(strcmp(oper, "ran") == 0){return rand() % 128 -64;}
else if(strcmp(oper, "xor") == 0){return a xor b;}
else if(strcmp(oper, "and") == 0){return a and b;}
else if(strcmp(oper, "or") == 0){return a or b;}
else if(strcmp(oper, "not") == 0){return not(a);}
else if(strcmp(oper, "mod") == 0){return a % b;}
else{errormsg();}
}

void Old_Engine_While_Loop(unsigned long long int strt,unsigned long long int stop,unsigned int frequency,char* tempop,char addative)
{
unsigned long long int dimX = 0;
unsigned int FreqCounter = 0;
int regbyte;
while((regbyte = fgetc(ifile)) != EOF) //While we haven't reached the end of the file...
	{ 
	if((FreqCounter >= frequency) and ((dimX >= strt) and (dimX <= stop))) //And the conditions are juuust right...
		{
		regbyte = op(tempop,regbyte,addative);putc(regbyte, ofile);FreqCounter = 0; //corrupt dat byte!
		}
	else{putc(regbyte, ofile);} //Or don't...
	FreqCounter = FreqCounter +1;dimX = dimX +1; //counting
	} 
printf("%s","The file seems to have been successfully corrupted...\n");
}

int main(int argc, char* argv[])
{
char loadarray[8][32];
FILE* savefile;
FILE* loadfile;
unsigned long long int fsize;
unsigned char arg = 1;
//Start the REAL code here
if(argc == 1){errormsg();return 1;}
if(strcmp(argv[arg],"-v") == 0){printf("%s","LAwl\n");arg = arg +1;return 0;}
if(strcmp(argv[arg],"-s") == 0)
	{
	saving = 1;arg = arg +1;
	savefile = fopen(argv[arg],"wb");arg = arg +1;
	}
if(strcmp(argv[arg],"-l") == 0)
	{
	arg = arg +1;
	loadfile = fopen(argv[arg],"rb");arg = arg +1;
	if(loadfile == NULL){printf("%s","loadfile does not exist.\n");return 1;}
	if(load_ifile(argv[arg]) == 1){return 1;}arg = arg +1;
	if(load_ofile(argv[arg]) == 1){return 1;}arg = arg +1;
	fgets(loadarray[1],32,loadfile);
	fgets(loadarray[2],32,loadfile);
	fgets(loadarray[3],32,loadfile);
	fgets(loadarray[4],32,loadfile);
	fgets(loadarray[5],32,loadfile);loadarray[5][3] = '\0';
	fgets(loadarray[6],32,loadfile);
	Old_Engine_While_Loop(atoi(loadarray[2]),atoi(loadarray[3]),atoi(loadarray[4]),loadarray[5],atoi(loadarray[6]));
	return 0;
	}
if(load_ifile(argv[arg]) == 1){return 1;}arg = arg +1;
if(load_ofile(argv[arg]) == 1){return 1;}arg = arg +1;
//Onward
if((argc-arg) != 5){errormsg();return 1;}
Old_Engine_While_Loop(handle_numbers(argv[arg]),handle_numbers(argv[arg+1]),handle_numbers(argv[arg+2]),argv[arg+3],handle_numbers(argv[arg+4]));
if(saving)
	{
	fputs("[A.0.2.2]\n",savefile);
	fputs(argv[arg],savefile);
	fputs("\n",savefile);
	fputs(argv[arg+1],savefile);
	fputs("\n",savefile);
	fputs(argv[arg+2],savefile);
	fputs("\n",savefile);
	fputs(argv[arg+3],savefile);
	fputs("\n",savefile);
	fputs(argv[arg+4],savefile);
	fputs("\n",savefile);
	}
return 0;
}
Hi,

Just some general comments from a backyard dabbler, if I may :+)

The code is C, with a few C++ things like bool in there. IMO, don't mix the two styles. Either write C, or write C++.

I like to put forward declarations before main with function definitions after main, so I don't have to go looking for main.

With your indenting, I would like to see code in the body of a function indented. And braces should line up with the statement they are attached to. Maybe it's the way this site has formatted the code? If you change your editor to do 4 spaces say instead of tabs, it will display better here.

long long int Can you stick with the types in stdint.h (C) ?

unsigned char NumbHandPercent = 0; Interesting type for a percentage.

long long double this type doesn't exist.

1
2
3
4
5
6
7
unsigned char load_ifile(char LoadIfileName[32])
{
ifile = fopen(LoadIfileName,"rb");
if(ifile == NULL){printf("%s","ifile does not exist.\n");return 1;}
IfileSize = GetIfileSize();
return 0;
}


Why return unsigned char? C has a bool #include <stdbool.h>

char op(char oper[3],char a,char b) I am sure you know what you are doing here, and have your own reasons. Curious as to why return type is char - does that play well with integer division?

Line 104: If I have lots of arguments, I format them 1 per line like this:

1
2
3
4
5
6
void Old_Engine_While_Loop(unsigned long long int strt,
                            unsigned long long int stop,
                            unsigned int frequency,
                            char* tempop,
                            char addative)
{


I find that easier to read. Could some of those parameters be const?

Hopefully this has been helpful, even a little bit :+)
Last edited on
Thanks a lot, that's very helpful.
I should also mention, that the language mixing is necessary. I'll try to stick to the types defined in the header, but I'm honestly new to it. Bool is definitely necessary. Hmm, I've never thought about my type declaration for the percentage. When I wrote that, I thought, "I want it to be only positive, so unsigned. I want it to be close to 100, so char." Would you recommend double or float for percentages? I've also never thought of my return type for load_ifile either... Also, I assumed C had its own bool type in cstdlib. The return type is char because a char is extracted from a file, modified and then placed in a new file. I tested it out on a txt and it worked out fine. Thanks for the line 104 tip, I had no idea what to do with all those arguments, haha. You should've seen my face when I wrote that initialization. I believe tempop could be made const, but I don't know how that'd work... Can non chars be const?
I should also mention, that the language mixing is necessary.


It seems to me the only C++ thing is bool, you could use the C bool #include <stdbool.h> , along with compiling to the C11 C standard. Would have to change the headers to the C versions though.

Hmm, I've never thought about my type declaration for the percentage. When I wrote that, I thought, "I want it to be only positive, so unsigned. I want it to be close to 100, so char."


I am not so sure about an advantage of using small types like char to try and save memory. It is slower because the compiler has to unpack a machine word (mostly 64 bit these days) to get at the variable, and that is assuming that several of these variables were packed into 1 machine word in the first place. I am not sure how that packing is implemented though. The stdint.h and <cstdint> headers have "fast" types, which store variables in a way which allows for fast storage and retrieval. It's easy to look at the typedef's in stdint.h to see how it works.

Would you recommend double or float for percentages?


I guess it depends on what you want to use it for. If it's a progress indicator say, then any unsigned integral type will do. If you need decimal places and are not worried about exceeding the precision of float, then float, otherwise double.

I've also never thought of my return type for load_ifile either... Also, I assumed C had its own bool type in cstdlib. The return type is char because a char is extracted from a file, modified and then placed in a new file. I tested it out on a txt and it worked out fine.


I mentioned that because the function only ever returns 1 or 0.

I believe tempop could be made const, but I don't know how that'd work... Can non chars be const


I was just saying in general about making parameters const, as a part of const correctness, do it whenever you can. I am sure you know that const can apply to any built in type?

Bleems ago, I remember using const pointers, in hindsight what I had was the same idea as a C++ reference, although the syntax wasn't as easy :+).

Also, I assumed C had its own bool type in cstdlib.


I couldn't find one in my system. If you are compiling with C++, it's probably using the C++ keyword.

A bit of white-space might make your code easier to read, even the load_ifile function:

1
2
3
4
5
6
7
8
9
10
11
12
13
unsigned char load_ifile(char LoadIfileName[32])
{
    ifile = fopen(LoadIfileName,"rb"); 

    if(ifile == NULL) {
        printf("%s","ifile does not exist.\n");
        return 1;
    } 

    IfileSize = GetIfileSize();

    return 0;
}


I know that having the if statement and two short statements all on one line, of itself is not a problem. But if one does that all the time, the code becomes dense.

The main function has a lot of code to process the options, could you kick that into a function of it's own?

Any way, we (or I?) might be getting bogged down in detail, I wonder what are some of the bigger things that are affecting your code? How many files of code do you have and how are they organised?
Sorry for getting back to you so late... I've never thought about using white-space to improve readability. I'll definitely break the switches down, I've thought about that before but I didn't know how to do it, so I left it. I don't mind the detail, I've never had anyone else really analize my code before. I think you've really helped me with a lot of the big stuff, I have a lot of writing to do. I currently have two files, but I'm only using the one. I'm going to rewrite it so it uses the functions in the other file. Also, when you ask how they're organized, what do you mean?
Last edited on
I don't mind the detail, I've never had anyone else really analize my code before.


No worries, I just wish some others might comment, I am just a backyard dabbler :+)

Also, when you ask how they're organized, what do you mean?


Well, just like C++ one organises the files into headers and implementation. When projects get even bigger, start putting files into separate directories. Do you have Kernighan & Ritchie's C programming book ? There is a simplified description of how to do this on pages 68 to 70.

Here is a link to K&R, if you don't already have the book. It's more moral IMO, if you were to buy that book, it shouldn't be that expensive. I still have my copy from 1987 :+)
http://zanasi.chem.unisa.it/download/C.pdf


BTW, is there any particular reason why you are doing this in C?

With the code formatting, your IDE should be able to do this for you. One can normally select which style you want, or make your own.
I don't have the book but I've always wanted a copy, thanks! You're awesome! Whoa, 1987? What sort of projects have you worked on in your life? Are you a hobbyist?

I'm doing this in C because the headers are much smaller and the final compiled program is smaller on most OS's. The biggest feature of my program is its ability to run on "more than just Windows". I'm trying to keep everything minimal and small in a language I (sort of) know well.

To be honest, I'm not using an IDE. I'm using gedit as an editor and gcc as my compiler, so I don't know what you mean.
To be honest, I'm not using an IDE. I'm using gedit as an editor and gcc as my compiler, so I don't know what you mean.

You can use astyle from the command line.

> astyle --style=allman headers/*.h*
> astyle --style=allman source/*.cpp
What does astyle even do?
Whoa, 1987? What sort of projects have you worked on in your life? Are you a hobbyist?


Yep, just a hobbyist. Back in 1987, I used to stay at work until about 11pm learning C from K&R on a UNIX machine. It was hard going, no internet or anyone to ask, just the book. I have been an Engineering Surveyor for the last 30 years and have had some exposure to UNIX shell scripting, awk (a UNIX pattern matching language) Fortran 77, Pascal, assembler (DOS 16bit), Relational Databases (Design and implementation), VBA (mainly AutoCAD), and finally C++. A lot of that I haven't used for years, so my memory of it is sketchy.

And only sometimes did I actually get to implement something for work. The times I did were spectacular though, I often felt like writing a book called "Computers for Managers". One day my boss gave me a print out of 30 pages of GPS co-ordinates (10 figures XYZ) of electrical assets, highlighting pen and a ruler: I was to manually type them in, drawing a particular symbol at that position :+( I said I could write a short program to do that automatically. No, Not allowed: we don't do programming here - never see the end of it :+( So I sat there all day typing in co-ordinates, but after work I wrote a 6 line LISP routine. Next morning, at 8:05AM I went to my boss and said that I was finished !! :+D He was upset, he wanted to charge the client for about 3 weeks of my time. I said: "No reason why you can't still do that :+)" I used that program time and time again, was especially handy when we were given a wrong dataset.

I did have an actual IT job for about 6 months: to be honest the system we were using I found to be a nightmare. The main problem for me was that fields had to have a unique name across the whole database, and there was a 40 char limit. The DB had 500 tables and some of them had 300 fields, so looking at some field names, they would all be the same except for the last 3 chars. So one can imagine how difficult it was to understand. I mentioned the idea of naming the fields with the table name, a delimiter, then the field name: thus having about 30 chars for the field name. But I got shouted down because that what MS Access does. To be fair, the DB had already been written (500KLOC) and had data so making those changes would not have been feasible.

I'm trying to keep everything minimal and small in a language I (sort of) know well.

No worries :+)

To be honest, I'm not using an IDE. I'm using gedit as an editor and gcc as my compiler, so I don't know what you mean.


I strongly recommend you get hold of the clang compiler from llvm. It prints nice error messages, and has been a frontrunner in the implementation of new standards. Although JLBorges informed me the other day that MS is ahead of the game at the moment, and working hard to achieve c++17 compliance.

llvm also has other utilities, one of them is llvm-format - you could use this from the shell. There are others like A-style which allows one to choose which style, and can be run from the shell.
Go through your functions and comment them: what are the inputs, what are the outputs? What do they do? What happens if there is an error? Make sure they don't use globals unless absolutely necessary. By doing this you'll make them much cleaner.

Take handle_numbers for example. The name isn't very meaningful. Also, the only line that has any effect is line 31. So what is this function really supposed to do? Decide what it does and then do it.

Here's a trick to writing well designed code: your functions/methods don't have to "do the work." They should "make doing the work easy." For example, getIfileSize() currently gets the size of one file and one file only. Change that to getFileSize(FILE *fp); Now it can get the size of any file. Also, you can get the size by simply seeking to the end of the file and then checking the position with ftell(). That's about a zillion times faster than reading every character.

load_ifile is confusing. It opens the file AND gets the size, storing both in globals. Load_ofile sounds like it should do the same thing but it doesn't. I suggest a single function that opens a file and checks for errors. Something like:
FILE *openFile(const char *name, const char *mode)
{
FILE *result = fopen(name, mode);
if (result == NULL) {
printf("Can't open %s: %s\n", name, strerror(errno));
}
return result;
}

function op() doesn't look right. You say it takes a 3-character array (oper) but compare it like it's 4 characters (remember those 3-character strings have terminating null bytes). It would be better to declare oper as const char *.
Old_Engine_While_Loop()? What the heck is that supposed to do? :) Choose a better name.

Learn and use getopt() for command line processing. It's very common and easy to use.
closed account (48bpfSEw)
If I would be faced with an unreadable sourcecode I would start to use my own classes and build em up with existing code. This technique is named as "facade":

https://sourcemaking.com/design_patterns/facade
What does astyle even do?


It's a code formatter. There are all kind of rules about bracing styles, indenting, white space around operators and parentheses, and so on. One can choose an existing style, or make your own.

@Necip

The OP is doing C. Is it possible to do DP without classes?
closed account (48bpfSEw)
@TheIdeasMan, designpatterns are independed from languages, these are concepts to work with.

I use the source navigator tool (https://sourceforge.net/projects/sourcenav/)
to get the overview of all functions and classes PLUS cross-referenzes!

It's amazing to work with this open source tool!

Control is nothing without knowing!
Necip wrote:
@TheIdeasMan, designpatterns are independed from languages, these are concepts to work with.


But they require the OOP paradigm, the OP is using C, which isn't OOP.
closed account (48bpfSEw)
The first C++ compiler ("C with classes") would actually generate C code, so that's definitely doable.


http://stackoverflow.com/questions/524033/how-can-i-simulate-oo-style-polymorphism-in-c

Last edited on
I like to chat with you! ^^


That may be so, but this is off topic and derailing the OP's topic - maybe you should start your own Topic?
Pages: 12