views:

99

answers:

3

I've little experience with advanced OO practices, and I want to design this properly as an exercise. I'm thinking of implementing the following, and I'm asking if I'm going about this the right way.

I have a class PImage that holds the raw data and some information I need for an image file. Its header is currently something like this:

#include <boost/filesytem.hpp>
#include <vector>
namespace fs = boost::filesystem;
class PImage
{
  public:
    PImage(const fs::path& path, const unsigned char* buffer, int bufferLen) :
      path_(path), filesize_(bufferLen),
      data_(buffer, buffer + filesize_),
      width_(0), height_(0) {}
    const vector<char> data() const { return data_; }
    const char* rawData() const { return &data_[0]; }
    /*** other assorted accessors ***/
  private:
    fs::path path_;
    int filesize_;
    vector<char> data_;
    int width_;
    int height_;
}

I want to fill the width_ and height_ by looking through the file's header. The trivial/inelegant solution would be to have a lot of messy control flow that identifies the type of image file (.gif, .jpg, .png, etc) and then parse the header accordingly.

Instead of using vector<char> data_, I was thinking of having PImage use a class, RawImageStream data_ that inherits from vector<char>. Each type of file I plan to support would then inherit from RawImageStream, e.g. RawGifStream, RawPngStream.

Each RawXYZStream would encapsulate the respective header-parsing functions, and PImage would only have to do something like height_ = data_.getHeight();.

  1. Am I thinking this through correctly?
  2. How would I create the proper RawImageStream subclass for data_ to be in the PImage ctor? Is this where I could use an object factory?
  3. Anything I'm forgetting?
+1  A: 

Yes, you could implement your class hierarchy in the way you describe. Nevertheless, I would probably have had PngImage, GifImage and JpegImage derive directly from PImage. PImage can then become abstract:

class PImage
{
  virtual ~PImage {}
  virtual unsigned int getWidth() const = 0
  virtual unsigned int getHeight() const = 0
  ...
};

Then, each concrete image type implements getWidth and getHeight.

The PImage class can then be created by a PImage factory:

boost::shared_ptr<PImage> createImage(const fs::path& path);

In the factory, you pretty much open the file, look what type it has, and create the concrete image class passing the data in the constructor, to finally return the image as the abstract PImage.

Finally, I'd like to add that you should not worry too much about your design up front, and be ready to refactor later when you discover that your design does not fulfil your needs. That's through trial and error that you will grow a feeling for what design is proper for your problem!

small_duck
You dont want an abstract PImage since for example handling the rawdata is generic across all images types, so you can keep the implementation in the base class.
Henri
@Henri: I think what you're saying makes sense since I will also be freeing the `data_` when it's not in use to save memory, and `PImage` might still be manipulated. (`data_` will need to be reloaded when I predict `PImage::data()` will be called in the near future.) I think I'll be making the type of `data_` abstract.
Kache4
A: 

I would create an AbstractImage class containing pure virtual functions getHeight and getWidth();

Then I would override these functions in for example GifImage, JpegImage and PngImage (which all inherit from AbstractImage).

Then what you make a factory (factory pattern wikipedia ;)), which creates the correct concrete class for you depending on the filename.

So this would be something like

static AbstractImage* CreateImage(_fileName)
{
    switch(getExtension(_fileName))
    {
        case ".gif":
            return new GifImage(_fileName);
        case ".jpg":
            return new JpgedImage(_filename);
    }
}

So beside in this function, you dont have to worry about the actual derived type you're using. You can do everything with the interface AbstractImage provides and polymorphism does the rest for you.

Henri
This sounds like a variant of the Strategy design pattern.
calico-cat
I want `height_` and `width_` to be looked up once in `data_`, and saved independently because `data_` will be freed to save memory (and reloaded later), but I still need the dimensions of `PImage` in order to sort a `deque<PImage>`. I'll be needing a concrete `PImage`, but I think I'll be using an abstract `ImageStream` or something.
Kache4
+1  A: 

You should have a look at the strategy pattern.

Tony
Hmm, the problem with the examples they give is that their strategy is decided outside of the context, with a public `context::setStrategy()`. My strategies would need to be determined *inside* the context, and having code in the context choosing what strategy to use defeats the purpose of using this method.
Kache4