New C programmer

[Mansoor]

Hi,

I recently (1 week) started to learn C language. I have been reading a
book about C and I've been using Turbo C++ in DOS. After I learnt the
basic concepts I decided to write a demo program which is about a very
simple linked-list managing.
I've included my code here. I just want to ask those who are
experienced in C , to take a bit of their time, have a look at this
code and write bac for me, what problems and error it has (in
programming techniques view like commenting, organizing the code,
etc). I believe if I write millions of lines but nobody tell me my
mistakes, I won't learn anything.

Code:

/* A Linked List Manager with Add, Delete, Display */
/* functionalities. */

#include <stdio.h>
#include <stdlib.h>

/* Function Prototypes */
char get_comm(struct s_l_list *ll_p);
void show_list(struct s_l_list *ll_p);
void add_elem(struct s_l_list *ll_p);
struct s_ll_rec *search_elem(struct s_l_list *ll_p, unsigned int id);
void del_elem(struct s_l_list *ll_p);
/* ------------- */
/* Data Storages */
/* ------------- */
/* Phone book record */
struct s_user_rec
{
unsigned int id; /* User ID */
char *username, /* User Name */
*phone; /* User Phone Number */
};
/* Linked list's record */
struct s_ll_rec
{
struct s_user_rec *data;/* Phone Book Record */
struct s_ll_rec *next; /* Next Linked List Record */
};
/* Linked list */
struct s_l_list
{
struct s_ll_rec *head; /* First List Element */
int len; /* Number of Records */
};
typedef struct s_user_rec user_rec;
typedef struct s_l_list l_list;
typedef struct s_ll_rec ll_rec;
/* Main Program */
main()
{
char com; /* command temp */
l_list list = { NULL , 0 }; /* Linked List */

/* Input Command and call functions */
while (1)
{
com = get_comm(&list); /* Get Command */
switch (com)
{
case 'D':
{
show_list(&list);
break;
}
case 'A':
{
add_elem(&list);
break;
}
case 'R':
{
del_elem(&list);
break;
}
case 'S':
{
}
case 'Q':
{
/* free the memory reserved for list */
if (list.head != NULL)
{
ll_rec *lt = list.head;
do
{
free(lt);
} while ((lt = lt->next) != NULL);
}
printf("\nThanks for using our program.");
exit(0);
}
}
}
return 0;
}
/* Diplaying the command list and read the commmand.
Prototype: char get_comm(l_list *ll_p)
Arguments: ll_p: pointer to linked list
Returns: the command character:
'D': display the list
'A': add an element to the list
'R': remove an element from the list
'S': search an element through the list
'Q': quit the program */
char get_comm(struct s_l_list *ll_p)
{
char buf[80]; /* Input Buffer */
/* Display the Command List */
while (1)
{
puts("\nCommand List:");
printf("\tCurrent list members: %d", ll_p->len);
printf("\n\t(D) Display the whole list.\
\n\t(A) Add an element to list.\
\n\t(R) Remove an element from list.\
\n\t(S) Search for an element.\
\n\t(Q) Quit the program.\
\n>> ");
gets(buf);
/* Check for command */
switch (buf[0])
{
case 'D':
;
case 'A':
;
case 'R':
;
case 'S':
;
case 'Q':
return buf[0];
}
}
}
/* Showing the entire list
Protoptype: void show_list(struct s_l_list *ll_p)
Arguments: ll_p: pointer to linked list
Returns: none */
void show_list(struct s_l_list *ll_p)
{
/* Check if list is empty or not */
if (ll_p->head != NULL)
{
struct s_ll_rec *cur = ll_p->head; /* Current Record */
/* Print Heading */
printf("\nLinked List:");
do
{
printf("\n (%s) has ID of (%u) and no. of (%s).",
cur->data->username, cur->data->id, cur->data->phone);
} while ((cur = cur->next) != NULL);
}
}
/* Add an element to linked list
Prototype: void add_elem(struct s_l_list *ll_p)
Arguments: ll_p: pointer to link list
Returns: none */
void add_elem(struct s_l_list *ll_p)
{
static unsigned long id;/* ID generator var */
user_rec *user; /* User Information Storage */
ll_rec *llr_p; /* List Record */
/* Set aside memory for objects */
user = (user_rec *) malloc(sizeof(user_rec));
llr_p = (ll_rec *) malloc(sizeof(ll_rec));
user->username = (char *) malloc(sizeof(char) * 20);
user->phone = (char *) malloc(sizeof(char) * 20);
/* Input the information from user */
printf("\nAdding a new record to list:\
\n Enter your username:");
scanf("%s", user->username);
printf("\n Enter your phone:");
scanf("%s", user->phone);
/* Add the record */
user->id = id > 100000 ? (id = 0) : id++;
llr_p->data = user;
llr_p->next = ll_p->head; /* set new element's following to first
element */
ll_p->head = llr_p; /* set first element to new one */
ll_p->len += 1; /* increase lenght by 1 */
/* Output success message */
printf("\n New record was successfully added to list.\
\n Record Info:\
\n ID: %u\
\n Username: %s\
\n Phone Number: %s", user->id, user->username, user->phone);
}
/* Search id element in a link list and return its first occurnce.
Prototype: struct s_ll_rec *search_elem(struct s_l_list *ll_p
, unsigned int id)
Arguments: ll_p: pointer to link list
id: id to search
Returns: a pointer to the found record, otherwise NULL */
struct s_ll_rec *search_elem(struct s_l_list *ll_p, unsigned int id)
{
/* check if list is empty or not */
if (ll_p->head == NULL)
return NULL;
struct s_ll_rec *cur = ll_p->head; /* Current record */
do
{
/* if id matches, return the pointer */
if (cur->data->id == id)
return cur;
} while ((cur = cur->next) != NULL);
/* otherwise return NULL */
return NULL;
}
/* Delete an element from list.
Prototype: void del_elem(struct s_l_list *ll_p)
Arguments: ll_p: pointer to linked list
Returns: none */
void del_elem(struct s_l_list *ll_p)
{
char sucess = 0; /* flag var */
unsigned int find_id; /* temp id var to hold search id */
struct s_ll_rec *rec_t, /* pointer to hold the found result, */
*prev; /* pervious one */
/* Input the id to search */
printf("\nEnter the ID to delete:");
scanf("%u", &find_id);
/* check if the record with find_id exists */
if ((rec_t = search_elem(ll_p, find_id)) != NULL)
{
/* check if it is the first record */
if (ll_p->head == rec_t)
ll_p->head = rec_t->next;
/* if not, get the previous record */
else
{
prev = ll_p->head;
do
if (prev->next == rec_t)
{
prev->next = rec_t->next;
sucess = 1; /* End the loop */
}
while (((prev = prev->next) != NULL) && (sucess == 0));
}
/* free the memory occupied by the record */
free(rec_t);
ll_p->len -= 1; /* decrease list's length by 1 */
printf("\nRecord was successfully deleted.");
}
/* if record doesn't exist, show a message saying so */
else
printf("\nNo record with ID %u was found.", find_id);
}
End of Code

 

[Richard Heathfield]

I've included my code here. I just want to ask those who are
experienced in C , to take a bit of their time, have a look at this
code and write bac for me, what problems and error it has (in
programming techniques view like commenting, organizing the code,
etc). I believe if I write millions of lines but nobody tell me my
mistakes, I won't learn anything.

Just about everything I write after this point is a criticism, so I thought
it'd be kinder to warn you of this in advance.

If you have really been writing C for only a week, I am surprised at how
quickly you've learned it. Unfortunately, you seem to have picked up some
bad habits really early on! Please understand that everything I write in
the remainder of this reply is intended to *help* you to learn more about C
and how to become a better C programmer.

Okay, grit your teeth...

/* Function Prototypes */
char get_comm(struct s_l_list *ll_p);

ll_p isn't very descriptive. Nor is get_comm. Try to pick more descriptive
names.

void show_list(struct s_l_list *ll_p);

For example, show_list is a good name. It's obvious what it does.

/* Main Program */
main()

int main(void)

{
char com; /* command temp */

When posting to Usenet, replace all tabs with an appropriate number of
spaces. As you can see, tabs tend not to port very well.

l_list list = { NULL , 0 }; /* Linked List */

/* Input Command and call functions */
while (1)

This loop is, at best, misleading, since it suggests the program will run
forever (or, at least, until someone flips the power button).

Presumably, you have some criterion for deciding whether to end the program.
It would make a lot of sense to put the test for that condition into your
while loop.

/* free the memory reserved for list */
if (list.head != NULL)
{
ll_rec *lt = list.head;
do
{
free(lt);
} while ((lt = lt->next) != NULL);

Having freed lt, you can't safely examine lt->next. Use a temp to keep hold
of the next link:

ll_rec *lt = list.head;
ll_rec *tmp;
do
{
tmp = lt->next;
free(lt);
lt = tmp;
} while(lt != NULL);

char get_comm(struct s_l_list *ll_p)
{
char buf[80]; /* Input Buffer */
/* Display the Command List */
while (1)

See above.

gets(buf);

Never use this function. It can't stop your user typing 81, 82, 83, or
perhaps a few hundred thousand bytes into your 80-byte buffer. This is
known as a buffer overrun (or overflow, depending on who you talk to). It's
not just a bug; it's actually a security weakness.

Look up fgets instead. fgets takes a buffer size argument, so that you can
tell it how much space it can safely use.

/* Set aside memory for objects */
user = (user_rec *) malloc(sizeof(user_rec));

Cleaner, simpler, better:

user = malloc(sizeof *user);


In general, where p is a pointer to some object type T (i.e. the declaration
resembles T *p, and you wish to allocate N objects of that type, you can
do this:

p = malloc(N * sizeof *p);

If you later change your mind and use a different object type, the malloc
call can be left entirely alone.

Note that no cast is required (unless you are using a C++ compiler, in which
case why are you using malloc anyway? For most C++ purposes, new is a
better bet). In C, the cast is just noise, and it's a good idea to reduce
noise to a minimum in C programs, to maximise the chance of reading the
signal clearly.

llr_p = (ll_rec *) malloc(sizeof(ll_rec));

llr_p = malloc(sizeof *llr_p);

user->username = (char *) malloc(sizeof(char) * 20);

Don't do this until you have ensured that user != NULL

Once you've ensured that, do this:

user->username = malloc(sizeof *user->username * 20);

Better still, replace 20 with a variable whose value you first set to 20.

user->phone = (char *) malloc(sizeof(char) * 20);

Is this the same 20 as the other 20, or a different 20? If it's the same 20,
use the same variable as before. Otherwise, use a different one.

user->phone = malloc(sizeof *user->phone * init_phone_len);

Always check that every malloc call returns a non-NULL value. This is part
of defensive programming. One of the reasons modern software crashes so
often is that a lot of people have forgotten, or have never learned, how to
program defensively.

/* Input the information from user */
printf("\nAdding a new record to list:\
\n Enter your username:");
scanf("%s", user->username);

This is no better than gets(). Either learn how to use scanf properly or, if
that seems too much like hard work, use fgets instead.



That's enough for now.

<big snip>

 

[Nick Austin]

Hi,

I recently (1 week) started to learn C language. I have been reading a
book about C and I've been using Turbo C++ in DOS. After I learnt the
basic concepts I decided to write a demo program which is about a very
simple linked-list managing.
I've included my code here. I just want to ask those who are
experienced in C , to take a bit of their time, have a look at this
code and write bac for me, what problems and error it has (in
programming techniques view like commenting, organizing the code,
etc). I believe if I write millions of lines but nobody tell me my
mistakes, I won't learn anything.

It's quite a good program. Linked lists are not easy to write but
apart from the memory leak you appear to have got it right. It's
not quite portable but only required two changes to work on my
system.

Style point. You could do with more blank lines to separate the
various parts of the the program (e.g. to separate declarations
and statements).

Code:

/* A Linked List Manager with Add, Delete, Display */
/* functionalities. */

#include <stdio.h>
#include <stdlib.h>

You're using forward declarations of struct s_l_list in the
function prototypes. You need to either move the function
prototypes after the structure declarations, or provide
an 'incomplete declaration' here in the form of:

struct s_l_list;

/* Function Prototypes */
char get_comm(struct s_l_list *ll_p);
void show_list(struct s_l_list *ll_p);
void add_elem(struct s_l_list *ll_p);
struct s_ll_rec *search_elem(struct s_l_list *ll_p, unsigned int id);
void del_elem(struct s_l_list *ll_p);

/* ------------- */
/* Data Storages */
/* ------------- */

/* Phone book record */
struct s_user_rec
{
unsigned int id; /* User ID */

Style point. If the variable holds the user ID then why
not call it user_id? Then you don't need the comment.

char *username, /* User Name */
*phone; /* User Phone Number */

Style point. I don't like using the comma separator in declarations
or definitions as IMO it makes programs harder to maintain. Better
to do:

char *username;
char *phone;

};

/* Linked list's record */
struct s_ll_rec
{
struct s_user_rec *data;/* Phone Book Record */
struct s_ll_rec *next; /* Next Linked List Record */
};

/* Linked list */
struct s_l_list
{
struct s_ll_rec *head; /* First List Element */
int len; /* Number of Records */
};

typedef struct s_user_rec user_rec;
typedef struct s_l_list l_list;
typedef struct s_ll_rec ll_rec;

/* Main Program */
main()

int main( void )

{
char com; /* command temp */
l_list list = { NULL , 0 }; /* Linked List */

/* Input Command and call functions */
while (1)
{
com = get_comm(&list); /* Get Command */
switch (com)
{
case 'D':
{
show_list(&list);
break;
}
case 'A':
{
add_elem(&list);
break;
}
case 'R':
{
del_elem(&list);
break;
}
case 'S':
{
}
case 'Q':
{
/* free the memory reserved for list */

Style point. I would make the following 'free' code into a separate
function.

if (list.head != NULL)
{
ll_rec *lt = list.head;
do
{
free(lt);

This leaks. Before this line should be:

free( lt->data->username );
free( lt->data->phone );
free( lt->data );

} while ((lt = lt->next) != NULL);

lt->next is an invalid pointer derefence, You have to save the
value of lt->next to a temporary variable before the free().

Also I dislike assignments inside conditionals. The entire
loop fixed is:

do
{
ll_rec *next = lt->next;

free( lt->data->username );
free( lt->data->phone );
free( lt->data );
free( lt );

lt = next;
} while( lt != NULL );

Also, if you made this a straight "while () {};" loop you avoid
the need for the line "if (list.head != NULL)" above.

}
printf("\nThanks for using our program.");

The output stream is buffered, so you need to ensure that the
list character sent to the stream is a newline:

printf("\nThanks for using our program.\n" );

Or, if you have a reason to not provide a final newline add:

fflush(stdout);

exit(0);
}
}
}
return 0;
}

/* Diplaying the command list and read the commmand.
Prototype: char get_comm(l_list *ll_p)
Arguments: ll_p: pointer to linked list
Returns: the command character:
'D': display the list
'A': add an element to the list
'R': remove an element from the list
'S': search an element through the list
'Q': quit the program */

char get_comm(struct s_l_list *ll_p)
{
char buf[80]; /* Input Buffer */

/* Display the Command List */
while (1)
{
puts("\nCommand List:");
printf("\tCurrent list members: %d", ll_p->len);
printf("\n\t(D) Display the whole list.\
\n\t(A) Add an element to list.\
\n\t(R) Remove an element from list.\
\n\t(S) Search for an element.\
\n\t(Q) Quit the program.\
\n>> ");
gets(buf);

Use fgets().

Also I would add this line:

buf[0] = toupper( buf[0] );

/* Check for command */
switch (buf[0])
{
case 'D':
;
case 'A':
;
case 'R':
;
case 'S':
;
case 'Q':
return buf[0];
}
}
}

/* Showing the entire list
Protoptype: void show_list(struct s_l_list *ll_p)
Arguments: ll_p: pointer to linked list
Returns: none */

void show_list(struct s_l_list *ll_p)
{
/* Check if list is empty or not */
if (ll_p->head != NULL)
{
struct s_ll_rec *cur = ll_p->head; /* Current Record */

/* Print Heading */
printf("\nLinked List:");
do
{
printf("\n (%s) has ID of (%u) and no. of (%s).",
cur->data->username, cur->data->id, cur->data->phone);
} while ((cur = cur->next) != NULL);

Style point. Don't do assignments inside while loops:

cur = cur->next;
} while (cur != NULL );

}
}

/* Add an element to linked list
Prototype: void add_elem(struct s_l_list *ll_p)
Arguments: ll_p: pointer to link list
Returns: none */

void add_elem(struct s_l_list *ll_p)
{
static unsigned long id;/* ID generator var */
user_rec *user; /* User Information Storage */
ll_rec *llr_p; /* List Record */
/* Set aside memory for objects */
user = (user_rec *) malloc(sizeof(user_rec));

You don't need to cast the return from malloc().

llr_p = (ll_rec *) malloc(sizeof(ll_rec));
user->username = (char *) malloc(sizeof(char) * 20);

This allocates more memory than you use. you could read the
username and phone number into temporary variables, then
malloc space the exact space that they require (strlen() + 1).

user->phone = (char *) malloc(sizeof(char) * 20);

/* Input the information from user */
printf("\nAdding a new record to list:\
\n Enter your username:");
scanf("%s", user->username);

Use fgets().

printf("\n Enter your phone:");
scanf("%s", user->phone);

Use fgets().

/* Add the record */
user->id = id > 100000 ? (id = 0) : id++;

Style point. I wouldn't put two assigments in one statement.
Change it to:

id = ( id > 100000 ) ? 0 : id + 1;
user->id = id;

llr_p->data = user;
llr_p->next = ll_p->head; /* set new element's following to first
element */
ll_p->head = llr_p; /* set first element to new one */
ll_p->len += 1; /* increase lenght by 1 */

/* Output success message */
printf("\n New record was successfully added to list.\
\n Record Info:\
\n ID: %u\
\n Username: %s\
\n Phone Number: %s", user->id, user->username, user->phone);
}

/* Search id element in a link list and return its first occurnce.
Prototype: struct s_ll_rec *search_elem(struct s_l_list *ll_p
, unsigned int id)
Arguments: ll_p: pointer to link list
id: id to search
Returns: a pointer to the found record, otherwise NULL */

struct s_ll_rec *search_elem(struct s_l_list *ll_p, unsigned int id)
{
/* check if list is empty or not */
if (ll_p->head == NULL)
return NULL;
struct s_ll_rec *cur = ll_p->head; /* Current record */
do
{
/* if id matches, return the pointer */
if (cur->data->id == id)
return cur;
} while ((cur = cur->next) != NULL);

/* otherwise return NULL */
return NULL;
}

/* Delete an element from list.
Prototype: void del_elem(struct s_l_list *ll_p)
Arguments: ll_p: pointer to linked list
Returns: none */

void del_elem(struct s_l_list *ll_p)
{
char sucess = 0; /* flag var */
unsigned int find_id; /* temp id var to hold search id */
struct s_ll_rec *rec_t, /* pointer to hold the found result, */
*prev; /* pervious one */

/* Input the id to search */
printf("\nEnter the ID to delete:");
scanf("%u", &find_id);

/* check if the record with find_id exists */
if ((rec_t = search_elem(ll_p, find_id)) != NULL)
{
/* check if it is the first record */
if (ll_p->head == rec_t)
ll_p->head = rec_t->next;
/* if not, get the previous record */
else
{
prev = ll_p->head;
do
if (prev->next == rec_t)
{
prev->next = rec_t->next;
sucess = 1; /* End the loop */
}
while (((prev = prev->next) != NULL) && (sucess == 0));
}

Style point. It's more efficient to integrate the initialis search
into the delete function. This way the search can maintain two
pointers (the current and previous records) so there is no need
to perform a separate search.

/* free the memory occupied by the record */

Also leaks memory. Add here:

free( rec_t->data->username );
free( rec_t->data->phone );
free( rec_t->data );

free(rec_t);
ll_p->len -= 1; /* decrease list's length by 1 */

Why do you keep the length of the list as a variable? It is only
ever used once and makes the code harder to maintain. You could
just as easily write a function that counts the number of list
elements.

printf("\nRecord was successfully deleted.");
}
/* if record doesn't exist, show a message saying so */
else
printf("\nNo record with ID %u was found.", find_id);
}
End of Code

Nick.

 

[Peter Nilsson]

Mansoor wrote: ....

This loop is, at best, misleading, since it suggests the program will run
forever (or, at least, until someone flips the power button).

I agree that it hides the termination condition, but I wouldn't say it's
misleading, and certainly not 'at best'. YMMV (and obviously does

Having freed lt, you can't safely examine lt->next. Use a temp to keep hold
of the next link:

ll_rec *lt = list.head;
ll_rec *tmp;
do
{
tmp = lt->next;

What if lt is a null pointer?

free(lt);
lt = tmp;
} while(lt != NULL);

ll_rec *lt, next = list.head;
while ((lt = next) != NULL)
{
next = lt->next;
free(lt);
}
list.head = NULL;

 

[Richard Heathfield]

I agree that it hides the termination condition, but I wouldn't say it's
misleading, and certainly not 'at best'. YMMV (and obviously does

MMDIV!!

What if lt is a null pointer?

Oh pooh. Good spot. But now it's my turn...

ll_rec *lt, next = list.head;

This defines next as an ll_rec, not as an ll_rec *, so the assignment is
wrong. You meant:

ll_rec *lt, *next = list.head;

 

[Tom Zych]

Note that no cast is required (unless you are using a C++ compiler, in which
case why are you using malloc anyway? For most C++ purposes, new is a
better bet). In C, the cast is just noise, and it's a good idea to reduce
noise to a minimum in C programs, to maximise the chance of reading the
signal clearly.

Another reason: if you cast, and forget to #include <stdlib.h>, the
compiler will assume you want to cast int to a pointer (int is the
default return type for an undeclared function), and you will get
undefined behavior.

 

[Mansoor]

Nick Austin wrote in message:

} while ((lt = lt->next) != NULL);

Also I dislike assignments inside conditionals. The entire
loop fixed is:

Nick, I have been reading "Teach Yourself C programming in 21 days" by
sams. I got all these ideas of putting assignements in loops from
there. Have you read that book? What do you reckon about teaching
someone with these technique from the beginning?( if you think it is
not a good way).

char get_comm(struct s_l_list *ll_p)
{
char buf[80]; /* Input Buffer */

/* Display the Command List */
while (1)
{
puts("\nCommand List:");
printf("\tCurrent list members: %d", ll_p->len);
printf("\n\t(D) Display the whole list.\
\n\t(A) Add an element to list.\
\n\t(R) Remove an element from list.\
\n\t(S) Search for an element.\
\n\t(Q) Quit the program.\
\n>> ");
gets(buf);

Use fgets().

I couldn't actually figure out why using gets() is not appropriate. Is
it because it just overwrites the memory with no warning?

You don't need to cast the return from malloc().

Actually Nick, I use Borland Turbo C++ and it requires the cast. (I'm
not sure if this is the only way)

Style point. I wouldn't put two assigments in one statement.
Change it to:

Is it just a style or does it make a real difference? I mean if
somebody can tell if you are good at C by examining this expression.
you know what I mean ?

id = ( id > 100000 ) ? 0 : id + 1;
user->id = id;

Thanks nick for you time.

 

[Mark Gordon]

} while ((lt = lt->next) != NULL);

Also I dislike assignments inside conditionals. The entire
loop fixed is:

Nick, I have been reading "Teach Yourself C programming in 21 days" by
sams.[/QUOTE]

I've seen people suggest that this is not a good book. I've never used
it myself so I don't know how valid the complaints are. However, if you
are learning C you should get a copy of K&R2 (The C Programming Language
Second Edition by Brian Kernighan & Dennis Ritchie).

I got all these ideas of putting assignements in loops from
there. Have you read that book? What do you reckon about teaching
someone with these technique from the beginning?( if you think it is
not a good way).

Doing assignments in conditions is not that unusual in C, and is
sometimes done by mistake, so it is something that you really need to
learn. However, you also have to judge whether it makes it harder to
read the code in any given instance.

char get_comm(struct s_l_list *ll_p)
{
char buf[80]; /* Input Buffer */

/* Display the Command List */
while (1)
{
puts("\nCommand List:");
printf("\tCurrent list members: %d", ll_p->len);
printf("\n\t(D) Display the whole list.\
\n\t(A) Add an element to list.\
\n\t(R) Remove an element from list.\
\n\t(S) Search for an element.\
\n\t(Q) Quit the program.\
\n>> ");
gets(buf);

Use fgets().

I couldn't actually figure out why using gets() is not appropriate. Is
it because it just overwrites the memory with no warning?

Yes, it is because it overwrite memory without any warning. This is a
massive security hole in any application, and one which *has* been used
to crack systems in the past.

Actually Nick, I use Borland Turbo C++ and it requires the cast. (I'm
not sure if this is the only way)

You need to ask on a Turbo C news group or read the instructions more
carefully. Turbo C is currently trying to compile your code as C++ code
and this can lead to all sorts of problems. You should also find out how
to get Turbo C++ to compile your code in ISO code (no extensions) and
with a high level of warnings so that the compiler will tell you as
about as many mistakes as possible.

The specifics of how to use Turbo C (or any other environment) are off
topic in this group.

Is it just a style or does it make a real difference? I mean if
somebody can tell if you are good at C by examining this expression.
you know what I mean ?

It is a style point.

Always remember that someone else may have to read your code and
maintain it. That someone may even be you in 10 years time when you have
forgotten the program.

I'm suspicious that the OP may not have written what was actually
wanted. Was an id of 100001 really intended to be valid?