Furthermore, there are different meaning of the word 'code'. When you write C code, the meaning is not that nobody else should be able to understand it. If you want your code to be understandable (for others as well as yourself) avoid function names like:
Yes, you documented the functions, but code should be self-documenting first. Source code documentation comes second.
It's also very hard to find some structure in your code. You have hundreds of functions that appear to be working on the same set of global variables. Most functions are also way too long. It looks like you have a lot of code duplication.
Cheers, D Drmmr
Please put [code][/code] tags around your code to preserve indentation and make it more readable.
As long as man ascribes to himself what is merely a posibility, he will not work for the attainment of it. - P. D. Ouspensky
Furthermore, there are different meaning of the word 'code'. When you write C code, the meaning is not that nobody else should be able to understand it. If you want your code to be understandable (for others as well as yourself) avoid function names like:
Yes, you documented the functions, but code should be self-documenting first. Source code documentation comes second.
It's also very hard to find some structure in your code. You have hundreds of functions that appear to be working on the same set of global variables. Most functions are also way too long. It looks like you have a lot of code duplication.
Meaning lengthen the function names so they are understood? Like build_drive_letter_array or something like that?
I thought the mainline was very structured - function calls (really subroutines). Boom boom boom on down the line.
That's definitely not somebody's first program, but I'll comment anyway.
I find all those comments really distracting and annoying. Give your functions and variables meaningful names so that you don't have to explain them with comments.
Stop commenting the end of functions, loops, if statements, etc. If you use proper indentation and alignment, that's really easy to see just by looking at the code. //end function completely unnecessary.
That's definitely not somebody's first program, but I'll comment anyway.
I find all those comments really distracting and annoying. Give your functions and variables meaningful names so that you don't have to explain them with comments.
Stop commenting the end of functions, loops, if statements, etc. If you use proper indentation and alignment, that's really easy to see just by looking at the code. //end function completely unnecessary.
Understood. But with structured languages like this, you can get lost even with correct indenting. At least I can..... Do mean more verbose variable and function names?
Understood. But with structured languages like this, you can get lost even with correct indenting. At least I can..... Do mean more verbose variable and function names?
Yeah. Given this variable I chose randomly, lwbbkpst, nobody would have any clue what it is or what it does. Give it a name that means something.
You don't line your braces up vertically, which I find annoying. Take this simple function,
Code:
void takeownCB ( TCHAR* takeownCBf ) {
// only display non-empty takeown output lines
if ( _tcslen ( takeownCBf ) > 1 ) {
_tprintf(_T("TAKEOWN %s"), takeownCBf);
}
return;
// end function
}
if you line things up, it's really easy to see where one block, function, etc. starts and ends. Give your variables meaningful names, learn how to use indentation properly and lose 95% of your comments.
Yes, I got in the habit in putting the first squiggly bracket on the right, behind the function/if/do/while/for construct. Simply so it didn't muddy up things and add 1 line of code for every one of those statements. The comments help me so I prefer to keep those in.
My naming conventions come from decades of assembler programming.
My naming conventions come from decades of assembler programming.
Well, this isn't assembler programming.
It doesn't cost anything to have meaningful names. If somone were to mantain your code, using cyptic names as you're doing makes that person's job much more difficult.
Second, when D_Drmmr mentioned duplicate code, more than likely what was being spotted is where code does the same thing, except a different leter is being checked, or a different variable is being used.
For example:
Code:
if ( parmbyt[1] == _T('I') ) {
if ( pinffl != 0 )
dupparm();
pinffl = 1;
// point to parm operand - exit if no more parms
cargc++;
if ( cargc >= argc ) exit ( -1 );
This same code is duplicated, with the only difference being the letter being checked and that pinffl variable. This is where you look carefully as to what that pinffl variable is supposed to do as opposed to the similar variable used in other cases, and whether that entire block of code could be made a function that returns whether there is a duplicate or not.
Third, in that code above, you had more than one statement per line. If you were to run your code under a debugger, it is difficult to determine whether the if() statement was true or false.
Fourth, why not use or write a command-line parser in C? There is getopt() and other 'C'-based command parsers, instead of having to reinvent the wheel next time you need to parse the command-line.
Last, since you're using 'C', there seems to be no check for writing beyond the bounds of those arrays you've declared. You're using Microsoft functions to copy to buffers, but nothing checks if there is room for the copy. You should have written a set of utility functions that wrap these copy functions, so that it ensures that the copying doesn't exceed the bounds of those arrays.
Regards,
Paul McKenzie
Last edited by Paul McKenzie; February 28th, 2012 at 11:18 AM.
Meaning lengthen the function names so they are understood? Like build_drive_letter_array or something like that?
Yes.
Originally Posted by jlewand61
I thought the mainline was very structured - function calls (really subroutines). Boom boom boom on down the line.
Where do you see duplicated code???
The structure is lost because your functions are so long and because you use global variables to pass data between functions. Good structure means you can look at a small part of the program in isolation. When you use global variables that becomes nearly impossible, because you don't know what happens to those variables in the rest of the program.
In your main function, you have some code that looks like this:
Code:
while ( cargc < argc ) {
// ...
if ( parmbyt[1] == _T('I') ) {
// some common code
// some specific code
cargc++;
continue;
}
if ( parmbyt[1] == _T('T') ) {
// some common code
// some specific code
cargc++;
continue;
}
// etc
}
That is, you duplicate a lot of code and to understand the loop logic, I need to look inside each if block. The structure becomes much clearer if you rewrite this as
Code:
for (; cargc < argc; ++cargc) {
// ...
// common code
if (parmbyt[1] == _T('I')) {
// specific code
}
else if (parmbyt[1] == _T('T')) {
// specific code
}
// etc
}
Now the logic of the program is communicated much better by the code.
Maybe you can even factor out the specific code into functions and make an array of pointer to those functions. Then call it like
Yes, I got in the habit in putting the first squiggly bracket on the right, behind the function/if/do/while/for construct.
I have the same preference as GCDEF when it comes to brace placement, but your style is fine, being a common style, as long as you indent correctly and sufficiently (which you mostly do, except for a slight inconsistency in the use of spaces versus tab characters).
I think that the benefit of the indentation is somewhat lost when your functions get too long. There are no hard and fast rules as to what is too long, but if I see a function with more than say, 100 lines, I will start itching to see how I can break it down to smaller functions that do one thing and do it well. Likewise, because indentation is a visual guide for scope, global variables lessen its value.
Last edited by laserlight; February 28th, 2012 at 12:22 PM.
C + C++ Compiler: MinGW port of GCC
Build + Version Control System: SCons + Bazaar
* The Best Reasons to Target Windows 8
Learn some of the best reasons why you should seriously consider bringing your Android mobile development expertise to bear on the Windows 8 platform.