I recently interviewed with a CAD-based studio where I live for a C++/Renderer position. The role was to work on their internal 3D rendering code that's written in DX/OpenGL. They asked for a code sample of one of my personal projects. I sent them one of my projects. It's a basic, open source, texture packer application using Qt to provide the UI. It's not much, but at least it's pretty complete. The CEO got back to me about 10 days later, and said I was too junior for their company at this time. They're looking to fill a senior position first, then they'd consider bringing me on to get mentored.

I was only a little disappointed at first, but more curious than anything. I've been programming since middle school, but I'm only 2 years into programming professionally full-time. I've worked in 2 smaller/startup companies in this time. None of my work has been C++ or lower-level rendering-oriented, so it didn't surprise me when they turned me down. I know I'm not the best C++ programmer out there. I'd like to have some one with better knowledge of C++ to point me in the right directly as far as decent C++ practices go. I don't know if there are any good resources for this online, so I wanted to ask for one in this thread, if that's allowed. I'd really appreciate it if anyone could give me feedback. It can be downloaded here. If anything, you'd get a texture packer with source out of it.

I'd venture to guess that your lack of experience (two years) contributed fairly significantly to your being considered "too junior," but the demo you submitted probably didn't help. Not necessarily because of the code itself (which I have not even looked at yet), but simply because of what it is: texture packing is not that advanced a topic unless you're actually demonstrating some new, novel mechanism of solving the bin-packing problem efficiently (which would still probably be mostly of note to somebody in academia). It's also not hugely relevant to low-level rendering.

I'll take a look at the code later on and see if I can give you some feedback.

Quick glance at the code, you pass parameters by value specially std::string, you do not use const anywhere including marking methods which do not change state of an object. Those two things would be give me the impression that you are a junior.

@DoctorGlow

(Offtopic Question probably)

std::string should be passed by reference? Or should you even pass a pointer to a string instead of the string itself? I always considered a string as a primitive type so I did neither...

Starting in BinPacker.h.

• Identifiers beginning with an underscore followed by a capital letter are reserved in all scopes (i.e., don't use _FOO_..." header guards).
• BinRect inheriting Rect seems is an abuse of inheritance as code-reuse; why not just contain a rectangle instead (same with BinNode, later).
• What is 'rotated," why is it a uint8_t that you XOR 1 into and check for > 0? Is this just a "clever" boolean? If so, use a boolean, if not then this is precisely the kind of weirdness a comment should explain somewhere.
• You don't need to check for null before deleting a pointer; deleting a null pointer is safe and defined.
• Unclear why CompareBinRects needs to be a template since you demand they're both of the same type anyway.
• Similarly, I'm not sure why BinPacker needs a template argument. I'm starting to get real worried about the subclasses of BinRect I'm going to find, as that virtual CanPack() on it suggests bad things.
• Why does BinPacker store pointers to rectangles, which you delete in the constructor? In Add you simply copy them; this looks like sloppy ownership management that can lead to double-delete bugs and other pointer tomfoolery. Unclear that pointers are necessary here at all, especially since you already have compile-time polymorphism via the template parameter *and* not much of BinRect is actually virtual.
• "Force" is a verb. "ForceSquare" sounds like it might actually change the packing to demand everything is square, but it doesn't, it just sets a state that is presumably read later. This is not the most clear name.
• If indices can't be negative (looking at Remove), why are they even signed values?
• If an index is out of range, that's an error. Treat it like one. By quietly forcing the index to be in range instead, you are potentially hiding bugs in calling code that is not dealing with the index range properly, which means in the face of such a bug you or your team will spend longer trying to track it down. Assert or throw an exception or return a failure code.
• Don't print to stdout in utility code. Throw an exception or return an error code.
• If a null rectangle pointer is invalid, prevent a null rectangle pointer from being added to the list in Add (or better yet, don't take pointers here at all). Checking during Commit is wasteful and lets bad data or bugs manifest further from their origin.
• CompareBinRects is used internally; if it has no external utility, don't expose it as part of your public interface.
• That root BinNode is allocated, used to call a method, and deleted. It doesn't need to be on the heap at all.
• GetNumPackedRects is too long to write on a single line. And it looks like it always returns 0 in a very complicated fashion.

Moving to BinPacker.cpp:

• Mainly more of the same critique, especially concerning writing to stdout for "errors."

Matrix.h

• Consider different names for the Ortho overrides, as they take slightly different parameters and it's not going to be exceedingly obvious which is which at any given call site.
• The matrix class itself provides no hint as to its storage conventions, et cetera (column major, row major). It offers almost nothign over a bare array of 16 floats.

SpritePacker.h

• Ah, here we go, a BinRect subclass. So this looks very much like conflating responsibilities and violating SRP all over the place. BinRect has a virtual CanPack just so SpriteBinRect can check a texture? Why does a texture matter? A rectangle packer packs rectangles. That some of those rectangles may be associated with textures is irrelevant and unneccessarily couples otherwise generic code to that concept.
• The result of texture != 0 is itself a boolean; there is no need for the subsequent use of the ternary operator to simply select true or false.
• Somewhat excessive passing of std::string by value.
• SpritePacker as a subclass of BinPacker has the same problems as above (for SpriteBinRect).

SpritePacket.cpp

• INPLACESWAP is a "clever" trick and not a good idea (especially when trying to swap the same things). Also, why the sudden capitals?
• Consider moving the definition of the vertex/pixel shader code to some static constant area.
• I'm going to skip the rest of this file as it seems to mostly be graphics stuff.

Types.h

• The assignment operator should usually be T& operator=(const T& other); yours (in your color structures) copy excessively.

I skipped anything that looked like Qt code, because meh.

Most of the above is not too bad, the biggest problems I would say are the way you use run- and compile-time polymorphism, the way you deal with ownership, and the apparent consideration you put in to the shape and surface area of your APIs. Those three things, especially together, do suggest a certain amount of inexperience. I still think the nature of the demo was probably more problematic for your getting the job than the specifics of the code. How much the specifics of the code matters is really up to the individuals reading the code; some people will be more picky than I am (even I would be more picky in an actual interview scenario, versus scanning this on my lunch break), but some would be less and only look at the algorithmic details.

That said, I hope you'll find my comments informative.

@DoctorGlow
(Offtopic Question probably)
std::string should be passed by reference? Or should you even pass a pointer to a string instead of the string itself? I always considered a string as a primitive type so I did neither...

std::string str = "wololo";

// pass-by value - the string is copied
void foo(std::string bar);
foo(str);

// pass by const-ref - the string is NOT copied, and the function accesses it in place
void bar(const std::string& foo);
bar(str);

Sorry if this is tl;dr. It took me 3 hours to write up a response. Took it to heart though.

I'd venture to guess that your lack of experience (two years) contributed fairly significantly to your being considered "too junior," but the demo you submitted probably didn't help.

This is what I thought too. The sprite packer and signed-distance font generator were the only two presentable projects I really had. All of my ambitious projects break over time due to underlying code changes from a shared code base. I'm fixing that by providing version control --something I never really used until 2 years ago.

Quick glance at the code, you pass parameters by value specially std::string, you do not use const anywhere including marking methods which do not change state of an object. Those two things would be give me the impression that you are a junior.

AND

std::string are objects (class) and can be expensive, specialy if it needs to copy data, so I "always" pass by const ref.

Another big concern of mine. I know that passing things by value's kills performance, but I haven't wrapped my head around how to use references, const references, and const getters effectively. I was going to make this a future priority, but since that half the replies are about how to properly pass data into functions, it sounds like it's a fundamental I need to work on right now.

I can see how passing by const reference acts as a security concern, but does it also increase performance as well making it read-only? The same goes for plain references: security in the sense that they're restricted pointer. They can't be re-assigned after assignment, no direct pointer arithmetic, can't ever be NULL, and can't take the address of references. These points were brought up in this Stack Overflow thread, but I'm not sure what the 3rd point means. Also, is it proper to return const references/pointers from methods? I've tried wrapping my head around this as well.

There are a lot of things I'll write, but I'll write them in weird ways because I'm unsure of the proper way for it to be written in certain situations. Josh brings up some of them.

@Josh Petrie: Thank you for taking the time during your lunch break to go over this. There are plenty of gems here for a junior like myself, and I did find your comments very informative. Here are my responses:

BinPacker.h

• I wrote my header guards like this since that's how I saw them early-on in C++ tutorials. As time went on, I adopted this convention. Then again, I've seen plenty of examples online that don't use underscores like this. It sounds like a potential issue for some compilers, so I'll remove both the preceeding and trailing underscores.

• Regarding BinRect and inheritance abuse: I agree with you. I thought about using composition here as I think simple types like VectorX, MatrixX, ColorX, Rect, etc should all be final classes. I should be implementing that as well. When I wrote this code back in November, 2014, I was just getting exposed to the idea that inheritance isn't the end-all solution in C++. I tried staying away from it, which is why BinPacker is generic, but again, inheritance prevailed as a design choice in every class contained in the BinPacker module. Providing a HAS A Rect relationship with BinRect and BinNode should help fix some points that you bring up later.

• 'rotated' is supposed to be treated as a bool value. I use uint8_t because I wanted it strictly typed for POD purposes so it's more portable when exporting to a file. I try to stay away from bool when working with POD due varying to compiler implementation. bool can vary between platforms/implementations, so I wanted to strictly type something to an unsigned 8-bit value. Also, it doesn't seem like I can XOR bools on some compiler. I use XOR to toggle rotation state of my BinRects. If using a bool, I could toggle simply by myBoolVar = !myBoolVar. Checking > 0 is true, but again, I should just check if(rotated == true) { }. Would a bool still be a better way to go? It'd certainly make it more readable.

• I always thought I had to check if my pointer was NULL before deleting it. Since this isn't the case, I'll remove the checks.

• You bring up a good point about CompareBinRects() as a template is unnecessary. I can't remember why I even did that, but again, making it a template function doesn't make sense. I moved it into BinPacker as a private method.

• BinPacker is a template because it's meant to take a subclass BinRect. BinRect::CanPack() was a hasty addition to the original design because I realized that some rects included in the rects vector shouldn't be packed due to some sort of invalidation in some implementations of this class. This is obviously poor design. Rect validation should be done before adding them to the packer.

• I agree with ForceSquare(): it sounds like an action. I was uneasy when I wrote it. It does set a state that'll be applied when Commit() gets called, but again, doesn't perform any action. This should be renamed something better, such as ForceSquareFlag() maybe?

• I agree with using "unsigned" when negative values should never be used. I only kept it signed as I thought this was a common convention, and also performed better. I remember reading about signed vs unsigned ints, and performance impact a while ago when I did PSP development. I'm probably wrong. I'll use "unsigned" in places where values should be unsigned, such as looking up an array item by index. Everything should be [0, max value), but don't some compilers yield a warning if I use an unsigned datatype instead of a signed int?

• I've gone back in forth between throwing an error, and bounding indices that are out-of-bounds. I'll throw an error instead. I'm starting to use assert() for rare situations like this. I stay away from exception handling in commonly-used code as it can be slow. Does this sound practical?

• I agree with logging in utility code: it shouldn't be used, but still should be reported. I'm a fan of error codes, but there are caveats to that such as providing a bunch of different defines. I'd like to use C++11 enum classes as error codes here, but I'm not sure if that's good design simply because I haven't seen that yet. I'm sure exceptions have their purpose (I use them in C# in cases where I'd typically use assert() in C++). What is your take on exception handling?

• I agree, NULL rects shouldn't be allows, and it'd be a much better practice to use a vector of objects instead a vector of pointers in this case. The only reason I'm doing this is because BinRect was originally intended to be subclassed. In reading your feedback, inheritance doesn't sound like a solid design. If I were to store objects instead of pointers in my vector, how should I match up those rects with my images or glyphs? The rects are re-ordered by size to make the packer work. I thought of providing a separate class to store my sprite info that gets "packed" by storing a pointer to the corresponding BinRect that actually gets packed, but if I'm pushing objects into a vector that could potentially resize. This could move my objects around in memory; invalidating my sprite info objects' pointers. Should an STL list of objects be a better solution? I try to stick with vectors as much as possible for cache reasons.

• I moved that function into BinPacker as a method since it's only practical purpose is specific to that class.
• Good call with the BinNode root. It'll become part of the stack.
• Again, right about GetNumPackedRects(): it literally does nothing but return zero. I forget what that was even supposed to do. It's not even used in the entire applications. It's deleted.

BinPacker.cpp

• Should have seen that one coming. I'll go with error codes for BinPacker::Divide(). It currently only returns a bool value, but I could convert that into error codes to get more specific on what went wrong.

Matrix.h:

• I've recently started to get away from being too vague about naming conventions, specifically functions/methods. I'd try to overload methods as much as possible, but I've seen cases where I've gone overboard in other projects. By naming these methods.
• I see your point, and I could provide documentation on this. This class uses its data in a column-major format as every graphics API I've seen uses the column-major interpretation (DirectX, OpenGL, the PSP's GU). I've considered this a given, but you're right: it wouldn't hurt to specify somewhere.
• NOTE: Just like all other complex math types I have in classes, they all have operator overloads. There are many opportunities for const reference parameters.

SpritePacker.h

• I was wondering if SpritePacker would violate SRP. In fact, I'm not very confident in my philosophy of the single responsibility pattern to derived classes without violating SRP. The CanPack() virtual method will be removed, as stated above. I initially thought the same thing: the packer should only pack rects, not sprites. I abandoned that philosophy because I wasn't sure how to associate sprites with packed rects if my BinRects are moved around in memory as a vector (might have to go with a list on this one, unfortunately). The idea was that SpriteBinRects could be pushed into the SpritePacker even when a texture wasn't working. Of course, if a texture were to fail, the SpriteBinRect shouldn't be instantiated in the first place.
• I thought the same thing about the ternary state: it returns a boolean value. The only reason I explicitly specify true : false is because I think I ran into a compiler error when assigning a bool to an expression evaluation.
• Again, you're right, passing by value is largely unneeded. I'm cleaning this up as I get the time to do so. Hopefully I'll have an amended version of this code completed by the end of the week.
• Yeah, SpritePacker will suffer the same issues from BinPacker.

SpritePacker.cpp:

• INPLACESWAP and SwapRedBlue32 aren't my own code. I took it from FreeImage, and put it in my code-base since these functions aren't always available in every version of FreeImage's source code. I need it to swap Blue/Red components since FreeImage has the nerve to store those components in a BGR format. Some people... lol jk, FreeImage has been VERY useful.
• I've wondered about where to put the vertex/shader source code. I put it in InitCommon() because it doesn't need to exist past setting up a few stock shaders. After that, the data isn't needed anymore. I let it release from the stack after InitCommon() reaches the end of its scope. Do I still need to move it somewhere as static common data? It'd only eat up a few KB of memory if I did that.

Types.h

• I setup the assignment operator overloads properly where the parameter takes a const reference, and returns a reference. I've seen this in others' code (tinyxml, FreeImage, STL, Box2D, etc). Again, I've stayed away from references and const references out of lack of understanding. As stated above, it sounds like this should be a top priority.

Most of the above is not too bad, the biggest problems I would say are the way you use run- and compile-time polymorphism, the way you deal with ownership, and the apparent consideration you put in to the shape and surface area of your APIs

I still struggle with all of these. I can nail a lot of the simple stuff, but I don't most of the time. I've been programming in C++ as an obsessive hobby since I was in middle school, about 11 years ago. That said, I still do consider myself novice. I've started to notice that I need to get feedback from other programmers to grow, otherwise I'll just be stuck in my old ways. I just gotta face that I'm not John Carmack --I'm just a regular guy lol. I'll to ask more questions, and apply the advice in smaller test cases to reinforce what I'm being taught.

I can't thank you all enough for the feedback.

I'd like to have some one with better knowledge of C++ to point me in the right directly as far as decent C++ practices go.

I think it's a pretty good idea to read Scott Meyers's Effective C++.

It's easy to read, there are short and easily understandable tips which can help you improve your code.

Another big concern of mine. I know that passing things by value's kills performance, but I haven't wrapped my head around how to use references, const references, and const getters effectively. I was going to make this a future priority, but since that half the replies are about how to properly pass data into functions, it sounds like it's a fundamental I need to work on right now.



I can see how passing by const reference acts as a security concern, but does it also increase performance as well making it read-only? The same goes for plain references: security in the sense that they're restricted pointer. They can't be re-assigned after assignment, no direct pointer arithmetic, can't ever be NULL, and can't take the address of references. These points were brought up in this Stack Overflow thread, but I'm not sure what the 3rd point means. Also, is it proper to return const references/pointers from methods? I've tried wrapping my head around this as well.

Wrapping your head around when to use (const) references is really easy.

For primitive values and small structs (something like 1-3 four-byte values), you pass by value, unless you want to modify the variable you are passing. In this case you pass by reference, unless the parameter is optional, in this case you pass by pointer.

For larger classes/structs, you generally always pass by at least const reference. Unless you want to modify the class of course, then you pass by reference. Unless again the value is optional, in which case you pass by pointer.

Performance in this regards will be increased when you pass stuff like std::vector, std::string which can take a large amount of time to copy. I wouldn't say they exactly increase security, since you can do stuff like this unfortunately:

void takeString(std::string& string)
{
    string.clear();
}

std::string* pString = nullptr;

takeString(*pString); // crash inside the function as if you were passing nullptr

but they document intent by telling the user of your function "the value of this parameter is required", while using only pointers technically make it unclear whether or not they have to take a value or can be nullptr.