2

I wonder how one would go about adding different data sources to an already existing code. For example, let's say that I have this piece of code:

//DataFromFile.cpp

#include "Data.h"

class DataFromFile
{
public:
    DataFromFile() { filename = ""; }
    ~DataFromFile() { delete data; }
    void loadValue(const char *filename) { //getting data from a file }
    void printData() { data->printData(); }
private:
    string filename;
    Data *data;
};


//Source.cpp

#include <iostream>
#include "DataFromParameter.h"

int main(int argc, char **argv)
{
    DataFromFile oldData;
    oldData.loadValue("example.txt");
    oldData.printData();

   return 0;
}

and need to create class DataFromDatabase or DataFromParameter but without changing the way DataFromFile works (this code is used throughout various programs and going through their code is out of the question). Creating an interface seemed like a good way to go, so I came up with this solution:

//IData.h

#include "Data.h"

class IData
{
public:
    IData() { data = NULL; }
    virtual ~IData() = 0 { delete data; }
    void printData() { if (data != NULL) data->printData(); }
protected:
    Data *data;
}

//DataFromParameter.h

#include "IData.h"

class DataFromParameter : public IData
{
public:
    DataFromParameter(const int value) { this->data = new Data(value); }
    ~DataFromParameter() {}
};


//DataFromFile.h

#include "IData.h"

class DataFromFile : public IData
{
public:
    DataFromFile() { filename = ""; }
    DataFromFile(const char *filename) { loadValue(filename); }
    ~DataFromFile() {}
    void loadValue(const char *filename) { //getting data from a file }
private:
    string filename;
}

As you can see IData and DataFromParameter are the new classes and I also added some methods to DataFromFile. Creating the new objects and calling their methods:

//Source.cpp

#include <iostream>
#include "DataFromParameter.h"

int main(int argc, char **argv)
{
    //Source: file (the old way)
    DataFromFile oldData;
    oldData.loadValue("example.txt");
    oldData.printData();

    //Source: file (the new way)
    IData *newData1 = new DataFromFile("example.txt");
    newData1->printData();
    delete newData1;

    //Source: parameter
    IData *newData2 = new DataFromParameter(atoi(argv[1]));
    newData2->printData();
    delete newData2;

   return 0;
}

It works, nevertheless I'm not very happy with it; especially the DataFromFile class code looks messy - I know why it looks that way but am sure someone else would get pretty confused what exactly is going on in there.

My question is: is there a better way to go here? Is the sloppiness necessary (given the circumstances) or am I missing some obvious solution to the problem?

naru
  • 95
  • 2
  • 8
  • You may want to implement it as streams instead, that's the usual way this is done. Also, your code does not follow [the rule of three](http://stackoverflow.com/a/4172724/4538344). – StenSoft May 15 '15 at 12:07
  • Do you really need all 3 classes to derive from a common base? – NathanOliver May 15 '15 at 12:09
  • 1
    Can you expand on what you think is sloppy or messy? I don't see anything obviously wrong though you've not given your implementation of `loadValue` – Component 10 May 15 '15 at 12:09
  • Also `DataFromFile(const char *filename)` is not assigning `filename` to the private member variable – NathanOliver May 15 '15 at 12:10
  • I am aware of the rule, this is just an example code and I didn't want it to be longer than what it takes to present my problem. – naru May 15 '15 at 12:10
  • 2
    Belongs to http://codereview.stackexchange.com/ – Jarod42 May 15 '15 at 12:14
  • 1
    Why not just create the free functions: `Data MakeDataFromParameter(int);` and `Data MakeDataFromFile(const char* filename);` ? (or returning `std::unique_ptr`). – Jarod42 May 15 '15 at 12:18
  • Why did you include "data.h" in "IData.h"? Just curious. – jaklucky May 15 '15 at 12:36
  • @Component10 My feeling of the code being messy is calling the loadValue method from the constructor - supposing someone using the code decides to create an object using `DataFromFile(const char *filename)`. Then he will still have the option to call the method explicity (even though it's already been called). The body of `loadValue` is irrelevant, it consists of some simple I/O operations. @Jarod42 Well, that seems like a reasonable thing to do. I'm not sure why I haven't thought about it earlier. @jaklucky To be able to declare a private member `data`, which is an instance of `Data` class. – naru May 16 '15 at 10:00
  • @naru: In that case, why not make `loadValue` private so users of your class can't access it? A simple case would be to take a [RAII] (http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization) approach and then prohibit users from re-initialising. If that's the case, user would have to provide a file-name on object construction, but is that such a bad thing? Otherwise, you'll have to code for initialised and non-initialised state in both `DataFromFile` and the code that uses it. – Component 10 May 18 '15 at 14:01
  • @Component10 Unfortunately I cannot do that, as the `DataFromFile` class is already used in several programs and I can't change their codes. The piece of code I'm working on is a library and it has to be backward compatible. – naru May 19 '15 at 05:30
  • @Jarod42 I thought about your solution and figured it might be somewhat messy as well. Since I will be using multiple files in several places in my code I figured I would have to store the filenames in class members and for an instance of the class which uses parameters/database as a data source those members would be redundant. While using an interface I can create those members only in the class I actually need them in (in this case `DataFromFile`). – naru May 19 '15 at 07:11

0 Answers0