HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Engineering a minimalist image interface without templates

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withoutimageinterfacetemplatesengineeringminimalist

Problem

I'm practicing different ways of class design, so I'd like feedback on the design characteristics. Of course code practice comments are also welcome.

It was a conscious decision to omit templates. My next version will use a class template to store image data, similar to CImg and boost::GIL. I intend to make more classes with increasing complexity (templates, abstract base class, variadic templates). Until I reach a detailed yet highly abstract image class.

Header:

```
#ifndef AIMG
#define AIMG

#include
#include

class AImg
{
public:
const std::vector dimsizes; //or std::array of length 4?
const int N; //# of elements ( = product of dimension sizes)
unsigned int Bytes {1}; //bytes per element
std::string tag; //meta info like "rgb"
std::string cast;//used for returning data of type cast
void* data; //pointer to some array/container/byte-blob

AImg(const std::vector&); //you have to specify image size when creating. But may be empty of data.
void setdata(void*);
void* getdata();

//example of pseudo-template method:
void plusone(std::string);
int operator()(unsigned int); //variable args would allow better indexing

private:
int getN();
//checkcast() ?
};

//public fcns
AImg::AImg(const std::vector& input): dimsizes(input), N(getN()), cast("") {}
int AImg::operator()(unsigned int idx){
if (cast=="int")
{return ((int*)data)[idx];} else return 0;
}
void AImg::setdata(void input){data=(void)input;}
void* AImg::getdata(){return data;}
void AImg::plusone(std::string cast){
if (cast=="int")
{
int idata = (int)data;
for (int i = 0;i<N;i++){idata[i] += 1;}
}
else if (cast == "float")
{
float fdata = (float)data;
for (int i = 0;i<N;i++){fdata[i] += 1.0;}
}
//...other types
// careful: N is based on elements of some user-defined type. Not all types have the same precision (#bytes).
// So you'd need to norma

Solution

A few notes about things and stuff (I know, my introductions are always original):

-
About that comment:

//doesn't STL have a std::prod() ?!?!


The standard library does have a function to compute the accumulated product of a range. It's called std::accumulate. You can use it like this:

int n = std::accumulate(
    std::begin(dimsizes), std::end(dimsizes),
    1, std::multiplies{}
);


Agreed, it's a bit verbose. But the function is quite generic since it may work with any well-formed binary operation. Note that if you are using C++14, you can use std::multiplies<> instead of std::multiplies and let the type be deduced.

-
Also, you mentioned std::array. If you know for sure that you will only store 4 values and not more, then please, by all means, use an std::array instead of an std::vector.

-
Your case isn't really consistent. For local variables, the generally accepted practice is to use snake_case everywhere and to drop every capital letter.

-
Ok, I hate to say this, but void AImg::plusone(std::string cast) without templates really hurts me. I know that you plan to change that later, but the current version still hurts me ç___ç Also, since you're not altering the std::string, it would be better to take a const std::string& parameter instead.

-
Instead of that:

int AImg::operator()(unsigned int idx);


Array subcript tends to use the... well, the array subcript operator[] instead of the parenthesis. Unless you have to feed it several parameters, in which case operator() is sometimes preferred, but that's not your case.

int AImg::operator[](unsigned int idx);


-
I don't think that the method setdata really needs a cast from void to void:

void AImg::setdata(void* input) { data = input; }

Code Snippets

//doesn't STL have a std::prod() ?!?!
int n = std::accumulate(
    std::begin(dimsizes), std::end(dimsizes),
    1, std::multiplies<int>{}
);
int AImg::operator()(unsigned int idx);
int AImg::operator[](unsigned int idx);
void AImg::setdata(void* input) { data = input; }

Context

StackExchange Code Review Q#95233, answer score: 6

Revisions (0)

No revisions yet.