Help in this programm in C

D

deppy_3

Hi.I am started learning Programm language C before some time.I am
trying to make a programm about a very simple "sell shop".This programm
hasn't got any compile problem but when i run it i face some other
ploblems which i can not correct.I would appreciated if someone take a
look at my programm so as to help me.I tried many times to find out
where my mistakes are but i didn't manage something.
I have made some comments due to the programm so as to make it more
easy to as.
Any recommendations about my writing style in the programm are
acceptable.
Please help me!!!

Programm


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

/* Functions*/
int menu(void);
struct sells *insert(void);
struct sells *search(void);
int show(struct sells *list);
int exit(void);

/*Struct*/
struct sells{
char *tittle;
char *singer;
int price;
int count;
struct sells *next;
};

struct sells *list;


struct sells *alloc(void)
{
return(struct sells*)malloc(sizeof(struct sells));
}


/*Function main*/
int main(void)
{
menu();
return 0;
}



/*Function menu.In this function the user has the opportunity to choose

what he/she want to do*/
int menu(void)
{
int i;

printf("---------MENU---------\n");
printf("Press \"1\" to insert data\n");
printf("Press \"2\" to search data\n");
printf("Press \"3\" to show data\n");
printf("Press \"4\" to exit from programm\n");
putchar('\n\n');

printf("Make your choice\t");
scanf("%d",&i);

while((i<1)&&(i>4))
{
printf("You made wrong choice.\nPlease make a new
choice\n");
scanf("%d",&i);
}

switch(i)
{
case 1:
{
insert();
break;
}
case 2:
{
search();
break;
}
case 3:
{
show(list);
break;
}
case 4:
{
exit();
break;
}
}
return i;
}



/*In this function the user insert the data.*/
struct sells *insert(void)
{
char titlos[15];
char tragoudisths[15];
int timi;
struct sells *n;
float m_out;
int i;
m_out=0;
printf("%f\n",&m_out);
n=alloc();

printf("Give the title of the CD\n");
scanf("%s",&titlos);
getchar();


printf("Give the name of the singer\n");
scanf("%s",&tragoudisths);
getchar();

printf("Give the price of the CD\n");
scanf("%d",&timi);
getchar();

while((timi<0)||(timi>100))
{
printf("You have insert a wrong price.\nGive a new
price.\n");
scanf("%d",&timi);
getchar();
}
getchar();

n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);
n->price=timi;

if(list==NULL)
{
list=alloc();
list->tittle=strdup(n->tittle);
list->singer=strdup(n->singer);
list->price=n->price;
list->next=NULL;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))==0)
{
list->count++;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))<0)
{
n=alloc();
n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);
n->price=timi;
n->count=1;
n->next=list;
list=n;

printf("%s\t\t%s\t\t%d\t\t\n",list->tittle,list->singer,list->price);
}
else
{
list->next=insert();
return list;
}
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return n;
}


/*In this function the user search for an element.Also he can modify
it.
If someone can i would like to tell me what i have to do so as to
delete an element.*/
struct sells *search(void)
{
char titlos[15];
int i,k,out;
char choice;

printf("Give the tittle of the CD that you want to find\n");
gets(titlos);
getchar();
if(list==NULL||(out=strcmp(list->tittle,titlos))>0)
{
printf("The tittle that you want doesn't exist.\n");
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return list;
}
else if(out==0)
{
printf("Tittle: \t%s\n",list->tittle);
printf("Singer: \t%s\n",list->singer);
printf("Price: \t%d\n",list->price);

printf("Do you want to make any change;\n");
printf("If Yes press \"1\" else \"0\" to return to
menu\n");
printf("Make your choice");
scanf("%d",&k);
switch(k)
{
case 1:
{
printf("Do you want ot change the tittle;\nPress Y(YES)
or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new tittle:\n");
gets(list->tittle);
getchar();
}
printf("Do you want ot change the name of the
singer;\nPress Y(YES) or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new name:\n");
gets(list->singer);
getchar();
}
printf("Do you want ot change the price of the
CD;\nPress Y(YES) or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new price:\n");
scanf("%d",&list->price);
while((list->price<0)&&(list->price>100))
{
printf("You have insert a wrong price.\nGive a new
price.\n");
scanf("%d",&list->price);
}
putchar('\n');
}
break;
}
case 0:
{
menu();
break;
}
}
}
return list;
}


/*Function which shows to the user all the element that has already
insert.
In this functio there is a problem.When i insert more the 3 elements,
these elements doesn't appear to me.I think that the problem is in my
insert
but i am not quite sure.*/
int show(struct sells *list)
{
int i,k;
k=1;
printf("\t\t-------LIST--------\n");
printf("Num\t\tTittle\t\tSinger\t\tPrice\n");
while(list!=NULL)
{

printf("%d\t%s\t\t%s\t\t%d\t\t\n",k,list->tittle,list->singer,list->price);

k++;
list=list->next;
}
printf("What do you want to do\n");
printf("Press \"1\" to retutn to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return 0;
}



/*Exit from the programm*/
int exit(void)
{
int i;

printf("Do you really want to exit from the programm;\n");
printf("If Yes press \"1\" else \"0\"\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
break;
}
return 0;
}
}
 
R

Richard Heathfield

(e-mail address removed) said:
Hi.I am started learning Programm language C before some time.I am
trying to make a programm about a very simple "sell shop".This programm
hasn't got any compile problem

I beg to differ. See below. Fix this lot, starting with the first, and then
get back to us.

(Caution: in the following diagnostic report, line numbers may differ
slightly to those in your source.)

gcc -W -Wall -ansi -pedantic -Wformat-nonliteral -Wcast-align
-Wpointer-arith -Wbad-function-cast -Wmissing-prototypes
-Wstrict-prototypes -Wmissing-declarations -Winline -Wundef
-Wnested-externs -Wcast-qual -Wshadow -Wconversion -Wwrite-strings
-Wno-conversion -ffloat-store -O2 -g -pg -c -o foo.o foo.c
foo.c:10: conflicting types for `exit'
/usr/include/stdlib.h:577: previous declaration of `exit'
foo.c:26: warning: no previous prototype for `alloc'
foo.c: In function `menu':
foo.c:52: warning: multi-character character constant
foo.c:59: warning: unknown escape sequence `\ '
foo.c: In function `insert':
foo.c:102: warning: double format, pointer arg (arg 2)
foo.c:105: warning: char format, different type arg (arg 2)
foo.c:108: warning: char format, different type arg (arg 2)
foo.c:115: warning: unknown escape sequence `\ '
foo.c:116: warning: unknown conversion type character ` ' in format
foo.c:116: warning: too many arguments for format
foo.c:120: warning: implicit declaration of function `strdup'
foo.c:120: warning: assignment makes pointer from integer without a cast
foo.c:121: warning: assignment makes pointer from integer without a cast
foo.c:126: warning: assignment makes pointer from integer without a cast
foo.c:127: warning: assignment makes pointer from integer without a cast
foo.c:130: warning: double format, pointer arg (arg 2)
foo.c:135: warning: double format, pointer arg (arg 2)
foo.c:140: warning: assignment makes pointer from integer without a cast
foo.c:141: warning: assignment makes pointer from integer without a cast
foo.c: In function `search':
foo.c:214: warning: unknown escape sequence `\ '
foo.c:222: warning: unknown escape sequence `\ '
foo.c:223: warning: unknown conversion type character ` ' in format
foo.c:223: warning: too many arguments for format
foo.c:232: warning: unknown escape sequence `\ '
foo.c:242: warning: unknown escape sequence `\ '
foo.c:252: warning: unknown escape sequence `\ '
foo.c:253: warning: unknown conversion type character ` ' in format
foo.c:253: warning: too many arguments for format
foo.c: At top level:
foo.c:276: warning: declaration of `list' shadows global declaration
foo.c: In function `show':
foo.c:277: warning: declaration of `list' shadows global declaration
foo.c: In function `exit':
foo.c:335: warning: control reaches end of non-void function
make: *** [foo.o] Error 1
 
J

jaysome

(e-mail address removed) said:


I beg to differ. See below. Fix this lot, starting with the first, and then
get back to us.

(Caution: in the following diagnostic report, line numbers may differ
slightly to those in your source.)

gcc -W -Wall -ansi -pedantic -Wformat-nonliteral -Wcast-align
-Wpointer-arith -Wbad-function-cast -Wmissing-prototypes
-Wstrict-prototypes -Wmissing-declarations -Winline -Wundef
-Wnested-externs -Wcast-qual -Wshadow -Wconversion -Wwrite-strings
-Wno-conversion -ffloat-store -O2 -g -pg -c -o foo.o foo.c

And then top it off with a run through PC-lint from
http://www.gimpel.com/ (warnings like 525 are supressed, and line
numbers may differ slightly to those in your source):

PC-lint for C/C++ (NT) Vers. 8.00u, Copyright Gimpel Software
1985-2006

--- Module: stdc.c (C)
_
int exit(void);
stdc.c(10) : Error 18: Symbol 'exit(int)' redeclared (void/nonvoid,
arg. count) conflicts with line 261, file stdlib.h
stdlib.h(261) : Info 830: Location cited in prior message
_
int exit(void);
stdc.c(10) : Warning 532: Return mode of function 'exit(int)'
inconsistent with line 261, file stdlib.h
stdlib.h(261) : Info 830: Location cited in prior message
_
menu();
stdc.c(33) : Warning 534: Ignoring return value of function
'menu(void)' (compare with line 6)
stdc.c(6) : Info 830: Location cited in prior message
_
#... = (char)(('\n\n'))) : _flsbuf((('\n\n')),((&_iob[1])))) /*lint
-restore *
#... ('\n\n'),stdout)
putchar('\n\n');
stdc.c(51) : Info 742: Multiple character constant
_
#... _flsbuf((('\n\n')),((&_iob[1])))) /*lint -restore */
#... ('\n\n'),stdout)
putchar('\n\n');
stdc.c(51) : Info 742: Multiple character constant
_
insert();
stdc.c(66) : Warning 534: Ignoring return value of function
'insert(void)' (compare with line 7)
stdc.c(7) : Info 830: Location cited in prior message
_
search();
stdc.c(71) : Warning 534: Ignoring return value of function
'search(void)' (compare with line 8)
stdc.c(8) : Info 830: Location cited in prior message
_
show(list);
stdc.c(76) : Warning 534: Ignoring return value of function
'show(struct sells *)' (compare with line 9)
stdc.c(9) : Info 830: Location cited in prior message
_
exit();
stdc.c(81) : Error 118: Too few arguments (0) for prototype
'exit(int)'
_
}
stdc.c(84) : Info 744: switch statement has no default
_
printf("%f\n",&m_out);
stdc.c(100) : Warning 559: Size of argument no. 2 inconsistent with
format
_
scanf("%s",&titlos);
stdc.c(104) : Warning 545: Suspicious use of &
stdc.c(104) : Warning 561: (arg. no. 2) indirect object inconsistent
with format
_
getchar();
stdc.c(105) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
scanf("%s",&tragoudisths);
stdc.c(109) : Warning 545: Suspicious use of &
stdc.c(109) : Warning 561: (arg. no. 2) indirect object inconsistent
with format
_
getchar();
stdc.c(110) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
getchar();
stdc.c(114) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
getchar();
stdc.c(120) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
getchar();
stdc.c(122) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
printf("%f",&m_out);
stdc.c(135) : Warning 559: Size of argument no. 2 inconsistent with
format
_
printf("%f",&m_out);
stdc.c(140) : Warning 559: Size of argument no. 2 inconsistent with
format
_
printf("Press \"1\" to return to menu or \"2\" to exit\n");
stdc.c(159) : Warning 539: Did not expect positive indentation from
line 154
stdc.c(154) : Info 830: Location cited in prior message
_
printf("Press \"1\" to return to menu or \"2\" to exit\n");
stdc.c(159) : Warning 539: Did not expect positive indentation from
line 142
stdc.c(142) : Info 830: Location cited in prior message
_
printf("Press \"1\" to return to menu or \"2\" to exit\n");
stdc.c(159) : Warning 539: Did not expect positive indentation from
line 137
stdc.c(137) : Info 830: Location cited in prior message
_
menu();
stdc.c(167) : Warning 534: Ignoring return value of function
'menu(void)' (compare with line 42)
stdc.c(42) : Info 830: Location cited in prior message
_
exit();
stdc.c(172) : Error 118: Too few arguments (0) for prototype
'exit(int)'
_
}
stdc.c(175) : Info 744: switch statement has no default
_
gets(titlos);
stdc.c(191) : Warning 534: Ignoring return value of function
'gets(char *)' (compare with line 324, file stdio.h)
stdio.h(324) : Info 830: Location cited in prior message
_
gets(titlos);
stdc.c(191) : Warning 421: Caution -- function 'gets(char *)' is
considered dangerous
_
getchar();
stdc.c(192) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
menu();
stdc.c(204) : Warning 534: Ignoring return value of function
'menu(void)' (compare with line 42)
stdc.c(42) : Info 830: Location cited in prior message
_
exit();
stdc.c(209) : Error 118: Too few arguments (0) for prototype
'exit(int)'
_
}
stdc.c(212) : Info 744: switch statement has no default
_
getchar();
stdc.c(231) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
gets(list->tittle);
stdc.c(235) : Warning 534: Ignoring return value of function
'gets(char *)' (compare with line 324, file stdio.h)
stdio.h(324) : Info 830: Location cited in prior message
_
gets(list->tittle);
stdc.c(235) : Warning 421: Caution -- function 'gets(char *)' is
considered dangerous
_
getchar();
stdc.c(236) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
getchar();
stdc.c(240) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
gets(list->singer);
stdc.c(244) : Warning 534: Ignoring return value of function
'gets(char *)' (compare with line 324, file stdio.h)
stdio.h(324) : Info 830: Location cited in prior message
_
gets(list->singer);
stdc.c(244) : Warning 421: Caution -- function 'gets(char *)' is
considered dangerous
_
getchar();
stdc.c(245) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
getchar();
stdc.c(249) : Warning 534: Ignoring return value of function
'_filbuf(struct _iobuf *)' (compare with line 275, file stdio.h)
stdio.h(275) : Info 830: Location cited in prior message
_
scanf("%d",&list->price);
stdc.c(257) : Info 725: Expected positive indentation from line 254
stdc.c(254) : Info 830: Location cited in prior message
_
menu();
stdc.c(265) : Warning 534: Ignoring return value of function
'menu(void)' (compare with line 42)
stdc.c(42) : Info 830: Location cited in prior message
_
}
stdc.c(268) : Info 744: switch statement has no default
_
{
stdc.c(281) : Warning 578: Declaration of symbol 'list' hides symbol
'list' (line 21)
stdc.c(21) : Info 830: Location cited in prior message
_
menu();
stdc.c(303) : Warning 534: Ignoring return value of function
'menu(void)' (compare with line 42)
stdc.c(42) : Info 830: Location cited in prior message
_
exit();
stdc.c(308) : Error 118: Too few arguments (0) for prototype
'exit(int)'
_
}
stdc.c(311) : Info 744: switch statement has no default
_
{
stdc.c(319) : Error 18: Symbol 'exit(int)' redeclared (void/nonvoid,
arg. count) conflicts with line 261, file stdlib.h
stdlib.h(261) : Info 830: Location cited in prior message
_
{
stdc.c(319) : Warning 515: Symbol 'exit(void)' has arg. count conflict
(0 vs. 1) with line 261, file stdlib.h
stdlib.h(261) : Info 830: Location cited in prior message
_
{
stdc.c(319) : Warning 532: Return mode of function 'exit(void)'
inconsistent with line 261, file stdlib.h
stdlib.h(261) : Info 830: Location cited in prior message
_
menu();
stdc.c(331) : Warning 534: Ignoring return value of function
'menu(void)' (compare with line 42)
stdc.c(42) : Info 830: Location cited in prior message
_
return 0;
stdc.c(338) : Warning 527: Unreachable code at token 'return'
_
}
stdc.c(339) : Info 744: switch statement has no default
_
}
stdc.c(340) : Warning 533: function 'exit(void)' should return a value
(see line 318)
stdc.c(318) : Info 830: Location cited in prior message

--- Global Wrap-up

Note 900: Successful completion, 91 messages produced
Tool returned code: 0
 
R

Richard Heathfield

jaysome said:

And then top it off with a run through PC-lint from
http://www.gimpel.com/ (warnings like 525 are supressed, and line
numbers may differ slightly to those in your source):

Oooh, unkind. :)

Some good news for the OP: my error list was somewhat inflated by my
cack-handed attempts at linewrap repair. Here is the revised list, which is
a fair bit shorter:

gcc -W -Wall -ansi -pedantic -Wformat-nonliteral -Wcast-align
-Wpointer-arith -Wbad-function-cast -Wmissing-prototypes
-Wstrict-prototypes -Wmissing-declarations -Winline -Wundef
-Wnested-externs -Wcast-qual -Wshadow -Wconversion -Wwrite-strings
-Wno-conversion -ffloat-store -O2 -g -pg -c -o foo.o foo.c
foo.c:10: conflicting types for `exit'
/usr/include/stdlib.h:577: previous declaration of `exit'
foo.c:26: warning: no previous prototype for `alloc'
foo.c: In function `menu':
foo.c:52: warning: multi-character character constant
foo.c: In function `insert':
foo.c:102: warning: double format, pointer arg (arg 2)
foo.c:105: warning: char format, different type arg (arg 2)
foo.c:108: warning: char format, different type arg (arg 2)
foo.c:120: warning: implicit declaration of function `strdup'
foo.c:120: warning: assignment makes pointer from integer without a cast
foo.c:121: warning: assignment makes pointer from integer without a cast
foo.c:126: warning: assignment makes pointer from integer without a cast
foo.c:127: warning: assignment makes pointer from integer without a cast
foo.c:130: warning: double format, pointer arg (arg 2)
foo.c:135: warning: double format, pointer arg (arg 2)
foo.c:140: warning: assignment makes pointer from integer without a cast
foo.c:141: warning: assignment makes pointer from integer without a cast
foo.c: At top level:
foo.c:276: warning: declaration of `list' shadows global declaration
foo.c: In function `show':
foo.c:277: warning: declaration of `list' shadows global declaration
foo.c: In function `exit':
foo.c:335: warning: control reaches end of non-void function
make: *** [foo.o] Error 1
 
J

Jiri Slaby

(e-mail address removed) napsal(a):
Hi.I am started learning Programm language C before some time.I am
trying to make a programm about a very simple "sell shop".This programm
hasn't got any compile problem but when i run it i face some other
ploblems which i can not correct.I would appreciated if someone take a
look at my programm so as to help me.I tried many times to find out
where my mistakes are but i didn't manage something.
I have made some comments due to the programm so as to make it more
easy to as.
Any recommendations about my writing style in the programm are
acceptable.

Coding style? yes, try to use indent in way such this one:
indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs
(un*x utility for indentation)
Please help me!!!

Programm


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

/* Functions*/
int menu(void);
struct sells *insert(void);
struct sells *search(void);
int show(struct sells *list);
int exit(void);

/*Struct*/
struct sells{
char *tittle;
char *singer;
int price;
int count;
struct sells *next;
};

struct sells *list;


struct sells *alloc(void)
{
return(struct sells*)malloc(sizeof(struct sells));

you don't need to cast.
}


/*Function main*/
int main(void)
{
menu();
return 0;
}



/*Function menu.In this function the user has the opportunity to choose

what he/she want to do*/
int menu(void)
{
int i;

printf("---------MENU---------\n");
printf("Press \"1\" to insert data\n");
printf("Press \"2\" to search data\n");
printf("Press \"3\" to show data\n");
printf("Press \"4\" to exit from programm\n");

You may use puts for these.
putchar('\n\n');

printf("Make your choice\t");
scanf("%d",&i);

while((i<1)&&(i>4))
{
printf("You made wrong choice.\nPlease make a new
choice\n");
scanf("%d",&i);
}

switch(i)
{
case 1:
{
insert();
break;
}
case 2:
{
search();
break;
}
case 3:
{
show(list);
break;
}
case 4:
{
exit();
break;
}
}
return i;
}



/*In this function the user insert the data.*/
struct sells *insert(void)
{
char titlos[15];
char tragoudisths[15];
int timi;
struct sells *n;
float m_out;
int i;
m_out=0;

you can do float m_out = 0;
printf("%f\n",&m_out);
n=alloc();

check for NULL.
printf("Give the title of the CD\n");
scanf("%s",&titlos);

This is bug-prone. If I enter char > 15, it will crash.
Another thing, turn on warnings, it would tell you, it's bad.
titlos is pointer yet, so scanf("%s", titlos); is correct,
getchar();


printf("Give the name of the singer\n");
scanf("%s",&tragoudisths);
....

getchar();

printf("Give the price of the CD\n");
scanf("%d",&timi);
getchar();

while((timi<0)||(timi>100))
{
printf("You have insert a wrong price.\nGive a new
price.\n");
scanf("%d",&timi);
getchar();
}
getchar();

n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);

NULLs handling
n->price=timi;

if(list==NULL)
{
list=alloc();
again

list->tittle=strdup(n->tittle);
list->singer=strdup(n->singer);
list->price=n->price;
list->next=NULL;
printf("%f",&m_out);

why pointer?
}
else if((m_out=(strcmp(titlos,n->tittle)))==0)
{
list->count++;
printf("%f",&m_out);
....

}
else if((m_out=(strcmp(titlos,n->tittle)))<0)
{
n=alloc();
....

n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);
n->price=timi;
n->count=1;
n->next=list;
list=n;

printf("%s\t\t%s\t\t%d\t\t\n",list->tittle,list->singer,list->price);
}
else
{
list->next=insert();
return list;
}
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return n;
}


/*In this function the user search for an element.Also he can modify
it.
If someone can i would like to tell me what i have to do so as to
delete an element.*/
struct sells *search(void)
{
char titlos[15];
int i,k,out;
char choice;

printf("Give the tittle of the CD that you want to find\n");
gets(titlos);
getchar();
if(list==NULL||(out=strcmp(list->tittle,titlos))>0)
{
printf("The tittle that you want doesn't exist.\n");
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return list;
}
else if(out==0)
{
printf("Tittle: \t%s\n",list->tittle);
printf("Singer: \t%s\n",list->singer);
printf("Price: \t%d\n",list->price);

printf("Do you want to make any change;\n");
printf("If Yes press \"1\" else \"0\" to return to
menu\n");
printf("Make your choice");
scanf("%d",&k);
switch(k)
{
case 1:
{
printf("Do you want ot change the tittle;\nPress Y(YES)
or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new tittle:\n");
gets(list->tittle);
getchar();
}
printf("Do you want ot change the name of the
singer;\nPress Y(YES) or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new name:\n");
gets(list->singer);
getchar();
}
printf("Do you want ot change the price of the
CD;\nPress Y(YES) or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new price:\n");
scanf("%d",&list->price);
while((list->price<0)&&(list->price>100))
{

unneeded { }
printf("You have insert a wrong price.\nGive a new
price.\n");
scanf("%d",&list->price);
}
putchar('\n');
}
break;
}
case 0:
{
menu();
break;
}
}
}
return list;
}


/*Function which shows to the user all the element that has already
insert.
In this functio there is a problem.When i insert more the 3 elements,
these elements doesn't appear to me.I think that the problem is in my
insert
but i am not quite sure.*/
int show(struct sells *list)
{
int i,k;
k=1;
printf("\t\t-------LIST--------\n");
printf("Num\t\tTittle\t\tSinger\t\tPrice\n");
while(list!=NULL)
{

printf("%d\t%s\t\t%s\t\t%d\t\t\n",k,list->tittle,list->singer,list->price);

k++;
list=list->next;
}
printf("What do you want to do\n");
printf("Press \"1\" to retutn to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return 0;
}



/*Exit from the programm*/
int exit(void)
{
int i;

printf("Do you really want to exit from the programm;\n");
printf("If Yes press \"1\" else \"0\"\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
break;
}

Why you call menu again, when you want to exit? Call exit(0);, but cleanup before.
 
J

jaysome

jaysome said:



Oooh, unkind. :)

One of the warnings, 421, flagged gets() as being dangerous. The
PC-lint text documentation (MSG.TXT), which you can download from
Gimpel's web site, says this about warning 421:

421 Caution -- function 'Symbol' is considered dangerous -- This
message is issued (by default) for the built-in function gets.
This function is considered dangerous because there is no
mechanism to ensure that the buffer provided as first argument
will not overflow. A well known computer virus (technically a
worm) was created based on this defect. Through the -function
option, the user may designate other functions as dangerous.

What other Standard C functions would be good candidates for the
"-function" option?

Best regards
 
R

Richard Heathfield

jaysome said:
One of the warnings, 421, flagged gets() as being dangerous. The
PC-lint text documentation (MSG.TXT), which you can download from
Gimpel's web site, says this about warning 421:
What other Standard C functions would be good candidates for the
"-function" option?

Just gets(). But I'd flag a few others as being "experts-only" functions,
which you should not use in "real" code unless you know /exactly/ what
you're doing:

From <stdio.h>

freopen, fflush, setvbuf, setbuf, sprintf, vsprintf, fscanf,
scanf, sscanf, feof

From <string.h>

strncpy, strncat, strtok, memcpy, memset

From <stdlib.h>

atof, atoi, atol, calloc, malloc, realloc, free, atexit,
system

From <stdarg.h>

va_start, va_arg, va_end

From <setjmp.h>

setjmp, longjmp

From <signal.h>

signal

From <time.h>

clock


As a matter of fact, I was tempted to list every single function in the
standard library! But the ones I've listed do, I think, represent the
principal "gotcha" functions.
 
R

Richard Bos

jaysome said:
One of the warnings, 421, flagged gets() as being dangerous. The
PC-lint text documentation (MSG.TXT), which you can download from
Gimpel's web site, says this about warning 421:

421 Caution -- function 'Symbol' is considered dangerous -- This
message is issued (by default) for the built-in function gets.
This function is considered dangerous because there is no
mechanism to ensure that the buffer provided as first argument
will not overflow. A well known computer virus (technically a
worm) was created based on this defect. Through the -function
option, the user may designate other functions as dangerous.

What other Standard C functions would be good candidates for the
"-function" option?

None, really. gets() is the only one that can _not_ be used safely.
Functions like scanf() are easy to use unsafely, but can be made safe in
a simple, though WOMBATty, way, by constructing the format string at run
time, using something like (warning: not tested!)

sprintf(format_string, "%%%ds", current_size_of_buffer);
scanf(format_string, destination_string);

Richard
 
R

Richard Heathfield

Richard Bos said:

Functions like scanf() are easy to use unsafely, but can be made safe in
a simple, though WOMBATty, way, by constructing the format string at run
time, using something like (warning: not tested!)

sprintf(format_string, "%%%ds", current_size_of_buffer);
scanf(format_string, destination_string);

You'll want to add a check on the return value of scanf.
 
O

osmium

Hi.I am started learning Programm language C before some time.I am
trying to make a programm about a very simple "sell shop".This programm
hasn't got any compile problem but when i run it i face some other
ploblems which i can not correct.I would appreciated if someone take a
look at my programm so as to help me.I tried many times to find out
where my mistakes are but i didn't manage something.
I have made some comments due to the programm so as to make it more
easy to as.
Any recommendations about my writing style in the programm are
acceptable.
Please help me!!!

Programm


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

/* Functions*/
int menu(void);
struct sells *insert(void);
struct sells *search(void);
int show(struct sells *list);
int exit(void);

exit() is an existing fucntion in <stdlib.h> It is not a good idea to reuse
names like this. Name your function exitx().
I wouldn't knowingly reuse names even if I *hadn't* included the particualar
include file.

/*Struct*/
struct sells{
char *tittle;
char *singer;
int price;
int count;
struct sells *next;
};

struct sells *list;


struct sells *alloc(void)
{
return(struct sells*)malloc(sizeof(struct sells));
}


/*Function main*/
int main(void)
{
menu();
return 0;

No data there. Your comments are detailed in nature, not "global". So I
can't tell what you are trying to do. I *guess* it is a linked list data
structure. But it is not owned by main, which would be typical. So I guess
it must be owned by menu. I'll look there.
}



/*Function menu.In this function the user has the opportunity to choose

what he/she want to do*/
int menu(void)
{
int i;

printf("---------MENU---------\n");

No, the only thing menu owns is a simple int. Who owns the list??? (If
there is a list.) Go to end.

printf("Press \"1\" to insert data\n");
printf("Press \"2\" to search data\n");
printf("Press \"3\" to show data\n");
printf("Press \"4\" to exit from programm\n");
putchar('\n\n');

printf("Make your choice\t");
scanf("%d",&i);

while((i<1)&&(i>4))
{
printf("You made wrong choice.\nPlease make a new
choice\n");
scanf("%d",&i);
}

switch(i)
{
case 1:
{
insert();
break;
}
case 2:
{
search();
break;
}
case 3:
{
show(list);
break;
}
case 4:
{
exit();
break;
}
}
return i;
}



/*In this function the user insert the data.*/
struct sells *insert(void)
{
char titlos[15];
char tragoudisths[15];
int timi;
struct sells *n;
float m_out;
int i;
m_out=0;
printf("%f\n",&m_out);
n=alloc();

printf("Give the title of the CD\n");
scanf("%s",&titlos);
getchar();


printf("Give the name of the singer\n");
scanf("%s",&tragoudisths);
getchar();

printf("Give the price of the CD\n");
scanf("%d",&timi);
getchar();

while((timi<0)||(timi>100))
{
printf("You have insert a wrong price.\nGive a new
price.\n");
scanf("%d",&timi);
getchar();
}
getchar();

n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);
n->price=timi;

if(list==NULL)
{
list=alloc();
list->tittle=strdup(n->tittle);
list->singer=strdup(n->singer);
list->price=n->price;
list->next=NULL;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))==0)
{
list->count++;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))<0)
{
n=alloc();
n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);
n->price=timi;
n->count=1;
n->next=list;
list=n;

printf("%s\t\t%s\t\t%d\t\t\n",list->tittle,list->singer,list->price);
}
else
{
list->next=insert();
return list;
}
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return n;
}


/*In this function the user search for an element.Also he can modify
it.
If someone can i would like to tell me what i have to do so as to
delete an element.*/
struct sells *search(void)
{
char titlos[15];
int i,k,out;
char choice;

printf("Give the tittle of the CD that you want to find\n");
gets(titlos);
getchar();
if(list==NULL||(out=strcmp(list->tittle,titlos))>0)
{
printf("The tittle that you want doesn't exist.\n");
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return list;
}
else if(out==0)
{
printf("Tittle: \t%s\n",list->tittle);
printf("Singer: \t%s\n",list->singer);
printf("Price: \t%d\n",list->price);

printf("Do you want to make any change;\n");
printf("If Yes press \"1\" else \"0\" to return to
menu\n");
printf("Make your choice");
scanf("%d",&k);
switch(k)
{
case 1:
{
printf("Do you want ot change the tittle;\nPress Y(YES)
or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new tittle:\n");
gets(list->tittle);
getchar();
}
printf("Do you want ot change the name of the
singer;\nPress Y(YES) or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new name:\n");
gets(list->singer);
getchar();
}
printf("Do you want ot change the price of the
CD;\nPress Y(YES) or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new price:\n");
scanf("%d",&list->price);
while((list->price<0)&&(list->price>100))
{
printf("You have insert a wrong price.\nGive a new
price.\n");
scanf("%d",&list->price);
}
putchar('\n');
}
break;
}
case 0:
{
menu();
break;
}
}
}
return list;
}


/*Function which shows to the user all the element that has already
insert.
In this functio there is a problem.When i insert more the 3 elements,
these elements doesn't appear to me.I think that the problem is in my
insert
but i am not quite sure.*/
int show(struct sells *list)
{
int i,k;
k=1;
printf("\t\t-------LIST--------\n");
printf("Num\t\tTittle\t\tSinger\t\tPrice\n");
while(list!=NULL)
{

printf("%d\t%s\t\t%s\t\t%d\t\t\n",k,list->tittle,list->singer,list->price);

k++;
list=list->next;
}
printf("What do you want to do\n");
printf("Press \"1\" to retutn to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}
return 0;
}



/*Exit from the programm*/
int exit(void)
{
int i;

printf("Do you really want to exit from the programm;\n");

Why do you assume the user is a moron? He just told you he wanted to exit.
printf("If Yes press \"1\" else \"0\"\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
break;
}
return 0;
}
}

I would start over. Don't do the menu, do it last, after the fundamentals
are working. Start with sell and show and get them working. Don't do all
the fussy details. For example, you can add reasonableness checks later.
Do you really need five fields in your data structure to test your ideas? I
doubt it. Programs should grow, Start small, test, add, test, add, test,
......
 
S

Skarmander

Richard said:
jaysome said:



Just gets(). But I'd flag a few others as being "experts-only" functions,
which you should not use in "real" code unless you know /exactly/ what
you're doing:
From <stdlib.h>

[...] calloc, malloc, realloc, free, [...]
Let's get this straight: you shouldn't dynamically allocate memory in C
unless you're an expert?

Well, actually, no arguments there -- but why not just say C is for experts
and leave it at that, then? For most applications, you need to be an expert
to write a program *without* dynamic memory allocation!

S.
 
R

Richard Heathfield

Skarmander said:
Richard Heathfield wrote:
[...] calloc, malloc, realloc, free, [...]
Let's get this straight: you shouldn't dynamically allocate memory in C
unless you're an expert?

Absolutely. Possibly the biggest single issue I have with code posted here
is shoddy dynamic memory allocation code.
 
A

Andrew Poelstra

Hi.I am started learning Programm language C before some time.I am
trying to make a programm about a very simple "sell shop".This programm
hasn't got any compile problem but when i run it i face some other
ploblems which i can not correct.I would appreciated if someone take a
look at my programm so as to help me.I tried many times to find out
where my mistakes are but i didn't manage something.
I have made some comments due to the programm so as to make it more
easy to as.
Any recommendations about my writing style in the programm are
acceptable.
Please help me!!!

Actually, this is one of the most well-styled programs I've seen posted
here (by a novice) in a while. I recommend you use 2 or 4-space tabs
instead of 8.
Programm


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

/* Functions*/
int menu(void);
struct sells *insert(void);
struct sells *search(void);
int show(struct sells *list);
int exit(void);

/*Struct*/
struct sells{
char *tittle;
char *singer;
int price;
int count;
struct sells *next;
};

struct sells *list;


struct sells *alloc(void)
{
return(struct sells*)malloc(sizeof(struct sells));
No need for the cast. Use
return malloc (sizeof (struct sells));

(Actually, I question why you're using a function at all for this.)
}

/*Function main*/
int main(void)
{
menu();
return 0;
}

Even in small functions, formatting is essential.
/*Function menu.In this function the user has the opportunity to choose
what he/she want to do*/
int menu(void)
{
int i;

printf("---------MENU---------\n");
printf("Press \"1\" to insert data\n");
printf("Press \"2\" to search data\n");
printf("Press \"3\" to show data\n");
printf("Press \"4\" to exit from programm\n");

It would perhaps be more efficient (and easier to read) if you replaced
printf("...\n"); with puts("...");

Notice how I eliminated the \n that way. Also, you might want to replace
your \"escaped double quotes\" with ``manual double quotes'' (notice how
they bend in the right direction).
putchar('\n\n');

This isn't correct. You can put two characters into single quotes. You'll
need to do printf ("\n\n"), two puts("") or two putchar ('\n').
printf("Make your choice\t");
scanf("%d",&i);

while((i<1)&&(i>4))

Space this out, and perhaps remove some of the superfluous parentheses:
while (i said:
{
printf("You made wrong choice.\nPlease make a new
choice\n");
scanf("%d",&i);
}

Perhaps you should have set i == 0 initially, and eliminated the first
input section. Or, you could have left i uninitialized and used a do...
....while loop.
switch(i)
{
case 1:
{
insert();
break;
}
case 2:
{
search();
break;
}
case 3:
{
show(list);
break;
}
case 4:
exit();
break;
}

Okay, here's where you need smaller tabs. I've corrected case 4 for you.
Notice also that the braces are unnecessary.
return i;
}

/*In this function the user insert the data.*/
struct sells *insert(void)
{
char titlos[15];
char tragoudisths[15];
int timi;
struct sells *n;
float m_out;
int i;
m_out=0;
printf("%f\n",&m_out);
n=alloc();

printf("Give the title of the CD\n");
scanf("%s",&titlos);
getchar();
scanf ("%s") is very dangerous. Replace with
fgets (titlos, sizeof titlos, stdin);
That will also eliminate the call to getchar() (although you will have
a newline at the end of titlos[] that you may wish to deal with).
printf("Give the name of the singer\n");
scanf("%s",&tragoudisths);
getchar();
fgets (tragoudisths, sizeof tragoudisths, stdin);
printf("Give the price of the CD\n");
scanf("%d",&timi);
getchar();
Here scanf() is okay, because you are getting an integer, not a string.
while((timi<0)||(timi>100))

Once again, space and unparentheses:
while (timi said:
{
printf("You have insert a wrong price.\nGive a new
price.\n");
I would put two calls to puts() here.
scanf("%d",&timi);
getchar();
}
getchar();

n->tittle=strdup(titlos);

strdup isn't an ANSI C function, and therefore shouldn't be used in clc
code. (Unless you defined it yourself, which I'll know by the end of the
source).
n->singer=strdup(tragoudisths);
n->price=timi;

if(list==NULL)
{
list=alloc();
Much better to write
list = malloc (sizeof *list);
and eliminate the alloc() function altogether.
list->tittle=strdup(n->tittle);
list->singer=strdup(n->singer);
list->price=n->price;
list->next=NULL;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))==0)
{
list->count++;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))<0)
{
n=alloc();
n = malloc (sizeof *n);
n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);
n->price=timi;
n->count=1;
n->next=list;
list=n;

printf("%s\t\t%s\t\t%d\t\t\n",list->tittle,list->singer,list->price);
}
else
{
list->next=insert();
return list;
}
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
exit();
break;
}
Fixed case 2 for you. (I use 2-space tabs, but 3 or 4 is fine.)
return n;
}


/*In this function the user search for an element.Also he can modify
it.
If someone can i would like to tell me what i have to do so as to
delete an element.*/
Make a new function which free()'s the element. free() is the opposite
of malloc. Also, if this is a list of some sort (I'm not looking at
the big picture this early in the thread), you'll need some other
management code.
struct sells *search(void)
{
char titlos[15];
int i,k,out;
char choice;

printf("Give the tittle of the CD that you want to find\n");
puts("Text doesn't need a \n with puts().");
gets(titlos);

Aaaah! gets should /never/, /ever/ be used. There is no possible way to
use it safely.

Use: fgets (titlos, sizeof titlos, stdin);
getchar();
if(list==NULL||(out=strcmp(list->tittle,titlos))>0)
{
printf("The tittle that you want doesn't exist.\n");
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
No braces, smaller indent.
}
return list;
}
else if(out==0)
{
printf("Tittle: \t%s\n",list->tittle);
printf("Singer: \t%s\n",list->singer);
printf("Price: \t%d\n",list->price);

printf("Do you want to make any change;\n");
printf("If Yes press \"1\" else \"0\" to return to
menu\n");
Again, consider `backtick and apostrophe' quotes. (Or, ``double'' quotes).
printf("Make your choice");
scanf("%d",&k);
switch(k)
{
case 1:
{
printf("Do you want ot change the tittle;\nPress Y(YES)
or N(NO)\t");
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new tittle:\n");
gets(list->tittle);
getchar();
}
printf("Do you want ot change the name of the
singer;\nPress Y(YES) or N(NO)\t");

1) Use lowercase.
2) Provide a default case, like "Change singer name (y/[n])? "

This indicates that your two choices are y and n, (usually case
insentive), which obviously mean yes and no, and that if nothing
is entered, n will be chosen.
scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new name:\n");
gets(list->singer);
Do /not/ use gets. Use fgets (list->singer, size, stdin) where size will
be determined wherever you allocated memory for list->singer. (You /did/
allocated memory, right?)
getchar();
}
printf("Do you want ot change the price of the
CD;\nPress Y(YES) or N(NO)\t");
(y/[n])

scanf("%s",&choice);
getchar();
if(choice=='Y')
{
printf("Give new price:\n");
scanf("%d",&list->price);
while((list->price<0)&&(list->price>100))
{
printf("You have insert a wrong price.\nGive a new
price.\n");
scanf("%d",&list->price);
}
putchar('\n');
}
break;
}
case 0:
{
menu();
break;
}

Smaller indent, no braces.
}
}
return list;
}


/*Function which shows to the user all the element that has already
insert.
In this functio there is a problem.When i insert more the 3 elements,
these elements doesn't appear to me.I think that the problem is in my
insert
but i am not quite sure.*/
int show(struct sells *list)
{
int i,k;
k=1;

These two lines can be combined: int i, k = 1;
printf("\t\t-------LIST--------\n");
printf("Num\t\tTittle\t\tSinger\t\tPrice\n");
while(list!=NULL)
{

printf("%d\t%s\t\t%s\t\t%d\t\t\n",k,list->tittle,list->singer,list->price);

k++;
list=list->next;
}
printf("What do you want to do\n");
printf("Press \"1\" to retutn to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
exit();
break;
}
}

In case 1, I'm pretty sure that you have indirect recursion. This source is
too big to check on that for sure. If so, you should reconsider your design.
return 0;
}



/*Exit from the programm*/
int exit(void)
{
int i;

printf("Do you really want to exit from the programm;\n");
printf("If Yes press \"1\" else \"0\"\n");
printf("Make your choice");

You need a ": " at the end of that line if you're grabbing input from it.
Otherwise if I type `monkey' I'll see "Make your choicemonkey" and I'll
think, "What's a choicemonkey?".
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
{
break;
}

In addition to my usual comments, you need a default case here to handle
erroneous input.
 
R

Richard Heathfield

Andrew Poelstra said:

No need for the cast. Use
return malloc (sizeof (struct sells));

Better still, either ditch the function entirely, or make it do some useful
work (e.g. initialisation):

struct sells *alloc(void)
{
static const struct sells blank;
struct sells *new = malloc(sizeof *new);
if(new != NULL)
{
*new = blank;
}
return new;
}
(Actually, I question why you're using a function at all for this.)

Quite so. But a use could be found. :)
 
R

Richard Heathfield

Jiri Slaby said:

Coding style? yes, try to use indent in way such this one:
indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs
(un*x utility for indentation)

There's a lot more to coding style than layout.

And anyway, you're using the wrong settings! :)

me@here:~> cat .indent.pro | fmt
--blank-lines-after-declarations --blank-lines-after-procedures
--blank-lines-before-block-comments --braces-after-if-line
--brace-indent0 --braces-after-struct-decl-line --blank-before-sizeof
--case-brace-indentation0 --continuation-indentation2 --case-indentation2
--break-function-decl-args --honour-newlines --indent-level2
--line-length72 --comment-line-length64 --continue-at-parentheses
--break-after-boolean-operator --dont-cuddle-do-while --dont-cuddle-else
--no-space-after-casts --no-space-after-function-call-names
--no-space-after-parentheses --dont-break-procedure-type
--no-space-after-for --no-space-after-if --no-space-after-while
--dont-space-special-semicolon -nut --preserve-mtime
--struct-brace-indentation0
 
R

Richard Heathfield

Andrew Poelstra said:

Space this out, and perhaps remove some of the superfluous parentheses:
while (i < 1 && i > 4);

That's the most efficient while loop I've seen in some time. Not content
with having the smallest possible loop body, it carefully avoids ever
executing it!

More prosaically: if the LHS of the && is true, the RHS cannot be true, so
the condition will always fail.

<snip>
 
J

Jiri Slaby

Andrew Poelstra napsal(a):
Actually, this is one of the most well-styled programs I've seen posted
here (by a novice) in a while. I recommend you use 2 or 4-space tabs
instead of 8.

Nope, indentation is done by 8-columns wide tabs, period.
No need for the cast. Use
return malloc (sizeof (struct sells));

You haven't read the thread, have you?
(Actually, I question why you're using a function at all for this.)

Even in small functions, formatting is essential.

It would perhaps be more efficient (and easier to read) if you replaced
printf("...\n"); with puts("...");
again...


Notice how I eliminated the \n that way. Also, you might want to replace
your \"escaped double quotes\" with ``manual double quotes'' (notice how
they bend in the right direction).


This isn't correct. You can put two characters into single quotes. You'll
need to do printf ("\n\n"), two puts("") or two putchar ('\n').
....


Space this out, and perhaps remove some of the superfluous parentheses:
while (i < 1 && i > 4);
semicolon?


Perhaps you should have set i == 0 initially, and eliminated the first
input section. Or, you could have left i uninitialized and used a do...
...while loop.


Okay, here's where you need smaller tabs. I've corrected case 4 for you.

Ugly as hell.
switch (whatever) {
case 1:
whatever
break;
}
is one of correct indentation.
Notice also that the braces are unnecessary.
return i;
}

/*In this function the user insert the data.*/
struct sells *insert(void)
{
char titlos[15];
char tragoudisths[15];
int timi;
struct sells *n;
float m_out;
int i;
m_out=0;
printf("%f\n",&m_out);
n=alloc();

printf("Give the title of the CD\n");
scanf("%s",&titlos);
getchar();
scanf ("%s") is very dangerous. Replace with
fgets (titlos, sizeof titlos, stdin);

and again...
That will also eliminate the call to getchar() (although you will have
a newline at the end of titlos[] that you may wish to deal with).
printf("Give the name of the singer\n");
scanf("%s",&tragoudisths);
getchar();
fgets (tragoudisths, sizeof tragoudisths, stdin);
printf("Give the price of the CD\n");
scanf("%d",&timi);
getchar();
Here scanf() is okay, because you are getting an integer, not a string.
while((timi<0)||(timi>100))

Once again, space and unparentheses:
while (timi said:
{
printf("You have insert a wrong price.\nGive a new
price.\n");
I would put two calls to puts() here.
scanf("%d",&timi);
getchar();
}
getchar();

n->tittle=strdup(titlos);

strdup isn't an ANSI C function, and therefore shouldn't be used in clc
code. (Unless you defined it yourself, which I'll know by the end of the
source).
n->singer=strdup(tragoudisths);
n->price=timi;

if(list==NULL)
{
list=alloc();
Much better to write
list = malloc (sizeof *list);
and eliminate the alloc() function altogether.
list->tittle=strdup(n->tittle);
list->singer=strdup(n->singer);
list->price=n->price;
list->next=NULL;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))==0)
{
list->count++;
printf("%f",&m_out);
}
else if((m_out=(strcmp(titlos,n->tittle)))<0)
{
n=alloc();
n = malloc (sizeof *n);
n->tittle=strdup(titlos);
n->singer=strdup(tragoudisths);
n->price=timi;
n->count=1;
n->next=list;
list=n;

printf("%s\t\t%s\t\t%d\t\t\n",list->tittle,list->singer,list->price);
}
else
{
list->next=insert();
return list;
}
printf("Press \"1\" to return to menu or \"2\" to exit\n");
printf("Make your choice");
scanf("%d",&i);

switch(i)
{
case 1:
{
menu();
break;
}
case 2:
exit();
break;
}
Fixed case 2 for you. (I use 2-space tabs, but 3 or 4 is fine.)

Blaah. Mixing of variable length indent is Evil(TM).
Read linux/Documentation/CodingStyle...
return n;
}


/*In this function the user search for an element.Also he can modify
it.
If someone can i would like to tell me what i have to do so as to
delete an element.*/
Make a new function which free()'s the element. free() is the opposite
of malloc. Also, if this is a list of some sort (I'm not looking at
the big picture this early in the thread), you'll need some other
management code.
struct sells *search(void)
{
char titlos[15];
int i,k,out;
char choice;

printf("Give the tittle of the CD that you want to find\n");
puts("Text doesn't need a \n with puts().");

Moreover it's not parsed...
Aaaah! gets should /never/, /ever/ be used. There is no possible way to
use it safely.

Use: fgets (titlos, sizeof titlos, stdin);

No braces, smaller indent.

....
 
F

Flash Gordon

Skarmander said:
Richard said:
jaysome said:



Just gets(). But I'd flag a few others as being "experts-only"
functions, which you should not use in "real" code unless you know
/exactly/ what you're doing:
From <stdlib.h>

[...] calloc, malloc, realloc, free, [...]
Let's get this straight: you shouldn't dynamically allocate memory in C
unless you're an expert?

It is extremely easy to get wrong.
Well, actually, no arguments there -- but why not just say C is for
experts and leave it at that, then? For most applications, you need to
be an expert to write a program *without* dynamic memory allocation!

Well, in significant parts of the embedded world you are explicitly
banned from using allocated memory, and I wrote a *lot* of code without
any difficulty with that restriction even when I was new to programming.
For example, the first significant program I ever wrote (which I wrote
in BASIC) was "The Game of Life".
http://www.google.com/search?sourceid=navclient-ff&ie=UTF-8&rlz=1B2GGGL_enGB177&q="the+game+of+life"
If you have a static board size there is absolutely no need for dynamic
memory allocation. IIRC it was interesting to implement.

There are plenty of other significant projects one can implement without
dynamic memory allocation. Sometimes by having a reasonable static size
and reporting if the user tries to go beyond it, sometimes without any
need at all for dynamic memory allocation.
 
O

osmium

I had a bit more time so I took another look.
struct sells *list;

There that little devil is! It's global, that's considered bad form but I
could live with that in this instance. But you need to initialize it to
NULL! The code in the sequel depends on that!

I still think the best thing to do is start over. If you follow all the
tweaking and formatting and syntax bitching in this thread and fix all that
up; I would guess the chances of your program working, after all that
effort, at about 1000:1. This is a pretty elementary error.
 
R

Richard Heathfield

osmium said:
I had a bit more time so I took another look.


There that little devil is! It's global, that's considered bad form but I
could live with that in this instance. But you need to initialize it to
NULL!

He did - by putting it at file scope. That was the mistake he didn't make.
He made almost all the others, though. :)
I would guess the chances of your program working, after all that
effort, at about 1000:1. This is a pretty elementary error.

It doesn't compile here, so I reckon the probability of it working is 0.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,755
Messages
2,569,536
Members
45,013
Latest member
KatriceSwa

Latest Threads

Top