String manipulation program not returning expected output

[Logan Lee]

Hi. I've written a small program to learn to write in C. But unfortunately the output is all jumbled up and not nice.
/* read_file.c
The whole point of this code is to read the entire content from a file then arrange the data as a single string. */

#include <stdio.h>

char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

// Total number of characters in the file.
int m=0;
while(feof(text_file)==0) {
fgetc(text_file); m++;
}
fclose(text_file);
fopen("text_file","r");

char file_content[m];

int i=0;
while(i<m) {
fscanf(text_file, "%c", &file_content[i++]);
}

fclose(text_file);

return file_content;
}
/* substring.c
Return B from ABC */
char* getSubstring(char* larger, int a, int b) {
char* smaller=(char *)malloc(sizeof(char) * b);
int i;
for(i=0; i<b; i++) {
if (larger[a+i]=='\0') {
break;
}
smaller[i]=larger[a+i];
}
return smaller;
}

/* main.c */
#include "read_file.h"
#include "substring.h"

int main(int argc, char* argv[]) {
char* file_name=argv[1];
int a=atoi(argv[2]);
int b=atoi(argv[3]);
char* larger=returnArrayFromFile(file_name);
char* smaller=getSubstring(larger, a, b);
printf("%s", smaller);
}

/* Lastly, text_file */
abcdefghi

If I compile them with gcc *.c -o main and execute by

main text_file 3 3

I'm expecting the output

def

But I'm getting others with jumbled characters.

[Logan Lee]

Logan Lee <Lo*********@student.uts.edu.auwrites:

Hi. I've written a small program to learn to write in C. But unfortunately the output is all jumbled up and not nice.
/* read_file.c
The whole point of this code is to read the entire content from a file then arrange the data as a single string. */

#include <stdio.h>

char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

// Total number of characters in the file.
int m=0;
while(feof(text_file)==0) {
fgetc(text_file); m++;
}
fclose(text_file);
fopen("text_file","r");

char file_content[m];

int i=0;
while(i<m) {
fscanf(text_file, "%c", &file_content[i++]);
}

fclose(text_file);

return file_content;
}
/* substring.c
Return B from ABC */
char* getSubstring(char* larger, int a, int b) {
char* smaller=(char *)malloc(sizeof(char) * b);
int i;
for(i=0; i<b; i++) {
if (larger[a+i]=='\0') {
break;
}
smaller[i]=larger[a+i];
}
return smaller;
}

/* main.c */
#include "read_file.h"
#include "substring.h"

int main(int argc, char* argv[]) {
char* file_name=argv[1];
int a=atoi(argv[2]);
int b=atoi(argv[3]);
char* larger=returnArrayFromFile(file_name);
char* smaller=getSubstring(larger, a, b);
printf("%s", smaller);
}

/* Lastly, text_file */
abcdefghi

If I compile them with gcc *.c -o main and execute by

main text_file 3 3

I'm expecting the output

def

But I'm getting others with jumbled characters.

I forgot to include header files but they are obvious from the code given.

[krishan]

>int m=0;

while(feof(text_file)==0) {
fgetc(text_file); m++;
}
fclose(text_file);
fopen("text_file","r");

-->>>>>> char file_content[m];

Which compiler are u using?
The highlighted line makes me suspicious. can u do that ?

[krishan]

On Dec 15, 1:40 pm, krishan <[e-mail]:

int m=0;
while(feof(text_file)==0) {
fgetc(text_file); m++;
}
fclose(text_file);
fopen("text_file","r");

-->>>>>> char file_content[m];

Which compiler are u using?
The highlighted line makes me suspicious. can u do that ?

WOW gcc allows me to do that.....

[pete]

krishan wrote:

>
On Dec 15, 1:40 pm, krishan <[e-mail]:

>int m=0;
while(feof(text_file)==0) {
fgetc(text_file); m++;
}
fclose(text_file);
fopen("text_file","r");

-->>>>>> char file_content[m];

Which compiler are u using?
The highlighted line makes me suspicious. can u do that ?

In C99, yes.
In C89, no.

WOW gcc allows me to do that.....

--
pete

[Flash Gordon]

Logan Lee wrote, On 15/12/07 02:08:

Hi. I've written a small program to learn to write in C. But unfortunately the output is all jumbled up and not nice.
/* read_file.c
The whole point of this code is to read the entire content from a file then arrange the data as a single string. */

#include <stdio.h>

char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

// Total number of characters in the file.
int m=0;
while(feof(text_file)==0) {

This is classic incorrect usage of feof. See the FAQ at
http://c-faq.com/ specifically question 12.2

fgetc(text_file); m++;

It might be slightly better to use getc since text_file does not have a
side effect. getc is allowed to be a macro with a lot of freedom and so
can make it easier for the compiler to do optimisation.

}
fclose(text_file);
fopen("text_file","r");

char file_content[m];

You can only do this in C99, which is not fully implemented by most
compilers. So whilst correct it limits the portability of your code.

More seriously, you have not left space for a null termination of the
string.

int i=0;
while(i<m) {
fscanf(text_file, "%c", &file_content[i++]);

Why on earth are you using fscanf? Use getc (or fgetc).

}

You do not nul terminate your string so you cannot use standard string
functions on it.

fclose(text_file);

return file_content;

This is wrong. See question 7.5a of the comp.lang.c FAQ. I believe this
problem was pointed out to you recently with reference to another function.

}
/* substring.c
Return B from ABC */
char* getSubstring(char* larger, int a, int b) {

Use sensible parameter names to people know what the parameters are
meant to be without reading the function body.

char* smaller=(char *)malloc(sizeof(char) * b);

Scrap the cast, and sizeof(char) is 1 by *definition*.
char *smaller=malloc(b);
Then the compiler will complain at you (if it isn't already). The
solution to the error/warning is *not* adding a cast, it is including
stdlib.h to provide the correct prototype for malloc.

int i;
for(i=0; i<b; i++) {
if (larger[a+i]=='\0') {
break;
}
smaller[i]=larger[a+i];
}
return smaller;

Again you have not nul terminated your data (you have no space for the
nul termination) so you cannot use C string functions on the returned data.

}

/* main.c */
#include "read_file.h"
#include "substring.h"

If these headers are wrong they can also break things.

int main(int argc, char* argv[]) {
char* file_name=argv[1];

What if the user does not provide any parameters? BANG!

int a=atoi(argv[2]);
int b=atoi(argv[3]);

Validate your input. What if the user enters "-5" or "fred" or "a" is
bigger than the size of the file.

char* larger=returnArrayFromFile(file_name);
char* smaller=getSubstring(larger, a, b);
printf("%s", smaller);

smaller is not nul terminated so you cannot print it. Also you need to
terminate the last line of your printout with a new line to ensure
correct behaviour.

main is defined as returning an int, so return one. 0, EXIT_SUCCESS and
EXIT_FAILURE are the values the standard guarantees, 0 being success.

}

/* Lastly, text_file */
abcdefghi

If I compile them with gcc *.c -o main and execute by

<snip>

Add at least "-ansi -pedantic" or "-std=c99 -pedantic" to your options.
It would be better to add "-Wall -Wextra" as well.
--
Flash Gordon

[Rod Pemberton]

"Logan Lee" <Lo*********@student.uts.edu.auwrote in message
news:87************@student.uts.edu.au...

>
Hi. I've written a small program to learn to write in C. But unfortunately

the output is all jumbled up and not nice.

>

....

I'm expecting the output

def

But I'm getting others with jumbled characters.

There are numerous issues, but the reason your getting "jumbled" characters
is due to the way you dynamically allocate space for file_content.

>
/* read_file.c
The whole point of this code is to read the entire content from a file

then arrange the data as a single string. */

>
#include <stdio.h>

I treated all three routines as one file and needed these includes for my
suggested changes:

#include <stdlib.h>
#include <string.h/* for my changes */

char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

There's non-obvious coding error here interacting with the rest of the
program. It has to due with the character(s) '\n'... I don't expect you to
understand this. Anyway, you'll want to use "rb" instead of "r". In fact,
you'll save yourself much grief if you just learn to avoid non-binary modes
as much as possible. So, use this:
FILE *text_file=fopen(file_name,"rb");

>
// Total number of characters in the file.
int m=0;
while(feof(text_file)==0) {
fgetc(text_file); m++;
}

The while loop is equivalent to using fseek() and ftell():
fseek(text_file,0L,SEEK_END);
m=ftell(text_file);

fclose(text_file);
fopen("text_file","r");

The fopen line is erroneous. You meant file_name instead of "text_file".
Again, you'll need "rb" instead of "r":
fopen(file_name,"rb");

>
char file_content[m];

The declaration of file_content has two errors. The first is that the scope
of the storage is limited to returnArrayFromFile(), i.e., you can't use the
storage outside of returnArrayFromFile()... You want to use malloc(). The
second error is an off by one. You need to allocate m+1 since you need to
ensure that the array in main(), called larger, has an additional character
so it can be nul terminated. You want this:
char *file_content=malloc(m+1);

>
int i=0;
while(i<m) {
fscanf(text_file, "%c", &file_content[i++]);
}

This while loop is equivalent to a call to fread(). one(1) in the following
is the size which is sizeof(char), i.e., one(1).
fread(file_content,1,m,text_file);

fclose(text_file);

return file_content;
}
/* substring.c
Return B from ABC */
char* getSubstring(char* larger, int a, int b) {
char* smaller=(char *)malloc(sizeof(char) * b);

There are two errors in the malloc of smaller. The first is that the cast
on malloc is unecessary, an obsolete coding style, and some say prevents
locating certain errors. The second is that it too is off by one. You'll
need an additional character for the nul. Also, the sizeof(char) is always
one(1). You want this:
char* smaller=malloc(b+1);

int i;
for(i=0; i<b; i++) {
if (larger[a+i]=='\0') {
break;
}
smaller[i]=larger[a+i];
}

There is a coding error in the loop. You are checking for nul, '\0', but
you never set a nul in larger or file_content... One doesn't exist in the
text file. I believe, but didn't thoroughly check, that the if()-break
should be after the smaller=larger statement to copy the nul, _if_ it had
been there... Anyway, the for loop can be reworked to the following which
nul terminates both larger and smaller.

larger[a+b]='\0';
strcpy(smaller,&larger[a]);

return smaller;
}

/* main.c */
#include "read_file.h"
#include "substring.h"

int main(int argc, char* argv[]) {
char* file_name=argv[1];
int a=atoi(argv[2]);
int b=atoi(argv[3]);
char* larger=returnArrayFromFile(file_name);
char* smaller=getSubstring(larger, a, b);
printf("%s", smaller);
}

/* Lastly, text_file */
abcdefghi

If I compile them with gcc *.c -o main and execute by

If you rework the C99 features to C90, declarations at the top of the
procedure, no C++ comments, no dynamic allocations, then you can increase
the level of warnings to detect problems:
gcc -Wall -ansi -pedantic *.c -o main

Anyway there are other issues you need to look into:
1) not checking that malloc returned space
2) missing return(0) or exit(EXIT_SUCCESS) for main()
3) strtod,strtol,strtoul have fewer side effects than atoi
4) assumed passed in parameters are the correct type
Rod Pemberton

[Keith Thompson]

"Rod Pemberton" <do*********@nohavenot.cmmwrites:

"Logan Lee" <Lo*********@student.uts.edu.auwrote in message
news:87************@student.uts.edu.au...

[...]

>char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

There's non-obvious coding error here interacting with the rest of the
program. It has to due with the character(s) '\n'... I don't expect you to
understand this.

Perhaps you'd care to explain it, because I don't understand it
either.

Anyway, you'll want to use "rb" instead of "r". In fact,
you'll save yourself much grief if you just learn to avoid non-binary modes
as much as possible. So, use this:
FILE *text_file=fopen(file_name,"rb");

[...]

That's bad advice. Use "r" if you want to read a text file; use "rb"
if you want to read a file in binary mode.

It's possible that the OP would be better off using binary mode,
depending on exactly what he's trying to do, but it's by no means
certain, especially given the name "text_file".

--
Keith Thompson (The_Other_Keith) <ks***@mib.org>
Looking for software development work in the San Diego area.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

[William Pursell]

On Dec 15, 2:08 am, Logan Lee <[e-mail]:

Hi. I've written a small program to learn to write in C. But unfortunately the output is all jumbled up and not nice.

/* read_file.c
The whole point of this code is to read the entire content from a file then arrange the data as a single string. */

#include <stdio.h>

char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

You need to check that fopen was successful.

>
// Total number of characters in the file.
int m=0;
while(feof(text_file)==0) {
fgetc(text_file); m++;
}
fclose(text_file);

It is not necessary to precompute the size of
the file. It would be a good exercise to
allocate a buffer, read into it, and resize
it if you don't reach eof. If you do feel
it appropriate to pre-compute the size of
the file, it may be a good idea to do it
non-portably and use features of the OS,
such as a call to stat. It would definitely
be a good idea to put it in a function.

fopen("text_file","r");

That's odd. I thought we were given a name in
file_name, but now you seem to be reading from
a file with the hard coded name "text_file"...except
that you are discarding the result of the fopen().

>
char file_content[m];

int i=0;
while(i<m) {
fscanf(text_file, "%c", &file_content[i++]);
}

Ack. What's wrong with fread()? Putting
fscanf inside a loop like this is...really weird.
Especially considering that you are reading from
a FILE * that you already closed.

>
fclose(text_file);

return file_content;

}

/* substring.c
Return B from ABC */
char* getSubstring(char* larger, int a, int b) {
char* smaller=(char *)malloc(sizeof(char) * b);

Don't cast. Do check the return value.

int i;
for(i=0; i<b; i++) {
if (larger[a+i]=='\0') {
break;
}
smaller[i]=larger[a+i];
}
return smaller;

}

/* main.c */
#include "read_file.h"
#include "substring.h"

int main(int argc, char* argv[]) {
char* file_name=argv[1];
int a=atoi(argv[2]);
int b=atoi(argv[3]);
char* larger=returnArrayFromFile(file_name);
char* smaller=getSubstring(larger, a, b);
printf("%s", smaller);

}

/* Lastly, text_file */
abcdefghi

If I compile them with gcc *.c -o main and execute by

main text_file 3 3

I'm expecting the output

def

But I'm getting others with jumbled characters.

You don't properly null terminate the value
created in getSubstring.

[Rod Pemberton]

"Keith Thompson" <ks***@mib.orgwrote in message
news:87************@kvetch.smov.org...

"Rod Pemberton" <do*********@nohavenot.cmmwrites:

"Logan Lee" <Lo*********@student.uts.edu.auwrote in message
news:87************@student.uts.edu.au...

[...]

char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

There's non-obvious coding error here interacting with the rest of the
program. It has to due with the character(s) '\n'... I don't expect

you to

understand this.

Perhaps you'd care to explain it, because I don't understand it
either.

The details of the issue are beyond the OP's understanding at this point in
time. Further explanation, other than use "rb", would only confuse the
issues for him.

But as for you, Keith, stop being lazy. The code, both his and mine, are
available for you to work through. It'd behoove you to actually compile
some code for once in your life instead only coding C mentally...

Anyway, you'll want to use "rb" instead of "r". In

fact,

you'll save yourself much grief if you just learn to avoid non-binary

modes

as much as possible. So, use this:
FILE *text_file=fopen(file_name,"rb");

[...]

That's bad advice.

Without understanding the issue, your claim is suspect at best. At worst,
it's flat out wrong.
Rod Pemberton

[pete]

Rod Pemberton wrote:

>
"Keith Thompson" <ks***@mib.orgwrote in message
news:87************@kvetch.smov.org...

"Rod Pemberton" <do*********@nohavenot.cmmwrites:

"Logan Lee" <Lo*********@student.uts.edu.auwrote in message
news:87************@student.uts.edu.au...

[...]

>char* returnArrayFromFile(char* file_name) {
> // Try opening a file
> FILE *text_file=fopen(file_name,"r");
>
There's non-obvious coding error here interacting
with the rest of the program.
It has to due with the character(s) '\n'...
I don't expect you to understand this.

Perhaps you'd care to explain it,
because I don't understand it either.

The details of the issue are beyond
the OP's understanding at this point in time.
Further explanation, other than use "rb",
would only confuse the issues for him.

But as for you, Keith, stop being lazy.
The code, both his and mine,
are available for you to work through.
It'd behoove you to actually compile
some code for once in your life instead only coding C mentally...

I've posted more tested C code to this newsgroup
than I have posted English text,
and I don't know what you mean either.

I like working with text files.

--
pete

[Keith Thompson]

"Rod Pemberton" <do*********@nohavenot.cmmwrites:

"Keith Thompson" <ks***@mib.orgwrote in message
news:87************@kvetch.smov.org...

>"Rod Pemberton" <do*********@nohavenot.cmmwrites:

"Logan Lee" <Lo*********@student.uts.edu.auwrote in message
news:87************@student.uts.edu.au...

[...]

>char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

There's non-obvious coding error here interacting with the rest
of the program. It has to due with the character(s) '\n'... I
don't expect you to understand this.

Perhaps you'd care to explain it, because I don't understand it
either.

The details of the issue are beyond the OP's understanding at this
point in time. Further explanation, other than use "rb", would only
confuse the issues for him.

But as for you, Keith, stop being lazy. The code, both his and
mine, are available for you to work through. It'd behoove you to
actually compile some code for once in your life instead only coding
C mentally...

I looked at the OP's code. Though it contains numerous errors (which
have already been discussed), it's clear enough what he's trying to
do. Opening the file in text mode makes perfect sense for what he's
doing.

He reads the contents of a file into a single string, then extracts a
specified substring (the file name and the substring bounds are
specified via command-line arguments), then he prints the substring.

On systems that distinguish between text mode and binary mode
(<OT>Windows does, Unix doesn't</OT>), opening the file in binary mode
would change the program's behavior. I see no basis for assuming that
the behavior seen in binary mode would be preferred.

It would be nice for the program's behavior to be specified more
precisely; a proper specification would settle the question of text
mode vs. binary mode.

Anyway, you'll want to use "rb" instead of "r".
In fact, you'll save yourself much grief if you just learn to
avoid non-binary modes as much as possible. So, use this: FILE
*text_file=fopen(file_name,"rb");

[...]

That's bad advice.

Without understanding the issue, your claim is suspect at best. At
worst, it's flat out wrong.

I understand the difference between text mode and binary mode. I
don't understand what "issue" you're talking about. Until and unless
you choose to explain it to the rest of us, I'll simply advise the OP
(and everyone else) to ignore your advice.

If you're reading text files, you should use text mode.

--
Keith Thompson (The_Other_Keith) <ks***@mib.org>
Looking for software development work in the San Diego area.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

[Philip Potter]

Rod Pemberton wrote:

"Keith Thompson" <ks***@mib.orgwrote in message
news:87************@kvetch.smov.org...

>"Rod Pemberton" <do*********@nohavenot.cmmwrites:

>>"Logan Lee" <Lo*********@student.uts.edu.auwrote in message
news:87************@student.uts.edu.au...

[...]

>>>char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");
There's non-obvious coding error here interacting with the rest of the
program. It has to due with the character(s) '\n'... I don't expect

you to

>>understand this.

Perhaps you'd care to explain it, because I don't understand it
either.

The details of the issue are beyond the OP's understanding at this point in
time. Further explanation, other than use "rb", would only confuse the
issues for him.

But as for you, Keith, stop being lazy. The code, both his and mine, are
available for you to work through. It'd behoove you to actually compile
some code for once in your life instead only coding C mentally...

Rod, you're the one being lazy by making assertions without backing them
up. I don't know what you're talking about, but I've a feeling it may
have to do with the crlf/cr/lf differences in text files between
operating systems. In this case, using text mode is the *correct* way to
handle these situations, unless you want to write an application which
should accept all three conventions rather than the local convention.

>> Anyway, you'll want to use "rb" instead of "r". In

fact,

>>you'll save yourself much grief if you just learn to avoid non-binary

modes

>>as much as possible. So, use this:
FILE *text_file=fopen(file_name,"rb");

[...]

That's bad advice.

Without understanding the issue, your claim is suspect at best. At worst,
it's flat out wrong.

Until you explain what your claim is, let alone justify that claim,
you'll win no arguments here. The only substantial claim you've made is
"avoid non-binary modes as much as possible". I can't think what
persuades you to think that is good advice.

[Barry Schwarz]

On Sat, 15 Dec 2007 13:08:45 +1100, Logan Lee
<Lo*********@student.uts.edu.auwrote:

>
Hi. I've written a small program to learn to write in C. But unfortunately the output is all jumbled up and not nice.
/* read_file.c
The whole point of this code is to read the entire content from a file then arrange the data as a single string. */

#include <stdio.h>

char* returnArrayFromFile(char* file_name) {
// Try opening a file
FILE *text_file=fopen(file_name,"r");

// Total number of characters in the file.
int m=0;
while(feof(text_file)==0) {

This will cause you to attempt to read the next character after you
have already read the last. However, as a consequence of the law of
compensating errors, you actually needed file_content to be one larger
than the character count to hold the terminating '\0' which you forgot
to add.

fgetc(text_file); m++;
}
fclose(text_file);
fopen("text_file","r");

char file_content[m];

Unless you have C99, array dimensions must be constant and
declarations must precede statements.

>
int i=0;
while(i<m) {
fscanf(text_file, "%c", &file_content[i++]);
}

fclose(text_file);

return file_content;

file_content ceases to exist when the function returns. The return
statement actually returns the address of file_content but as soon as
the function ends this value becomes indeterminate by definition.

>}
/* substring.c
Return B from ABC */
char* getSubstring(char* larger, int a, int b) {
char* smaller=(char *)malloc(sizeof(char) * b);
int i;
for(i=0; i<b; i++) {
if (larger[a+i]=='\0') {

Due to the error in returnArrayFromFile, this if can never be true.

> break;
}
smaller[i]=larger[a+i];

As a result, smaller is never a string.

> }
return smaller;
}

/* main.c */
#include "read_file.h"
#include "substring.h"

int main(int argc, char* argv[]) {
char* file_name=argv[1];
int a=atoi(argv[2]);
int b=atoi(argv[3]);
char* larger=returnArrayFromFile(file_name);
char* smaller=getSubstring(larger, a, b);

You are not allowed to evaluate larger at this point. Doing so
invokes undefined behavior.

getSubstring allocates space. You should free it.

printf("%s", smaller);

Since smaller is not terminated, this also invokes undefined behavior.

>}

/* Lastly, text_file */
abcdefghi

If I compile them with gcc *.c -o main and execute by

main text_file 3 3

I'm expecting the output

def

But I'm getting others with jumbled characters.

That's why it's called undefined behavior.
Remove del for email