Saturday, February 25, 2012

The value of well placed goto.

Some legacy programming languages do not support nor destructors nor exceptions. You must carefully check the returned error codes in case of error you must carefully free all resources. Often this leads to code like this:
error_code =some_func(some_params);
if (error_code!=NOERROR) {
  goto cleanup;
}
error_code =other_func(some_params);
if (error_code!=NOERROR) {
  goto cleanup;
}
error_code =yetanother_func(some_params);
if (error_code!=NOERROR) {
  goto cleanup;
}
//... More code here


There is much code like this. At some point a well-intentioned developer who heard that goto is evil changed the code to look like this:
do {
  error_code =some_func(some_params);
  if (error_code!=NOERROR) {
    break;
  }
  error_code =other_func(some_params);
  if (error_code!=NOERROR) {
    break;
  }
  error_code =yetanother_func(some_params);
  if (error_code!=NOERROR) {
    break;
  }
  //... More code here
} while (0);


In my opinion the do-break-while(0) is much uglier and harder to read than goto.
1. Here break is just a goto in disguise.
2. If you see the beginning but not the end you think there is a loop here.
3. In original version goto is used to control the flow in exceptional cases. Goto does not change the structure of the code under non-exceptional scenario.

What do you think?

Saturday, February 18, 2012

More about globals.

In addition to possibility of an API that silently relies on global variables there are more potential pitfalls.

Suppose that you have a global variable which is modified within ten different if-else blocks. You wrote two tests - in one all ifs are true, in another all ifs are false. Now you believe that you have 100% code coverage.

The real coverage, however is not so good. 10 ifs that manipulate the same globals are semantically similar to a huge switch-case statement with 2^10=1024 possible cases. It will take 1024 tests to give true full coverage.

Tuesday, February 14, 2012

assert(g_everything->isEvil())

I'm in development for 3 months now. I must learn I understood something important. Something that I learned long ago but understood only now. Global vars are evil.

I saw many books about it, but I did not understand. A well placed global variable is very convenient and there is no harm in it. This is what I though when the largest project I experienced was around 2000 lines of code.

If you think what I thought 3 months ago you will hopefully learn from this blog post.

Look at this function signature:
error_code_t loadItemFromDefaultDir(const std::string filename);

As you can see function loads item and returns error code. Where is the item?

if(loadItemFromDefaultDir("it42.item")!=NO_ERR) {
throw load_exception;
}
sub_item_t* mySubItem = g_everything->repository->collections->getSub("it42", MY_SUB_CODE);


You may think there is no problem here, and you are partially right. The problem begins when the item is loaded in one place but the function that uses it is in different module. And the loader does not check the error code. And the item name is not constant, but dynamically created string... You got the idea.

Sometimes you write code like this and it does not work for your argument, but works for any other argument.
convertAndNormilize(&myConvertibleString);

You spend much time looking at the code and finally veteran developer feels pity on you and explains that the string you pass is defined in g_everything->exceptions->conversion_exempt_strings and you must first call
g_everything->exceptions->override_convert_exc_when_norm(myConvertibleString)

Monday, February 6, 2012

C++11 factory

Today I was playing with awesome new features of C++11. In the examples below B is base class and D is derived.
The following code works fine:
std::function<std::shared_ptr<B>()> factory;
factory = [](){return std::make_shared<D>();};

The following code doesn't work:
auto factory = std::make_shared<B>;
factory = std::make_shared<D>;

The first code works because polymorphic wrappers for function objects are smart; the second does not work because function pointers are not smart at all. You can not cast from function that returns derived shared pointer to function that returns base shared pointer, you need function wrappers for it.
I tried to implement parametrized factory method. VC10 does not support initializer so the following did not work yet:
std::map<std::string, std::function<std::shared_ptr<B>()>> factory { 
    std::make_pair("D", [](){return std::make_shared<D>();}),
    std::make_pair("D1", [](){return std::make_shared<D1>();})
};

I'll test this trick with gcc4.6 tomorrow. In VC10 you can initialize the map by hand:
std::map<std::string, std::function<std::shared_ptr<B>()>> factory;
factory.emplace(std::make_pair("D", [](){return std::make_shared<D>();}));
factory.emplace(std::make_pair("D1", [](){return std::make_shared<D1>();}));

Please note the use of emplace method.
You can use factory like this:
auto Bptr = factory["D"]();
//Do something with D object...
Bptr = factory["D1"]();

Unfortunately std::make_unique is not a part of new standard, you can copy the code for make_unique from Sutter's blog. Factories returning unique_ptr are considered better.