Sign in to follow this  
DaTruth29

Problem with my function

Recommended Posts

/* Reads characters from f until newline into s; returns pointer */
/* to s if successful; NULL otherwise */
String * StringFGet(String *s, FILE *f) {
int c = 0, i = 0;
char buff[516];

if (f == NULL)
{
fprintf(stderr, "File was not passed");
exit(1);
}

c = fgetc(f);
while (c != '\n' && c != EOF) {
buff[i] = c;
i++;
c = fgetc(f);
}
if (c == EOF)
return NULL;
buff[i] = '\0';
free(s->val);
s->val = (char*)malloc(sizeof(char)*(i+1));
strcpy(s->val, &buff);
return s;
}

This is part of a project that I'm working on in school. The purpose of this function is to put that data in a file and please in a string structure. However, whenever I try to run it, it always crashes on me, specifically on the c=fgetc(f) part. Can somebody give me any ideas as to what is going wrong? I also wouldnt mind if gave me some tips on the code in general. Thanks guys!

Share this post


Link to post
Share on other sites
Note I may be a bit rusty on my C


/* Reads characters from f until newline into s; returns pointer */
/* to s if successful; NULL otherwise */

String * StringFGet(String *s, FILE *f)
{
// Rather than use these, use more descriptive names!
//int c = 0, i = 0;
int i = o;
char c = 0;
// int cur_char, total_chars

//char buff[516];
// Hey, why not [smile] This will make sure that we have enough space for simple files.
char buff[131073];

// Good that you are error checking!
if (f == NULL)
{
fprintf( stderr, "File was not passed" );
// But why exit and kill the program?!
exit(1);
}

// But what about this?
if( s == NULL )
{
fprintf( stderr, "No string was passed" );
// But why exit and kill the program?!
exit(1);
}

// Returns a char ;)
c = fgetc(f);
while (c != '\n' && c != EOF)
{
buff[i] = c;
i++;
c = fgetc(f);
// Let's add this in
if( i >= 131072 )
{
fprintf( stderr, "Need more room!" );
// But why exit and kill the program?!
exit(1);
}
}
/* Not good to do! Probabally the problem for you
if (c == EOF)
return NULL;*/


// If the file was empty
if (i == 0)
return NULL;

// I made buff 131073 rather than 131072, so this will always be valid
buff[i] = '\0';

// Not very safe!
//free( s->val );

// Let's check first
if( s->val )
free( s->val );

// Now allocate the memory, you do not need the i+1 anymore
s->val = (char*)malloc(sizeof(char)*(i));

strcpy( s->val, &buff );

return s;
}







Now that's a few things to read and look at. I think your problem is that if you have a file without a newline because the EOF terminates it, then the function will fail. That and you should have a few more checks before doing certain things, such as auto free a pointer that you have not checked yet.

However, I don't know if you can do this, but why not make everything simpler:

String * StringFGet(String *s, FILE *f)
{
// Check 1
if ( pFile == NULL )
{
fprintf( stderr, "No file was passed" );
return 0;
}
// Check 2
if ( s == NULL )
{
fprintf( stderr, "No string was passed" );
return 0;
}
char string [131073] = {0};
// If the line has more than 131072, OH WELL [grin]
fgets ( string, 131072, pFile);

// Let's check first
if( s->val )
free( s->val );

// Now allocate the memory, you do not need the i+1 anymore
s->val = (char*) malloc( sizeof(char) * strlen( string ) );

strcpy( s->val, &buff );

return s;
}




Like I said, not sure if you can do that, but is a bit easier to use. Note that it is not tested either [wink]. If you have to use that fgetc and it's still not working, let us know! Also, what does your string class look like? I'm thinking something like:

struct String
{
char* val;
...
};

Share this post


Link to post
Share on other sites
Do you have to use only C? And nothing on but your code is damn ugly. We aren't supposed to help with homework though bud so im not gonna tell you howto change that. I will give you en example of you code completly unchanged besides formatting to make it less-ugly.


/* Reads characters from f until newline into s; returns pointer */
/* to s if successful; NULL otherwise */
String * StringFGet(String *s, FILE *f)
{
int c = 0, i = 0;
char buff[516];

if (f == NULL)
{
fprintf(stderr, "File was not passed");
exit(1);
}

c = fgetc(f);
while (c != '\n' && c != EOF)
{
buff[i] = c;
i++;
c = fgetc(f);
}

if (c == EOF)
return NULL;

buff[i] = '\0';

free(s->val);
s->val = (char*)malloc(sizeof(char)*(i+1));
strcpy(s->val, &buff);

return s;
}



Isnt that much more readable with whitespce and tabs?

Share this post


Link to post
Share on other sites
Thanks Guys! And I'm sorry for the nasty code. I should have checked it before I sent it. Its actually alot more better looking then that. Basically, the assignment was to write the implemetation for a file called mystring.h. What I gave was only part of the implementation. I basically have all the other functions completed and working probably. The only problem I'm having now is with that function. Heres a copy of the whole implementation:

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

#include "mystring.h"

/* Initializes s to sequence of chars in cp */
void StringInit(String *s, char *cp){
s->val = (char *)malloc(sizeof(char)*(strlen(cp)+ 1));
strcpy(s->val, cp);
}

/* Resets s to the empty string */
void StringClr(String *s){
free(s->val);
s->val=(char *)malloc(sizeof(char));
*s->val=NULL;
}
/* Returns length of s */
int StringLen(String s){
return (strlen(s.val));
}
/* Returns nth char in s; '\0' if n is outside legal range of s */
char StringAt(String s, int n){
if (n > StringLen(s)|| n < 0)
return NULL;
return s.val[n];
}
/* Copies s2 to s1, returns pointer to s1 */
String *StringCpy(String *s1, String s2){
free(s1->val);
StringInit(s1, s2.val);
return s1;
}
/* Catenates s2 to s1, returns pointer to s1 */
String *StringCat(String *s1, String s2){
char *temp = (char*)malloc(sizeof(char)*(strlen(s1->val) + StringLen(s2)+ 1));

strcpy(temp, s1->val);
strcat(temp, s2.val);
s1->val = temp;
return s1;
}
/* Reads characters from stdin until newline */
/* into s; returns pointer to s if */
/* successful; NULL otherwise */
String *StringGet(String *s){
return StringFGet(s, stdin);
}
/* Prints s to stdout */
void StringPut(String s){
StringFPut(s, stdout);
}
/* Reads characters from f until newline into s; returns pointer */
/* to s if successful; NULL otherwise */
String * StringFGet(String *s, FILE *f) {
int c = 0, i = 0;
char buff[516];

if (f == NULL)
{
fprintf(stderr, "File was not passed");
exit(1);
}

c = fgetc(f);
while (c != '\n' && c != EOF) {
buff[i] = c;
i++;
c = fgetc(f);
}
if (c == EOF)
return NULL;
buff[i] = '\0';
free(s->val);
s->val = (char*)malloc(sizeof(char)*(i+1));
strcpy(s->val, &buff);
return s;
}
/* Prints s to f */
void StringFPut(String s, FILE *f){
if (f==NULL){
printf("Error opening file\n");
exit(1);
}
fprintf(f, "%s", s.val);
}



Drew_Benton gave me some good tips that I'm definitely going to try. And yes, your right, thats exactly how he gave us the String Structure. The funny thing is, when I call the StringGet function, which calls StringFGet, it works perfectly. Its when I pass a filepointer it that I get into trouble. I was thinking that maybe I shouldnt use fgetc and use something else.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this