Simple read from TXT

Hi,

I cant find an error in this code... Its supposed to read simple txt file.

Code:

#include <iostream>
#include <stdio.h>

int end;
int size;
FILE *file;
char *buffer;
char *read;

void Open()
{
    if((file = fopen("text.txt", "r+")) == NULL)
    {
            printf("File not found\n");
    }
    else
    {
            printf("File has been opened\n");
    }
}

void Read()
{
     if(fread(read, size, 1, file))
     {
                     std::cout << "Read from file: \n\t" << read << "\n";
     }
     else
     {
                    printf("Failed to read from file\n");
     }
}

void Close()
{
    if(fclose(file))
    {
                          printf("File can not be closed\n");
    }
    else
    {
                          printf("File closed\n");
    }
}

int main(void)
{
    Open();
    Read();
    Close();
    std::cin >> end;
}

Can you help me out please? The app never reads the content or crashes right away

[Dev-C++]

- initialize your variables
- reserve memory to hold read values
- use RAII (encapsulate your parameters and functions open, read, close in a class)

>> char *read;

a pointer alone allocates no storage. use malloc or a local buffer. or better yet use a real container like std::string or std::vector. and remember that uninitialized pointers are very dangerous since they cause code to crash at unpredictable times...

remember that uninitialized pointers are very dangerous since they cause code to crash at unpredictable times...

Not necessarily -- uninitialized pointers by themselves can do no harm. It's just when you try to dereference them that things can go wrong. This program, for example, never crashes. (Unless your compiler or environment is seriously messed up.)

Code:

#include <string.h>

int main() {
    char str[10];
    char *uninitialized;
    char *p;

    p = str;
    strcpy(p, "data");

    return 0;
}

A suggestion: don't use global variables if you can help it. Passing stuff around between functions might seem more laborious, but it's a much better habit to have in the long run.

Don't teach newbies very bad practices! This is a very big ticking time bomb!
Never copy data into an unallocated space.
Just defining a pointer somewhere does not allocate memory.
And this is all C, but the end std::cin. Use proper C++ if you really want to use it. C++ isn't C, as C++ can do wonderful stuff C cannot, and it is obvious you should take advantage of it.
As has already been suggested, use a std::vector or std::string for buffer and read using file streams.

Originally Posted by Elysia

Don't teach newbies very bad practices! This is a very big ticking time bomb!
Never copy data into an unallocated space.

Umm . . . I didn't. I declared an uninitialized pointer, then later set it to point to a buffer of ten characters, and then copied something into that buffer via the pointer. It was meant to show that an uninitialized pointer does not affect how it can be used later on.

Just defining a pointer somewhere does not allocate memory.

This is true. I never said otherwise. I was debating the point that any uninitialized pointer could cause the program to crash at any time.

And this is all C, but the end std::cin. Use proper C++ if you really want to use it. C++ isn't C, as C++ can do wonderful stuff C cannot, and it is obvious you should take advantage of it.

Yes, it is. That's why I carefully wrote a sample program that is valid C and C++.

>> uninitialized pointers by themselves can do no harm. It's just when you try to dereference them that things can go wrong.

precisely my point. initialized pointers are that much easier to debug!

>> C++ isn't C, as C++ can do wonderful stuff C cannot, and it is obvious you should take advantage of it.

perhaps less obvious is that some of us don't make as much of a distinction. it is foolish not to take advantage the STL, of course (especially as a beginner), but you do still have to learn the core parts of the language too, eventually.

>> uninitialized pointers by themselves can do no harm. It's just when you try to dereference them that things can go wrong.

precisely my point. initialized pointers are that much easier to debug!

I didn't mean to suggest that having pointers that are only initialized later in the code with some convoluted logic that is very hard to follow is a good thing. I was just contesting this statement:

remember that uninitialized pointers are very dangerous since they cause code to crash at unpredictable times...

The fact is, uninitialized pointers do not "cause code to crash". They can cause code to crash. Adding "char *uninitialized_pointer;" to your code is not going to cause it to crash. In addition, writing to an uninitialized pointer is not guaranteed to crash your program -- although that outcome is very likely. It's simply undefined behaviour.

You can tell I'm a programmer with such stubborn emphasis on being "correct".

Anyway, it's not important. I think we both know what we're talking about, and we're probably just confusing the OP -- especially me.

Thanks everyone for help... Have solved it now

For record:

Code:

char read[1024];
FILE *file;

void Open()
{
    if((file = fopen("text.txt", "r+")) == NULL)
    {
            printf("File not found\n");
    }
    else
    {
            printf("File has been opened\n");
    }
}

void Read(int lenght)
{
     for(int i = 0; i <= lenght; i++)
     {
             read[i] = getc(file);
     }
     if(read)
     {
                     std::cout << "Read from file: \n\n\t" << read << "\n";
     }
     else
     {
                    printf("Failed to read from file\n");
     }
}

So if you fail to open the file, you're still going to try to read and then close it? As soon as that fails, you should do something about it. (Although this usually means spitting out an error messaging and crapping out)

Don't use a variable name like lenght. Change it to length.
Normally, you should not access read[length]. The highest one should be read[length-1] unless length is not what is normally meant by length.

If you're going to use C, use C. Otherwise use C++. I'm assuming you want C++, so us fstream for file i/o.




And clean up your tabs before the indentation police (Elysia) get after you.