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

Refactoring image loading/manipulation class

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

Problem

I have this Image class that I've been using for some time now on personal
projects and games I eventually write. It was written in C++98 style and it
is starting to show its age. Over time, I added a few C++11 elements here and there,
but I think it is now due for a major refactoring.

image.hpp:

```
#ifndef IMAGE_HPP
#define IMAGE_HPP

// Dependencies:
#include
#include

// ======================================================
// PixelFormat:
// ======================================================

//
// Supported internal image and texture formats.
//
struct PixelFormat
{
enum Enum
{
// RGB images are composed of 3 color components.
// Each component can be a byte [0,255] or a normalized float [0,1].
RgbU8,
RgbF32,

// RGBA images are composed of 3 color components and
// 1 component for the transparency level (alpha).
// Each component can be a byte [0,255] or a normalized float [0,1].
RgbaU8,
RgbaF32,

// Sentinel value. Not a valid pixel format. Internal use.
Invalid = -1
};

// Converts a pixel format enum value to a printable string, for debugging purposes.
static const char * toString(PixelFormat::Enum pixelFormat);

// Returns the size in bytes of a single pixel of a given pixel format.
static size_t sizeBytes(PixelFormat::Enum pixelFormat);

// Returns the number of color components of a pixel format.
static uint32_t componentCount(PixelFormat::Enum pixelFormat);
};

// ======================================================
// TPixel templates:
// ======================================================

// RGB pixel:
template
struct TPixel3
{
T r;
T g;
T b;
};

// RGBA pixel:
template
struct TPixel4
{
T r;
T g;
T b;
T a;
};

// ======================================================
// Image:
// ======================================================

//
// Represents and array of pixels.
// Images ar

Solution

If this code is working. Then DON'T do any major refactoring without unit tests.

You can add your standard modernizations that are easy to show work:

Use Copy and Swap Idiom on the assignment operator:

Image & Image::operator = (const Image & other) 
{
    freeImageStorage();   // This is not exception safe.
    initFromCopy(other);  // If the initFromCopy() throws then you are in a bad position.
    return *this;
}


Easy re-write:

Image & Image::operator = (Image copy) 
{
    copy.swap(*this);
    return *this;
}
void swap(Image& other) noexcept
{
    using std::swap;
    swap(data,          other.data);
    swap(dataSizeBytes, other.dataSizeBytes);
    swap(width,         other.width);
    swap(height,        other.height);
    swap(pixelFormat,   other.pixelFormat);
}


You could also add Move Semantics. (I did this to some old code and got a 30% boost in speed by doing nothing else but recompiling with C++11 on).

Image(Image&& other) noexcept
    : data(null)
{
    other.swap(*this);
}
Image& Image::operator=(Image&& other) noexcept
{
    other.swap(*this);
    return *this;
}

Code Snippets

Image & Image::operator = (const Image & other) 
{
    freeImageStorage();   // This is not exception safe.
    initFromCopy(other);  // If the initFromCopy() throws then you are in a bad position.
    return *this;
}
Image & Image::operator = (Image copy) 
{
    copy.swap(*this);
    return *this;
}
void swap(Image& other) noexcept
{
    using std::swap;
    swap(data,          other.data);
    swap(dataSizeBytes, other.dataSizeBytes);
    swap(width,         other.width);
    swap(height,        other.height);
    swap(pixelFormat,   other.pixelFormat);
}
Image(Image&& other) noexcept
    : data(null)
{
    other.swap(*this);
}
Image& Image::operator=(Image&& other) noexcept
{
    other.swap(*this);
    return *this;
}

Context

StackExchange Code Review Q#67843, answer score: 10

Revisions (0)

No revisions yet.