I can't believe I managed to learn this quirky language after all these years.
Critique away and help me build good coding habits.
Printable View
I can't believe I managed to learn this quirky language after all these years.
Critique away and help me build good coding habits.
This is far, far away from a "My very first C program".
Are you joking or what?
Macro's are evil, even in C. So if you have to use them, better make misuse as difficult as possible. See http://www.parashift.com/c++-faq-lit....html#faq-39.4Quote:
Originally Posted by jlewand61
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.Code:void bdla ();
int fdla ( TCHAR* fdlal );
void bsopta ();
...
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.
????Quote:
Originally Posted by ovidiucucu
Quote:
Originally Posted by D_Drmmr
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.
Where do you see duplicated code???
Quote:
Originally Posted by jlewand61
That would be more like a "very first C program".Code:#include <stdio.h>
int main()
{
printf("Hello World");
}
Regards,
Paul McKenzie
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.
I think I did do one of those.Quote:
Originally Posted by Paul McKenzie
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?Quote:
Originally Posted by GCDEF
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.Quote:
Originally Posted by jlewand61
You don't line your braces up vertically, which I find annoying. Take this simple function,
change the indentationCode: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.Code:void takeownCB ( TCHAR* takeownCBf )
{
if ( _tcslen ( takeownCBf ) > 1 )
{
_tprintf(_T("TAKEOWN %s"), takeownCBf);
}
return;
}
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.
Well you did say "Critique away and help me build good coding habits.".
Well, this isn't assembler programming.Quote:
Originally Posted by jlewand61
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:
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.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 );
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
Yes.Quote:
Originally Posted by jlewand61
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.Quote:
Originally Posted by jlewand61
In your main function, you have some code that looks like this:
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 asCode: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
}
Now the logic of the program is communicated much better by the code.Code:for (; cargc < argc; ++cargc) {
// ...
// common code
if (parmbyt[1] == _T('I')) {
// specific code
}
else if (parmbyt[1] == _T('T')) {
// specific code
}
// etc
}
Maybe you can even factor out the specific code into functions and make an array of pointer to those functions. Then call it like
Code:FuncPtr functionTable[256] = { 0 };
functionTable['I'] = functionForI;
functionTable['T'] = functionForT;
// etc
for (; cargc < argc; ++cargc) {
// ...
// common code
functionTable[parmbyt[1]]();
}
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).Quote:
Originally Posted by jlewand61
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.
I don't see any point in breaking a function into smaller pieces just because it's too long. It's one logical concept. You end up massaging and making things more complex for no real benefit.
I appreciate everyone's comments.
The benefit is that you end up with smaller pieces that are easier to verify correct by inspection, and which can be more easily unit tested. Besides this, the abstraction means that a reader can more easily "ignore" parts of the functions that calls these smaller pieces because he/she already knows what they do, and either knows that they are correct or will verify their correctness if need be.Quote:
Originally Posted by jlewand61
I'm curious, did you really come here for advice on your first program? You seem to be finding reasons to disagree with everything we're saying.Quote:
Originally Posted by jlewand61
Beside bad coding style observations (some of them already pointed above) like bad naming, global variables, ignored warnings, too complex functions, "java-style" brackets, reinvented wheels and so on, I have an additional one.
Let's make a simple program based on few lines from your code (being closer to "my very first C program" ;)).
My_Test_Console.cpp compiles with no problem.Code:// My_Test_Console.cpp
#include <windows.h>
#include <tchar.h>
#define MAXDLA 40 // max # entries in drive-letter array
#define MAXSOPTA 40 // max # entries in space option file array
TCHAR TCHbuff[MAX_PATH]; // GetLogicalDriveStrings
TCHAR dlal[MAXSOPTA][3]; // driveletter 00 - NO COLON
TCHAR dlay[MAXDLA][12]; // drive type - FIXED -room for REMOVABLE?
int dlat = 0; // num of entries - or next element to populate
int _tmain(int argc, _TCHAR* argv[])
{
// populate driveletter and drive type
dlal[dlat][0] = TCHbuff[0];
dlal[dlat][1] = _T('\0');
_tcscpy_s ( dlay[dlat], _T("FIXED") );
return 0;
}
However, if change the source file extension from ".cpp" to ".c" telling Visual Studio to use C compiler it gives an error.
Why the C compiler complains while the C++ one has nothing to say?
Let's take a look in _tcscpy_s (strcpy_s, wcscpy_s) documentation: http://msdn.microsoft.com/en-us/libr...(v=VS.80).aspx
We can notice overloaded functions strcpy_s/wcscpy_s.
In the above program, we've passed two parameters, then C++ compiler choses the (template) one which requires two parameters.
The C compiler gives an error because it expects three parameters.
To keep in mind
Unlike C++, C language has not:
- function overloading;
- template functions;
Conclusion
- if write a C-style program, does not mean it will be always obviously C compiled;
- although related, C an C++ are different programming languages;
- everytime we are using a library function, we have to pay a serious attention to its documentation.
Speaking of this, jlewand61, in a message board community that you participated in, I responded to one of your threads with this:Quote:
Originally Posted by GCDEF
Quote:
Originally Posted by jlewand
Quote:
Originally Posted by jlewand
Maybe you would like to keep an open mind about the suggestions made here? If you are indeed experiencing a "culture shock", there may be an automatic reaction to fall back on what you know from your assembly language experience, even if it does not necessarily apply when programming in C (or C++).Quote:
Originally Posted by laserlight
Yeah, unless you're talking about COBOL or something, recursion and local variables shouldn't be an alien concept. They're pretty common in just about all modern high-level languages that I know of. Recursion is a basic theme in any algorithms course.Quote:
Originally Posted by laserlight
Recursion is an alien concept from a programming standpoint. In all the assemblers I've worked on, there is no such thing as "local" or "global" variables. Variables are simply addressible storage.
Not from a programming standpoint it isn't. It's a fundamental concept taught early in any programming course. Assembly programming is a long way removed from any higher level language. If that's all you have experience with, you'll need to wipe your brain clean when it comes to approaching problems and start again.Quote:
Originally Posted by jlewand61
We are talking here about C and C++ languages and not about Assembly.Quote:
Originally Posted by jlewand61
Experience, not "reasons". It shouldn't bother anyone whether I take advice or not. It wouldn't bother me if someone deflected my advice. I've sincerely asked to help me get into good C coding habits. C is a new animal and I'm trying to develop good habits from the start.Quote:
Originally Posted by GCDEF
I was resopnding to the comment that recursion or local/global variables should not be foreign concepts. They ARE. Quite so.Quote:
Originally Posted by ovidiucucu
Well then, just forget the old habits.Quote:
Originally Posted by jlewand61
Like somebody already said, C is not Assembly and vice versa.
In practically every single mid to high level language, there are such things as local and global variables. Whether it is C, C++, Pascal, FORTRAN, VB, C#, Java, I could go on and on listing all the languages where local and global variables exist.Quote:
Originally Posted by jlewand61
In none of these languages is it desirable to "globalize" your variables to such an extent as your code does. It isn't just the few voices here saying it, every expert will tell you the same thing.
Regards,
Paul McKenzie
Just to note: At least MASM does support local variables since a bunch of versions. I've never used that feature myself, though, since I never felt the urge to write anything in assembler that needs local variables or does recursion.Quote:
Originally Posted by jlewand61
And local variables are even so non-strange that there's even hardware support for them to some extent in some CPU architectures (including the x86, BTW ;)).
If a function is long, yet it serves a single purpose and does not include code duplication, then I tend to agree. Separating code just to make each function fit on a single screen doesn't help readability IMO.Quote:
Originally Posted by jlewand61
However, if there is code duplication like in various places in your code, then that should be factored out even in small functions. This makes the program easier to maintain, test and understand.
Something that hasn't been mentioned (unless I missed it) is that several of your functions have multiple exit points (i.e. return statements). Also, you are calling exit in quite a lot of places. In a C++ program that would be OK, since resources should be managed using RAII classes anyway. But in C that's not possible, so having multiple exit points makes it very hard to properly manage resources in all possible execution paths.
I'm well aware that many languages have local/global variables. I've never ever had to address this concept until a month ago.Quote:
Originally Posted by Paul McKenzie
What should I do when many of these arrays are accessed in multiple functions?
The usual approach is to pass to a function only what it needs to do its work, so you would pass a pointer to the first element of the array as an argument when calling a function that accesses the elements of the array. The array itself would be local to a function, e.g., the main function. It may be the case that say, two arrays are logically part of some kind of object, in which case you might define a struct containing these two arrays, then pass a pointer to the struct instead.Quote:
Originally Posted by jlewand61
It bothers us because we are taking the time to review your code, and tell you what we think about it, and how to make it better. Yet you seem bent on ignoring what is said, hence our time is wasted...Quote:
Originally Posted by jlewand61
----
That said, if you want my advice about trying to move from assembler to C, then I'd actually suggest you learn C++ !
For you, C may only feel like an evolution from assembly, making it hard to shake your old habits, as it allows you to do things "the good old assembly way"
C++ won't take that crap. If you take the time to learn some if it correctly, chances are it will shift your mindset, making it more prepared to learn the C way. C++ will teach you proper encapsulation, abstraction etc.
Once you've learned those, you'll realize they can also be used in C, and will strive to use them.
That and learning C++ is also useful. IMO learning C => C++: bad idea. C++ => C works perfectly fine.
My 0.02$
One of the quotes I used to have pinned over my desk.Quote:
Originally Posted by jlewand61
Quote:
"There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies."
C.A.R. Hoare
Yeah. If you're going to forget everything you know and start over, it's best to only do it once.Quote:
Originally Posted by monarch_dodra
Well, if you think that's a big step, wait till you get to programming multithreaded applications. In that paradigm, excessive globalizing of variables is a death blow.Quote:
Originally Posted by jlewand61
I also agree that if your real goal is to learn C++ (and not actually just 'C'), then you need to learn C++. If you are learning 'C' to prepare yourself to learn C++, well, don't. Go directly to C++.
Regards,
Paul McKenzie