Problem with C program

exvor · 01-03-2005, 11:40 AM

This is a problem im doing for the ctutorial questions. It asked me to write a program that takes input from the user and removes all the spaces and replaces them with newlines. I did this to solve the problem but im getting one issue.

It displayes the error "Error Reading file for input" this is the error i told it to put if the open command return with a null.

problem is with the this part "temp = fopen ("temp.str","w");" in theory acording to my tutorial this should find the file if it exists remove its contents and open it for writeing.

problem is thats not what is occuring. its just apending the file not removing the current contents.

even if the file does not exist i still get the message. mentiond above when trying to read the file.

all and all the program works correctly ?

so do i just need to abandon my quest with high level file routines and go low level or am i doing something wrong ?

here is the code

Code:

/* program takes input from user and strips */ 
/* All of the spaces out and replaces them with newlines */ 


#include <stdio.h> 

char *typed; 
int errchk;
int SIZE = 100; 

void gettyped(char*);
void striprint(char*); 
 


int main()
{
    typed = (char *) malloc (SIZE); 
    
    gettyped(typed); 
    striprint(typed); 
     
    
    return 0; 
}

void gettyped(char *typed)
{
    /* This function gets values from the user */ 
    
    errchk = getline(&typed,&SIZE,stdin); 
        if (errchk == -1) 
	    {
		puts("Invalid input.");
	    }
}

void striprint(char *typed)
{
    FILE *temp;
    char *typedread;
    int err2chk;
    typedread = (char*) malloc(100);
    
   /* Opens a file for tempoary holding space */      
    
    temp = fopen ("temp.str","w");
    if (temp == NULL) 
	{
	    puts ("Error opening temporary file space"); 
	}
    fprintf(temp,typed); 
    errchk = 0; 
    
    errchk = fclose (temp); /* This action closes the file to write it */ 
    if (errchk == -1) 
	{
	    puts ("Failed to close file");
	}
    
    /* we need to reopen the file to parse it for the strings */ 
    
    temp = fopen ("temp.str","r"); 
      if (temp == NULL); 
        { 
	    puts("Error Reading file for input"); 
	} 
    /* file should be opened at this point */ 
    /* I probably should have made this a seperate function and may */ 
    /* do that later if i find it eazier to maintain */ 
    err2chk = 0; 
    /* The below code does the actual printing and adding a new line */ 
    do 
    { 
    
    err2chk = fscanf (temp,"%s",typedread);
    if (err2chk != -1)
	{
	printf("%s\n",typedread); 
	}
    }
 while (err2chk != -1);
 errchk = fclose (temp);
 if (errchk == -1) 
   {
    puts("error closeing file");
    }
}

itsme86 · 01-03-2005, 12:52 PM

What's the getline() function?

Hko · 01-03-2005, 12:56 PM

Quote:

It displayes the error "Error Reading file for input" this is the error i told it to put if the open command return with a null.

Yes, it will allways print that error, even when everything is OK. This s because you need to remove the ; after the "if"-statement (see commented code below)

Quote:

problem is with the this part "temp = fopen ("temp.str","w");" in theory acording to my tutorial this should find the file if it exists remove its contents and open it for writeing.

This works OK on my computer. Try fixing the other mistakes (see comments in the code below, and then try again.

But why are you writing the input to a file and then read it back in? Once the input from the user is read into the buffer (the char *typed is the buffer) you can parse it for spaces from that buffer! Why first writing it to a file and then reading it into another buffer before parsing it?

The getline() function is an advanced version of fgets(), and does some memory-fiddling if the buffer is not large enough to hold the data read. You are not using or checking for this, and I think you don't understand what its memory-fiddling feature is about (you have a mistake in your code caused by this, see comment below). Also getline() is a GNU-only function (it's not a commonly used function), which means your program will not compiled and run correctly on UNIX-like OS's that have not the GNU libc installed. In other words: it's not portable.

I'd recommend fgets() instead of getline(), unless you have special reasons (which you don't in this case).

One more thing:
When you compile with "-Wall" you get all compiler warnings. This will give you more clues about mistakes.

Code:

/*
 * program takes input from user and strips 
 * All of the spaces out and replaces them with newlines 
 */

/* #define added for getline() (see it man page) 
 * Must go before #include <stdio.h>
 */
#define _GNU_SOURCE

#include <stdio.h>

/* Header added for malloc() and free() */
#include <stdlib.h>

char *typed;
int errchk;
int SIZE = 100;

void gettyped(char *);
void striprint(char *);

int main()
{
    typed = (char *) malloc(SIZE);

    gettyped(typed);
    striprint(typed);

    /* Make a good habit of freeing malloc()ed memory.
     * While it does not matter really in this program,
     * not freeing malloc() memory is a popular way of 
     * introducing bugs, especially memory-leaks..
     */
    free(typed);

    return 0;
}

void gettyped(char *typed)
{
    /* This function gets values from the user  */

    /* It would have been better if you declared SIZE of type "size_t"
     * instead of int if you want it to use it here in getline().
     * If you compile with gcc's --Wall option ("Warnings-all") you get
     * a warning about this.
     *
     * Better though to use fgets() instead of getline(). You can then leave
     * it an int.
     */
    errchk = getline(&typed,  &SIZE, stdin);

    if (errchk == -1) {
        puts("Invalid input.");
    }
}

void striprint(char *typed)
{
    FILE *temp;
    char *typedread;
    int err2chk;

    typedread = (char *) malloc(100); /* Better use SIZE here instead of 100 */

    /* Opens a file for tempoary holding space */

    temp = fopen("temp.str", "w");
    if (temp == NULL) {
        puts("Error opening temporary file space");
    }
    fprintf(temp, typed);
    errchk = 0;

    errchk = fclose(temp);  /* This action closes the file to write it */
    if (errchk == -1) {
        puts("Failed to close file");
    }

    /* we need to reopen the file to parse it for the strings */
    /* You could have opened it for reading AND writing instead, and "rewind"
     * it here to start reading from the start by calling rewind() or fseek().
     * (see the man pages of those functions for more info)
     */

    temp = fopen("temp.str", "r");
    if (temp == NULL) ; /* <-- Remove the ';' here or the "if" won't work, so
                               error will print allways without reason! */
    {
        puts("Error Reading file for input");
    }
    /* file should be opened at this point  */
    /* I probably should have made this a seperate function and may */
    /* do that later if i find it eazier to maintain */
    err2chk = 0;
    /* The below code does the actual printing and adding a new line */
    do {

        err2chk = fscanf(temp, "%s", typedread);
        if (err2chk != -1) {
            printf("%s\n", typedread);
        }
    } while (err2chk != -1);
    errchk = fclose(temp);
    if (errchk == -1) {
        puts("error closeing file");
    }
    free(typedread); /* Again, a good habit */
}

exvor · 01-03-2005, 01:13 PM

Yea seams to me im not totaly grasping the whole sizeof thing yet. Im just learning c and this is the first problem that was semi advanced that i have tried to tackle. The tutorial praises getline that is why i tried to use it instead of other methods. It doesent even mention fgets.

as for why i did not use the buffer im still a little in the fog on buffers. And file postion. I figured i would need to make a file because im using file postion to determine where the next word typed is.

Is there a better way to replace a character in a string of undetermined length rather then the model that i have created ??

Hivemind · 01-03-2005, 01:27 PM

And remove the casts of malloc()'s return value:

Code:

typed = (char *) malloc (SIZE);

should be

Code:

typed =  malloc (SIZE);

The code compiles without the cast as long as you have the proper header (stdlib.h) included. If you don't have stdlib.h included (or a header that includes it), a compilation error is generated. If you cast what malloc() returns an error indicating that the proper header hasn't been included won't appear and you're left with a compilable, but non-working program.

I wonder why people insist on casting...must be alot of crappy tutorials out there. I know you have to cast in C++ but in C++ you should use new/delete because malloc/free doesn't understand constructors.

exvor · 01-03-2005, 01:35 PM

Yea stupid tutorial never even explained what the (char * ) even was just that it needed to be in front of malloc.

I dident see why i needed to put it there either ???

exvor · 01-03-2005, 01:37 PM

The header and the ; in front of the if statement was my own bad :P and i think i tracked my less on the file problem down. Its less thats messed up not my data file that was created :P

STUPID LESS BUFFER !!

Hko · 01-03-2005, 01:51 PM

Quote:

Originally posted by exvor
The tutorial praises getline that is why i tried to use it instead of other methods. It doesent even mention fgets.

OK. I had never heard of getline() before. Now that I read its man page, I realize it can be really useful. May be better to stick with it during the tutorial. But realize it can also be very tricky: Your program has a hard to understand-find-and-solve bug only because you use getline(). (this bug will only appear when the user type more than 100 characters). It's too far fetched to try explain this here, while you're still "in the fog" about buffers. To solve this, just remove the gettyped() function and put everyting that was in gettyped() into main(). Like this:

Code:

int main()
{
    typed = (char *) malloc(SIZE);
    errchk = getline(&typed, &SIZE, stdin);
    if (errchk == -1) {
        puts("Invalid input.");
    }
    striprint(typed);
    free(typed);
    return 0;
}

void striprint(char *typed)
{
  /* ..... left out code ......*/
}

And important in this case: use SIZE instead of 100 everywhere the buffer size is needed.

Quote:

I figured i would need to make a file because im using file postion to determine where the next word typed is.

Better use an variable (int) as an index to the buffer-chars for that. Reading files is slow compared to memory reading (not to mention first writing the file)

Quote:

Is there a better way to replace a character in a string of undetermined length rather then the model that i have created ??

Yes. By using the buffer. Much easier, faster & shorter. You can replace your striprint() function with just this:

Code:

void striprint(char *typed)
{
    int i;

    for (i = 0; i < SIZE; ++i) {
        if (typed[i] == ' ') {
            typed[i] = '\n';
        }
    }
    printf(typed);
}

You said: "...in a string of undetermined length". This is where getline() comes in really handy. When you really have no idea how long the input will be, and you don't want to put a limit on it, getline() can save a lot of troubles and opportunities to put bugs in a program. On the other hand getline() opens up a few bug-opportunities itself, but this is definately more managable if you understand how it works. BTW Note that this getline stuff only applies to C. Other, higher level, languages (java, python, perl, php...) have easier solutions. AFAIK even C++-strings make this GNU-getline() a non-option (when using C++ that is).

jlliagre · 01-03-2005, 02:46 PM

Quote:

I wonder why people insist on casting...must be alot of crappy tutorials out there.

Hmm, or perhaps some crappy comments ...

"Man malloc" or "grep malloc /usr/include/stdlib.h" will show you that malloc returns a void*, so it must be casted to the destination pointer type, unless it is a void*.
Not doing it will trigger a C compiler warning.

itsme86 · 01-03-2005, 03:00 PM

Quote:

Originally posted by jlliagre
Hmm, or perhaps some crappy comments ...

"Man malloc" or "grep malloc /usr/include/stdlib.h" will show you that malloc returns a void*, so it must be casted to the destination pointer type, unless it is a void*.
Not doing it will trigger a C compiler warning.

That applies to C++ only, not C. A void * pointer can be assigned to any other type of pointer type without the need for a cast. You can see it in action here:

Code:

itsme@dreams:~/C$ cat nowarn.c
#include <stdio.h>

void *func(void)
{
  void *ptr;

  return ptr;
}

int main(void)
{
  int *num;

  num = func();

  return 0;
}
itsme@dreams:~/C$ gcc -Wall -ansi -pedantic nowarn.c -o nowarn
itsme@dreams:~/C$

exvor · 01-03-2005, 03:06 PM

LOL this is so much eazier

Code:

void striprint(char *typed)
{
    int i;

    for (i = 0; i < SIZE; ++i) {
        if (typed[i] == ' ') {
            typed[i] = '\n';
        }
    }
    printf(typed);
}

silly me forgetting strings are arrays

itsme86 · 01-03-2005, 03:14 PM

Or you could just do it on-the-fly while printing the output:

Code:

{
  int i;

  for(i = 0;typed[i];++i)
    putchar(typed[i] == ' ' ? '\n' : typed[i]);
}

exvor · 01-03-2005, 03:40 PM

Actually theres a problem with doing this way. What happens is because the size of the buffer is 100 it asumes the string is 100 so it loops untill it gets to 100 rather then stopping at the end of the input. Maybe this is why i dident do it this way?

exvor · 01-03-2005, 03:46 PM

upon setting malloc to 1 i get some very strange results.

exvor · 01-03-2005, 03:54 PM

never mind i wasent minding wehre the } are

it seams to work ok but i wonder if you make malloc too big would it continue to loop untill it got to 100