2
\$\begingroup\$

The task was: Create a custom iterator to work with files.

I also was told to use a proxy pattern which is new to me. Is there everything I can change to make my code look better?

Or maybe I have some slight errors which my compiler fixed

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

using namespace std;


class IntFile
{
public:
    int value;
    FILE* file;


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

    ~IntFile()
    {

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

    virtual int setValue(int value, int index)
    {
        fseek(file, 4*index, SEEK_SET);
        fwrite(&value, 4, sizeof(value),file);
        return 0;
    }


    class Proxy
    {
    private:
        IntFile *ptr;
        long index;
    public:
        Proxy(IntFile* ptr3, int idx)
        {
            ptr = ptr3;
            index = idx;
        }
        Proxy& operator=(int value)
        {
            ptr->setValue(value, index);
        }
        operator int() const
        {
            return ptr->getValue(index);
            cout << "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" << endl;
        }

    };

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

        iterator(IntFile* ptr2, int idx, FILE* ptrfile)
        {
            ptr = ptr2;
            index = idx;
            fseek(ptrfile, 4*index, SEEK_SET);
        }

        iterator (int i = 0) : value2(i)
        {

        }

        bool operator==(const iterator&other) const
        {
            return index == other.index;
        }
        bool operator!=(const iterator&other) const
        {
            return index!=other.index;
        }
        Proxy operator*()
        {
            return Proxy(ptr, index);
        }
        int &operator=(int value)
        {
            this->value2 = value;
        }
        iterator &operator=(const iterator& other)
        {
            this->value2 = other.value2;
            return *this;
        }
        iterator&operator++()
        {
            this->index = index+1;
        }
        iterator&operator--()
        {
            this->index = index -1;
        }
        iterator operator+=(int index2)
        {
            this->index = index + index2;
        }
        iterator operator+(const iterator& other)
        {
            return iterator(value2 + other.value2);
        }
        iterator operator-(const iterator& other)
        {
            return iterator(value2 - other.value2);
        }

    };
    iterator begin()
    {
        return iterator(this, 0, file);
    }
    iterator end(int number)
    {
        return iterator(this, number, file);
    }
    iterator rbegin(int number)
    {
        return iterator(this, number-1, file);
    }
    iterator rend()
    {
        return iterator(this, -1, file);
    }
};


int main()
{
    IntFile myfile;

    int number;
    cout << "Enter number of elements: " << endl;
    cin >> number;

    vector <int> myVector;

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

    fwrite(&myVector[0], sizeof(vector<int>::value_type), myVector.size(),myfile.file);

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


    cout << endl << "BACKWARD 1 by 1: " << endl;
    for (IntFile::iterator i = myfile.rbegin(number); i != myfile.rend(); --i)
    {
        cout << *i << " ";
    }


    IntFile::iterator i = myfile.begin();

    /** *i = value **/
    int a = 75;
    *i = a;
    cout << endl << endl << "75 should be here: " << *i << endl;

    /** int value = *i **/
    int k = 0;
    k = *i;
    cout << "76 should be here: " << k+1 << endl;

    cout << endl;
    return 0;
}

Here you can see my input and output just in case.

Input: 
10, 1 2 3 4 5 6 7 8 9 10

Output:
FORWARD 1 by 1: 1 2 3 4 5 6 7 8 9 10 
BACKWARD 1 by 1: 10 9 8 7 6 5 4 3 2 1
75 should be here: 75
76 should be here: 76
\$\endgroup\$
  • \$\begingroup\$ You know there is an iterator that works with streams (ie files) as part of the standard. std::istream_iterator \$\endgroup\$ – Martin York Nov 2 '17 at 0:33
4
\$\begingroup\$

Implementation

  • using namespace std; is considered bad practice and should be avoided.
  • Naming: Many variable names tell nothing about their meaning. For example, what does value2 do? How is it different from value? Better names enhance the readability of the code!
  • The IntFile constructor should probably take the path to the file as a parameter (there might be other files than "test.txt").
  • The IntFile constructor never initializes IntFile::value.
  • IntFile::file and IntFile::value should probably be private.
  • file is never checked if it actually is a valid file handle.
  • Rule of 3/5 violation: IntFile acquires a resource and has a custom destructor. As such, the copy constructor, move constructor, copy assignment operator and move assignment operator should also be manually implemented or deleted (the compiler generated ones will very likely do the wrong thing).
  • getValue and setValue assume that sizeof(int) will always be 4 bytes. This is not the case! (e.g. if compiled for 64bit, sizeof(int) == 8)
  • getValue and setValue aren't aware of endianess. This can mean that writing a value 0x00000001 on one platform will be read as 0x01000000 on another.
  • setValues return value is always the same. Maybe its return type should be changed to void?
  • friend struct iterator does not refer to IntFile::iterator, but to std::iterator instead (reason: no forward declaration for IntFile::iterator and using namespace std; combined with #include <vector>).
  • iterator::iterator(IntFile*, int, int) never initializes iterator::value2.
  • iterator::iterator(IntFile*, int, int) unnecessarily calls fseek.
  • iterator::iterator(int) does not initialize iterator::ptr and iterator::index, thus setting every subsequent operation up for undefined behavior (e.g. dereferncing iterator::ptr).
  • iterator &iterator::operator=(const iterator&) never assigns iterator::ptr and iterator::index.
  • int &iterator::operator=(int) never returns an int value.
  • iterator &iterator::operator++() never returns a iterator&.
  • iterator &iterator::operator--() never returns a iterator&.
  • iterator &iterator::operator+=(int) never returns a iterator&.
  • I can't imagine what iterator::operator+(const iterator&) or iterator::operator-(const iterator&) would be used for.
  • IntFile::end(int) never checks if Number is out of bounds (could be negative or too large).
  • IntFile::rbegin() and IntFile::rend(int) should probably return std::reverse_iterator<iterator> instead (currently iterating backwards has to be done with --iter instead of the expected ++iter.
  • Proxy::index should be of the same type as the parameter index of IntFile::setValue and IntFile::getValue to prevent slicing errors (sizeof(long) might be greater than sizeof(int)).
  • The call to print to std::cout in Proxy::operator int() is never reached.

Design

  • IntFile::setValue: I'd expect the first parameter to be the index and the second parameter to be the value).
  • IntFile could provide an overload for operator[].
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your patience! I appreciate it! I've changed as much as I could \$\endgroup\$ – Alex Nov 1 '17 at 12:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.