Code review. How to make my code look better.

[Cppmiel]

Hello, the task was: I have a binary file. I need to write int values to that file and then read and print them to console using my own iterator.

My program works I guess, but

Is there anything you can advice me?

Like maybe my program may cause either memory leak or some errors in different situations?

  #include <iostream>
#include <cstdio>
#include <vector>

using namespace std;


class IntFile
{
public:
    int value;
    FILE* file;
    //int mnumbers[10];

    IntFile()
    {
        file = fopen("text.txt", "r+b");
    }

    ~IntFile()
    {
        fclose(fopen("text.txt", "w"));
        //fclose(file);
    }

    int& getValue(int index)
    {
        fseek(file, 4*index, SEEK_SET);
        fread(&value, 4, 1, file);
        return value;
    }

    friend struct iterator;
    struct iterator
    {
        int index = 0;
        int value2 = 0;
        IntFile* ptr;

        iterator(IntFile* ptr2, int idx, FILE* ptrfile)
        {
            ptr = ptr2;
            index = idx;
            fseek(ptrfile, 4*index, SEEK_SET);
        }
        bool operator==(const iterator&other) const
        {
            return index == other.index;
        }
        bool operator!=(const iterator&other) const
        {
            return index!=other.index;
        }

        int &operator*()
        {
            return ptr->getValue(index);
        }
        iterator&operator++()
        {
            this->index = index+1;

        }
        iterator&operator--()
        {
            this->index = index -1;
        }
    };
    iterator begin()
    {
        return iterator(this, 0, file);
    }
    iterator end(int number)
    {
        return iterator(this, number, file);
    }
    iterator end2(int number)
    {
        return iterator(this, number-1, file);
    }
    iterator begin2()
    {
        return iterator(this, -1, file);
    }
};


int main()
{
    IntFile myfile;
    int number;
    cout << "Enter number of elements: " << endl;
    cin >> number;

    int mnumbers[number];
    cout << "Enter your numbers: ";
    for ( int i = 0; i < number; i++)
    {
        cin >> myfile.value;
        mnumbers[i] = myfile.value;
    }
    cout << endl;

    fwrite(mnumbers,4,sizeof(mnumbers),myfile.file);

    cout << "FORWARD: " << endl;
    for (IntFile::iterator i = myfile.begin(); i != myfile.end(number); ++i)
    {
        cout << *i << " ";
    }

    cout << endl;

    cout << "BACKWARD: " << endl;
    for (IntFile::iterator i = myfile.end2(number); i != myfile.begin2(); --i)
    {
        cout << *i << " ";
    }


    cout << endl;
    return 0;
}

Thanks.

[AbstractionAnon]

Line 11: Why is value public?

Line 12: Why are you using a C FILE? You should be using a C++ stream.

Line 22: You're opening a new instance of the FILE just to close it. You're not closing the existing instance.

Line 95: That's not valid C++. In standard compliant C++ array dimensions must be known at compile time.

Line 77,81: If begin2 and end2 are supposed to be reverse iterators, the convention is to call them rbegin and rend.

[ne555]

compile with warnings, use a sanitizer.

        iterator&operator--()
        {
            this->index = index -1;
        }

undefined behaviour.

file = fopen("text.txt", "r+b");
¿what if the file doesn't exist?
`file' would be NULL, so operations on it will be invalid, but you never check that.

cin >> number;
int mnumbers[number];

illegal, array size must be known at compile time.

fwrite(mnumbers,4,sizeof(mnumbers),myfile.file);
screw encapsulation.
and you are giving a wrong size there:
4 may be sizeof(int) in your plataform, others may change. You may use sizeof(int) or better sizeof(mnumbers[0]) instead.
Then you write sizeof(mnumbers) ¿how much would that be? ¿what does sizeof give you? ¿and haven't you stored the number of elements elsewhere?

    IntFile()
    {
        file = fopen("text.txt", "r+b");
    }

is not line anyone would ever want to work on another file.

    ~IntFile()
    {
        fclose(fopen("text.txt", "w"));
        //fclose(file);
    }

please expalin why did you do that.

by the way, ¿what happens if you copy an IntFile?

[Cppmiel]

@AbstractionAnon

Correct me if I'm wrong

value is public because I need that in Line 11

Line 12: This is a tricky one, can't explain why I did that.

Line 77,81: I made rbegin and end instead of begin2() and end2(). Thanks for that.

I also tried to do a dynamical array, but that didn't work well. But I probably should do that anyway, if my way of array is wrong.

[Cppmiel]

@ne555

fclose(fopen("text.txt", "w")); - this was made to clear a file after I worked with it. But it's probably wrong if you say that I don't actually close the file.

I also tried to do a dynamical array, but that didn't work well. But I probably should do that anyway, if my way of array is wrong.

When I make sizeof(mnumbers[0]) instead of sizeof(mnumbers)
My programe shows wrong things.
Input: 5(amount), 1,2,3,4,5(numbers)
Output should be : 12345 and 54321
Real output: 12344 and 44321
So all values after 4th are similar to 4th

sizeof(mnumbers) gives me 4*amount of numbers so I had 20 for 5 elements

And I don't get what you mean by copy an IntFile.

Thanks.

[Cppmiel]

And also
Do I need to open/close file in constructor and destructor or it will be better to do that in int main()

And can I do that?

~IntFile()
    {
        fclose(fopen("text.txt", "w"));
        fclose(file);
    }

Or how can close and clear it.

[Thomas1965]

Do I need to open/close file in constructor and destructor

Yes that's the best solution.

There is nothing to clear. Just close it.

~IntFile()
{
  fclose(file);
}

Instead of an dynamic array you should use a vector if you are allowed.

[JLBorges]

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <stdexcept>
#include <iterator>
#include <limits>
#include <cassert>

class IntFile
{
    public:

        static constexpr std::size_t SZ_INT = sizeof(int) ;

        IntFile( const std::string& file_name = "text.txt" ) : file{ file_name, std::ios::binary }
        { if( !file.is_open() ) throw std::invalid_argument( "bad file name" ) ; }

        int getValue( std::streamsize index ) // return value
        {
            seek(index) ;

            int value ;
            if( file.read( reinterpret_cast<char*>( std::addressof(value) ), SZ_INT ) )
                return value ;

            else throw std::runtime_error( "input failure" ) ;
        }

        void seek( std::streamsize index )
        {
            file.clear() ;
            if( !file.seekg( SZ_INT*index ) ) throw std::out_of_range( "index out of range" ) ;
        }

    struct iterator : std::iterator< std::input_iterator_tag, int > // even if deprecated
    {
        iterator() = default ;
        iterator( std::istream& stm ) : pstm( stm ? std::addressof(stm) : nullptr )
        { if(pstm) ++*this ; }

        int operator*() const { return value ; }

        iterator& operator++()
        {
            if( !pstm->read( reinterpret_cast<char*>( std::addressof(value) ), IntFile::SZ_INT ) )
                pstm = nullptr ;
            return *this ;
        }

        iterator& operator++(int) { return ++*this ; }

        bool operator== ( const iterator& other ) const { return pstm == other.pstm ; }
        bool operator!= ( const iterator& other ) const { return pstm != other.pstm ; }

        private:
            std::istream* pstm = nullptr ;
            int value = 0;
    };

    iterator begin() { return iterator(file) ; }
    iterator end() { return iterator{} ; }

    private:
        std::ifstream file ;
};

int main()
{
    static_assert( std::numeric_limits<unsigned int>::digits ==
                       IntFile::SZ_INT * std::numeric_limits<unsigned char>::digits,
                   "this simple test driver assumes that all bits in the "
                   "object representation of int contribute to its value representation" ) ;

    IntFile ifile( __FILE__ ) ; // test with this file (interpret it as a sequence of int)

    const std::streamsize offset = 23 ;

    const int value = ifile.getValue(offset) ;
    std::cout << value << '\n' ;

    ifile.seek(0) ;
    std::vector<int> vec( ifile.begin(), ifile.end() ) ;
    assert( vec.size() > offset && vec[offset] == value ) ;
    std::cout << vec[offset] << '\n' ;
}

http://coliru.stacked-crooked.com/a/f95018b6826bea42
http://rextester.com/JGZY16727

[Cppmiel]

@JLBorges
I appreciate it, sir. But I'm afraid that's a little bit too complicated for me