3
\$\begingroup\$

I have been coding Java for 5+ years and before that did a little professional C. Picking up C++ again for the fun of it and I feel like I am just writing Java in C++ rather than understanding the C++ way of doing things.

Anyway, I wanted some thoughts on my simple little class to make sure I am C++ing right.

This is my header:

#pragma once

#include <iostream>

#include <SDL.h>
#include <SDL_image.h>

class ImageResource final
{
    SDL_Surface* surface;

public:
    ImageResource(std::string& path);
    ~ImageResource();

    SDL_Surface* get_surface();
};

This is the implementation:

#include "ImageResource.h"
#include <SDL.h>
#include <SDL_image.h>

ImageResource::ImageResource(std::string& path)
{
    this->surface = IMG_Load(path.c_str());
    if (this->surface == NULL) 
    { 
        throw std::runtime_error{ std::string{"Unable to load image: "} + path };
    }
}

ImageResource::~ImageResource()
{
    SDL_FreeSurface(this->surface);
}

SDL_Surface * ImageResource::get_surface()
{
    return this->surface;
}

Things I don't know if I am doing right are const correctness (actually I am pretty sure I am doing it wrong because nothing is const), exception throwing, and general style. Any other comments are welcome.

\$\endgroup\$
2
\$\begingroup\$

Overall, it looks pretty good. Not very much code, so not very much that could be bad. I'll give you some tips, from my perspective:

This first note isn't exactly about C++, but more common than that: It's a good idea to put "public" things first in your class, and more protected things further down. This is because when people look at the class, they are more likely looking for public things, rather than protected/private things. So if you put those at the top, they don't have to scroll through to find them.

Consider making get_surface(); an inlined function, by defining it directly in the header. It's a shame if such a quick and simple function would incur the cost of a function call, and the cost of inlining such a tiny thing is near to nothing.
As I see it, all direct/simple getters and setters should be inlined.

There is no need to write this->, since that is always implied. They only clutter the code.

It is good practice to point out which variables are actually protected members, in the name of the member. I personally put a small "m" in front of all protected members, such as "mSurface". Some people write "m_Surface", or just a "_" in front, with no "m", and so on.

The part that says (this->surface == NULL), you should use nullptr instead of NULL. It's a keyword from C++11, assuming you're using at least C++11. nullptr is a pointer-type, while NULL is simply an int. Writing NULLis exactly the same as writing 0. In some cases, this causes ambiguity, so it's good practice to always use this keyword for null-pointers, since it sometimes actually matters, even if not always.

And as for const-correctness, there is only one function to consider, here. And since it returns a pointer, which could be used to change internal data, it shouldn't really be considered a const member function. (Or "method", to you Java people.)

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review. I keep it short and simple because I have learned that a ten line CR will get useful feedback and a 1,000 CR will get a ship-it. \$\endgroup\$ – Karl Strings Apr 15 '16 at 21:21
  • \$\begingroup\$ @KarlStrings True, but this was probably a bit too short to be useful. A more useful size would probably be 2-3 times as much code. I mean actual code, not #includes" and such. \$\endgroup\$ – antiHUMAN Apr 16 '16 at 12:34

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.