My very first C program (Windows - console)

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?

Quote:

---

Originally Posted by jlewand61

I can't believe I managed to learn this quirky language after all these years.

Critique away and help me build good coding habits.

---

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.4

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:

Code:

---

void bdla ();
int fdla ( TCHAR* fdlal );
void bsopta ();
...

---

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.

Quote:

---

Originally Posted by ovidiucucu

This is far, far away from a "My very first C program".
Are you joking or what?

---

????

Quote:

---

Originally Posted by D_Drmmr

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.4

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:

Code:

---

void bdla ();
int fdla ( TCHAR* fdlal );
void bsopta ();
...

---

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.

Where do you see duplicated code???

Quote:

---

Originally Posted by jlewand61

????

---

Code:

---

#include <stdio.h>

int main()
{
  printf("Hello World");
}

---

That would be more like a "very first C program".

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.

Quote:

---

Originally Posted by Paul McKenzie

Code:

---

#include <stdio.h>

int main()
{
  printf("Hello World");
}

---

That would be more like a "very first C program".

Regards,

Paul McKenzie

---

I think I did do one of those.

Quote:

---

Originally Posted by GCDEF

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?

Quote:

---

Originally Posted by jlewand61

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
}

---

change the indentation

Code:

---

void takeownCB ( TCHAR* takeownCBf )
{
        if ( _tcslen ( takeownCBf ) > 1 )
        {
                _tprintf(_T("TAKEOWN  %s"), takeownCBf);
        }
        return;
}

---

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.

Well you did say "Critique away and help me build good coding habits.".

Quote:

---

Originally Posted by jlewand61

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

Quote:

---

Originally Posted by jlewand61

Meaning lengthen the function names so they are understood? Like build_drive_letter_array or something like that?

---

Yes.

Quote:

---

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

Code:

---

FuncPtr functionTable[256] = { 0 };
functionTable['I'] = functionForI;
functionTable['T'] = functionForT;
// etc

for (; cargc < argc; ++cargc) {
    // ...
    // common code
    functionTable[parmbyt[1]]();
}

---

Quote:

---

Originally Posted by jlewand61

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.