Newbie prgramm and question

[Cmorriskuerten]

HI,

this not a really useful program, only learning, i'm a newbie, it uses
functions like: malloc, realloc strcat and I just want to know, if it is good
programming or program style:

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

#define TRUE 1
#define FALSE 0
#define PUFFER_SIZE 150 /*Is this the right place for definition?*/

void inputString(void);
void outputString(void);
int newLine(void);

char *string; /*was it a good desicion to use as global var!?*/

int main(void)
{
inputString();
outputString();

return (0);
}

void inputString(void)
{
char puffer[150];

puts("Type sth.: ");

gets(puffer);

/*I know I should test if malloc == NULL, realloc also, later*/

string = malloc((strlen(puffer)+1) * sizeof(char)); /*Or should I use realloc
with NULL*/
strcpy(string, puffer);


while(newLine() == TRUE)
{
puts("Type sth.: ");

fflush(stdin); /*Is there a better way to do this!?*/
gets(puffer);

string = realloc(string, (strlen(string) + strlen(puffer)+1));
strcat(string, puffer);
}

}

void outputString(void)
{
puts(string);

fflush(stdin); /*Is there a better way to do this!?*/
getchar();
}

int newLine(void)
{
int ch;

puts("\nContinue? [y/n]: ");

fflush(stdin); /*Is there a better to do this!?*/
ch = getchar();

printf("\n");

while(ch != 'y' && ch != 'Y' && ch != 'n' && ch != 'N')
{
/*Here comes a error message*/

puts("\nContinue? [y/n]: ");

fflush(stdin); /*Is there a better to do this!?*/
ch = getchar();

printf("\n");
}

if(ch == 'y' || ch == 'Y')
return (TRUE);
else
return (FALSE);
}

Tia,

Chris

 

[Mark A. Odell]

(e-mail address removed) (Cmorriskuerten) wrote in

HI,

this not a really useful program, only learning, i'm a newbie, it uses
functions like: malloc, realloc strcat and I just want to know, if it is
good programming or program style:

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

#define TRUE 1
#define FALSE 0
#define PUFFER_SIZE 150 /*Is this the right place for definition?*/

void inputString(void);
void outputString(void);
int newLine(void);

char *string; /*was it a good desicion to use as global var!?*/

No, allocate it main() and pass it around. Also, have functions that can
fail return a failure value that you can test. If you must use a var.
throughout the file, at least make it

static char *pString;

so one cannot extern it on you.

int main(void)
{
inputString();
outputString();

return (0);
}

void inputString(void)
{
char puffer[150];

puts("Type sth.: ");

gets(puffer);

/*I know I should test if malloc == NULL, realloc also, later*/

string = malloc((strlen(puffer)+1) * sizeof(char)); /*Or should I
use realloc
with NULL*/

Test malloc's return now. And sizeof (char) is guaranteed to be one so
eliminate this clutter.

strcpy(string, puffer);


while(newLine() == TRUE)
{
puts("Type sth.: ");

fflush(stdin); /*Is there a better way to do this!?*/

No, and this is wrong since you cannot flush an input stream. You will
need a platform-specific (e.g. not part of standard C) to do what you
want.

gets(puffer);

string = realloc(string, (strlen(string) + strlen(puffer)+1));
strcat(string, puffer);
}

}

void outputString(void)
{
puts(string);

fflush(stdin); /*Is there a better way to do this!?*/

see prev. comment.

getchar();
}

[snip]

 

[Irrwahn Grausewitz]

HI,

this not a really useful program, only learning, i'm a newbie, it uses
functions like: malloc, realloc strcat and I just want to know, if it is good
programming or program style:

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

#define TRUE 1
#define FALSE 0
#define PUFFER_SIZE 150 /*Is this the right place for definition?*/

void inputString(void);
void outputString(void);
int newLine(void);

char *string; /*was it a good desicion to use as global var!?*/

IMHO, no - you'd better declare it in main(), do the allocation
there, change your input/output functions to take and return pointers
to character and provide an additional paramter to pass the size of
the buffer to protect against possible buffer overruns (see below).

int main(void)

Good.

{
inputString();
outputString();

See above.

return (0);
}

void inputString(void)
{
char puffer[150];

Why did you #define PUFFER_SIZE 150, and then use this magic number?

puts("Type sth.: ");

Add a '\n' to your output or fflush(stdout) to make sure your message
gets written immediately.

gets(puffer);

NEVER EVER use gets(), it's unsafe in that it may result in buffer
overruns - use fgets() instead.
Please read the c.l.c-faq at http://www.eskimo.com/~scs/C-faq/top.html
section 12.23 on this topic.

/*I know I should test if malloc == NULL, realloc also, later*/ OK.


string = malloc((strlen(puffer)+1) * sizeof(char)); /*Or should I use realloc
with NULL*/

[Note my above suggestion to do this in main()]
string = malloc( strlen puffer + 1 ); sizeof (char) is always 1.

strcpy(string, puffer);


while(newLine() == TRUE)
{
puts("Type sth.: ");

See above.

fflush(stdin); /*Is there a better way to do this!?*/

Don't do this! The behaviour of fflush() is undefined for input
streams. Please read the faq, section 12.26 on this.

gets(puffer);

See above.

string = realloc(string, (strlen(string) + strlen(puffer)+1));

Well, you said you know you should check *alloc()'s return values...

strcat(string, puffer);
}

}

void outputString(void)
{
puts(string);

fflush(stdin); /*Is there a better way to do this!?*/

See above.
Hm...- I think you would like to fflush(stdout) here...

getchar();
}

int newLine(void)
{
int ch;

puts("\nContinue? [y/n]: ");

See above.

fflush(stdin); /*Is there a better to do this!?*/

See above.

ch = getchar();

printf("\n");

while(ch != 'y' && ch != 'Y' && ch != 'n' && ch != 'N')
{
/*Here comes a error message*/

puts("\nContinue? [y/n]: ");

See above.

fflush(stdin); /*Is there a better to do this!?*/

See above.

ch = getchar();

printf("\n");
}

if(ch == 'y' || ch == 'Y')
return (TRUE);
else
return (FALSE);
}

Tia,

Chris

Well, I'm not sure I caught all the flaws in your code, but I think you
got enough suggestions on how to improve it for now.

BTW: It's of course worth to read the whole c.l.c-faq at
http://www.eskimo.com/~scs/C-faq/top.html, it contains a lot of valuable
information...

Regards,

Irrwahn

 

[Derk Gwen]

(e-mail address removed) (Cmorriskuerten) wrote:
# HI,
#
# this not a really useful program, only learning, i'm a newbie, it uses
# functions like: malloc, realloc strcat and I just want to know, if it is good
# programming or program style:

The code has unnecessary quadratic time when with a few changes it could be
linear time. Since you're just starting, doing this is not a grievous sin,
but this can be a learning experience.

# while(newLine() == TRUE)
# {
# puts("Type sth.: ");
#
# fflush(stdin); /*Is there a better way to do this!?*/
# gets(puffer);
#
# string = realloc(string, (strlen(string) + strlen(puffer)+1));
# strcat(string, puffer);
# }

It is possible that every realloc has to copy the previously allocated block. If you
realloc n times, and each time you increase the block size by at most a constant w,
then you are potentially moving
w first iteration
+2w second iteration
+3w third iteration
...
+nw last iteration
or a total of (n^2 + n)w/2 bytes. This is the first place you introduce quadratic
time. Of instead you realloc by twice as much memory everytime you exhaust the
allocation, you instead are potentialy moving
w first iteration - first allocation
+2w second iteration - second allocation
+4w fourth iteration - fourth allocation
...
+2^(k-1) w approximately n/2 iteration - kth allocation
or a total of about 2^k w bytes where k = log n, so that 2^k = n, or a total
of about nw bytes are moved. This is linear time.

The second place is when you use strcat. Potentially strcat must scan the entire
destination string to calculate its length each time. This means scanning
w + 2w + 3w + ... + nw = O(n^2) bytes in total. If instead you keep track of the
destination string length and then strcpy to string+currentlength instead of using
strcat, the overall behaviour remains linear.

I often use something like
char *p/*destination*/ = 0;
int n/*actual allocation*/ = 0, o/*used portion*/ = 0;
for ...
char *q/*appended text*/ = ...
int l/*length of appended text*/ = strlen(q);
if (o+l+1>n)/*whether enough allocated space for q*/ {
/*if not approximately double the allocation*/
n = 2*(o+l+1); p = realloc(p,n);
}
strcpy(p+o,q) /*append the text*/
o += l;/*update new used portion*/