When I created my helper classes, am I over designing?

Question

I am a C++ programmer and recently joined a new company that uses a lot of C. When they reviewed my code, they were thinking I over-designed some of the things which I totally disagreed. The company is doing everything in embedded system, so my code needs to be memory efficient, but the stuff I am doing is not CPU intensive. I would like to know how you guys think what my design. Here is the list.

1. I have some arrays that need to pass around and eventually need to pass to some C code. I could pass a pointer and a size all over of the place. But I chose to create a class that represents this -- a class that has a fixed size (we know the maximum size) buffer, and a length which should be always <= the size of the buffer, otherwise assert. In this way, I can pass the array around with only one variable instead of two, and if the maximum size changes in the future, I should be able to change it easily. I don't use dynamic allocation for the array because it is embedded system and memory allocation could potentially fail and we don't use exception. The class is probably less than 30 lines code, and I use it for quite a few places. They said I over-designed it.

2. They have their own containers implementation in C. I needed to use one of them, but I wanted to hide all the detailed code away from my main logic, so I created a wrapper class for it. The wrapper class is similar to stl, so I have iterators and it manages the memory allocation internally, but unlike stl, it returns a error code when it can't allocate more memory. Their argument on this one is that I am the only one uses it, so they don't want it to be in the repository. I found it stupid to be honest.

EDIT: The following class is more or less that I used for point 1. All I wanted to do is to have something to pass around without carrying the length all the time.

class A
{
  static const MAX_SIZE = 20;
  int m_Array[MAX_SIZE];
  size_t m_Len;

public:
  A(const int* array, size_t len)
  {
    assert(len <= MAX_SIZE);
    memcpy(m_Array, array, len);
    m_Len = len;
  }

  size_t GetLen() const { return m_Len; }
  const int* GetArray() const { return m_Array; }
};

Answer 1

First things first: You joined a new company, so you can expect that you need to learn to play by their rules. You're still "the new guy" and there will be some resistance to "your way" of doing things, even if it is better. Get used to them and slowly integrate yourself and your ideas.

As to #1, passing a pointer+size is certainly a pain for the programmer, but it's the most memory-efficient way of doing things. Your class is not "over"-designed, but what happens if MAXSIZE becomes really large at some point in the future? All of your instances will take that much space each, even when they don't need to. You could wind up running out of space simply because the MAXSIZE changed, even if nothing needed that much space.

As to #2, it's a tossup on whether it's an unnecessary layer (perhaps it would be better suited to improve their wrapper instead of simply wrapping it again?), but this will come down to how well you integrate with them and make suggestions.

In summary, I wouldn't call it "overdesigned", but in an embedded situation you need to be very wary of generalizing code to save yourself effort vs saving memory..

Answer 2

You're probably right, but on the other hand if everyone in the company decided that they don't like the existing APIs, and each designed their own shims and helper functions, that only they used, then maintenance would be tricky.

If your array wrapper is "over-designed", then I'd question whether the code reviewer considers any amount of design to be acceptable. It looks harmless to me[*]. I suppose you could have just made it a struct with two public members, and lose the benefit of read-onliness. How keen are your colleagues on const-correctness in general?

I think the goal for 2 should be to reach consensus on whether that C API should be used directly from C++, or wrapped. If it should be wrapped (and the arguments for that are probably quite strong, what with namespacing and RAII), design a wrapper that everyone will use, and designate it "the C++ API for this module" rather than "a C++ wrapper that one module uses for this other module".

It's possible that everyone else genuinely prefers the API as it is, over a more OO API or using STL. Following their conventions will make it easiest for them to maintain your code, as long as their conventions are solid C programming style. C++ is a multi-paradigm language, and "C with a limited number of bells and whistles" isn't the paradigm you're used to. But it is a valid paradigm, and if it's what the existing codebase uses then you have to question whether what your company needs right now is a one-man revolution, however enlightened.

[*] (the API, that is. They might question whether it will be passed by value inappropriately, and whether it's wise for every instance to have to be as big as the biggest. That's all for you to argue out with the reviewer, but is nothing to do with "over-design".

Answer 3

A c++ wrapper for the already existing c-style container-library is a nice thing to have. But make sure it's really a wrapper, not a wrapper with bolted on bells and whistles.

If you declare all your access functions as inline and write your wrapper using the leanest possible implementation there should be exactly zero overhead in code and data-size.

Such a wrapper will just give you a class-like interface to the c-library, and that's good practice for a C/C++ project.

I suggest to check your code. Take a look at the disassembly of a little test-case using your wrapper and compare it to a c-only implementation. If you see any additional complexity, allocations or calls to your class implementation you have overhead and maybe overengineered a bit.

Proving that your wrapper generates more or less identical code compared to the c-version may convince the other programmers that a wrapper here and there does no harm. It follows the way C++ programs are written and will result in a cleaner and more consistent code-base.

Btw - The "we don't want no useless wrapper-class in our repository" thing is a social problem, not a technical one. You're the new guy. If I would be in your position I would play by their rules for a couple of month and - after I've proven myself - introduce them to your ideas.

Answer 4

On the one hand, if you're comfortable with C++ and they use C, then helper classes are probably a good thing, for you at least. On the other hand, if you're writing any sort of container class, you're almost certainly better off using STL directly and writing a helper function to bridge the gap between your STL container and the C code. Don't reinvent the wheel.

Answer 5

"Over designed" is very relative. The C-guys in your group probably see a problem with your class because when you pass your class by reference, you're going to make a copy of that entire array. That'll can a fair amount of time on an "embedded" system (depending on your definition of embedded) Of course, if you pass by address, this isn't a problem.

Your #2 is a cultural issue. If they're not willing to try new programming techniques, then you're going to be viewed as an outsider. That's something you don't want to happen. Introduce them slowly.

As for not putting it in the repository - that's bullshit. Everyone should put everything in the repository so that people can access it and find bugs later.

Not to toot my own horn, but I asked a related question just a few days ago: Using C++ in an embedded environment

Answer 6

I am unsure about 1. Maybe you could give a simple code example to illustrate.

As for 2, I agree with you. As a hardened C programmer who's trying to get up-to-speed on C++, I am totally sold by the idea of writing thin facades around existing C APIs, for ease of error-handling and resource management.

Answer 7

You make a copy of the data for your class, which no longer qualifies your class as being a mere wrapper class. Although this will protect you from the data getting deleted out from under you, if the original data was modified, your data would be out of date.

The idea of encapsulating the array with the size is reasonable. Perhaps you can get them to buy into adding the size field to their container, assuming it's not just a const int*. But I can see that for the mere privilege of encapsulating the size, the overhead of extra memory and runtime to model and initialize the class may seem unpalatable for them.

Beyond that, I have to sympathize completely as far as dealing with the classic C-bias, which is that C++ is evil and slow. Flashbacks...shudder.