Dealing with Legacy Code Part 2: Extract the API out of your interfaces


Think about the following situation:

There is an interface / class interface / function in your application used by others which conains non-standard enums / types / whatever coming from an external API like OpenGL, DirectX, some kind of commercial API. The underlying code strongly uses this API. So using parts of this API was straight forward. Instead of having a strong encapsulation your API was spreaded all oer your codebase. Unfortunately the Product-Manager generates this requests: we need to exchange the underlying API because of whatever … Easy, just change this one API below your code and … wait a minute. There are all these symbols coming from the old API. WTF!

It looks like:

enum class API1Enum {
  feature1,
  feature2
};

class MyInterface {
public:
  void enableFeature( API1Enum myEnum ) = 0;
};

// Implementation for API1
class MyAPI1Impl : public MyInterface {
  void enableFeature( API1Enum myEnum ) {
    // handle the enum from the API1
    switch (myEnum) { ... }
  }
};

For the first implementation this concept works fine. Now you get a new API to wrap, just implement the interface, right?

class MyAPI2Impl : public MyInterface {
  // Damned, API1 does not exist anymore ...
  void enableFeature( API1Enum myEnum ) {
    ..
  }
};

Solution: Wrap the enum as well:

Of course you can introduce another app-specific enum, wrap the API by using it and you are fine:

// From the API
enum class API1Enum {
  feature1,
  feature2
};

// defined in your app
enum class AppEnum {
  Appfeature1,
  Appfeature2,
  AppNotSupported
};

// change your interface
class MyInterface {
public:
  void enableFeature( AppEnum myEnum ) = 0;
};

// Introduce functions for translation
static API1Enum translateAPI1( AppEnum type ) {
  switch( type ) {
    case AppEnum::Appfeatue1; return API1Enum::feature1;
    case AppEnum::Appfeature2: return API1Enum::feature2;
  }
  // error handling
}

static API2Enum translateAPI2( AppEnum api2_type ) {
  // do it for API2
}

class MyAPI1Impl : public MyInterface {
  void enableFeature( AppEnum myEnum ) {
    // translate AppEnum and handle it for API1
    switch (translateAPI1(myEnum)) { ... }
  }
};

class MyAPI2Impl : public MyInterface {
  void enableFeature( AppEnum myEnum ) {
    // translate AppEnum and handle it for API1
    switch (translateAPI2(myEnum)) { ... }
  }
};

What you are doing:

  1. Introduce an APP-specific enum
  2. Use this enum instead of the API-specific enum
  3. Introduce translation functions to translate the AppEnum to the API1Enum / API2Enum
  4. For the API-specific implementations: translate the AppEnum into the API-specific enum

Optimizations?

You can use lookup-tables instead of the translation-functions. But as a first step translation-functions a much easier to debug.

Dealing with Legacy Code Part 1: Get started


What is this all about?

At the moment I have to deal with legacy code during my day-job. And I got the idea that, maybe, some other people are interested how I solved all the different issues as well. So I decided to start a serie of blogposts about the different kind of challenges I faced and how I dealt with them.

Oh my fucking god, what a mess …

Maybe you know the following situation as well: your boss told you to take care of this suspicious project no one in the company wants to touch. When people hear about your new task they look like you told them that you gonne die a painful dead at the weekend. So WFT!
You try to find the repo and there is no. There is only a folder containing some binaries and a zip-file containing the source. Or there is a repo with only one version checked in some weeks ago. Or there are different named folders with version names in it containing the source.  WFT, again!
You will try to open the project and you don’t have a clue how to do this. No obvious entry points how to start. WTF, WTF, WTF!
You want to build the project and you cannot find any makefiles at all. The you find the makefile but not documentation how to run the build. Or you are able to runf the build but the 3-party-libs are not there.
After a hard week you are going into your weekend. You think about quitting the job and try to get a shepherd in Australia. But stop! Next week we will start to get out of this mess.

Getting back control: try to build it

The first thing what I try to understand is to get an overview about the project. Often I saw often used peaces of code next to dead ends, commented code or just experiments, which are not in use any more. The makefile / build description shows you which parts are currently in use for the production code. In the companies I worked people are using one kind of build system in a bundle of projects. If there are still colleages left which dealted with this project you can ask them which how this worked. For instance if a GNU-make was in use you can grep ( see How to use grep on linux or How to use findstr on Windows ) to look for Makefile. Visual Studio project files are using the sln-extension. Grep for it and try to run the build.
Copy the build output and look for errors. Each build error provides information about the struture of your project:

  • Dependencies are missing:
    • The project is using the following dependencies
    • Try to find it and intregate is as well
  • The project is looking for different includes ( in c++ ), modules ( python ) or packackes ( Java / C# ) and the build cannot find them:
    • The project is using an environment variable to locate different packages
    • Identify the packages and install them
    • Modify your environment until it works
    • If this creates issues: try to run your build in verbose mode, this helps a lot to analyze what is going wrong
  • The source does not build, syntax errors all over the place:
    • Which version of the language was in use? Any extensions? For instance if you are using Cuda-Code in c++ you need to install a special compiler for that.
    • Try different compilers, some code which build fine with VS-2010 does not build when using g++ / clangs
    • Are the syntax errors caused by OS-specific defines? Identify them
    • Try to find them,  Stack-Overflow can help you
  • Linking / assembling fails:
    • Try to answer:
      • Are you sure that you have run all makefiles?
      • Do you need to copy stuff manually?

Still does not work…

.. but you learnd some stuff about your project. Hopefully you have some better understanding.

A new concept for testing Asset-Importer-Lib


Our problem!

After struggeling with bugs in assimp for more than 10 years until now I have to accept the fact: I need to improve our unittests + regression-test-strategy:

  • New patches are breaking older behaviour and I haven’t recognized it until a new issue comes up created by a frustrated user.
  • I do not have a clue about most of the importers. I didn’t implement them and it is really really hard to get in the code ( special when there is no up-to-date spec ).
  • When not knowing the code of a buggy importer it is also hard to start a debugging session for tracing issues down to its root cause.

Yes, these are all signs of legacy code :-)…

So I started to review our old testing-strategy:

  • We have some unittests, but most of the importers are not covered by these tests.
  • We have a great regression testsuite ( many Kudos to Alexander Gessler ). When running this app it will import all our models and generate a md5-checksum of its content. This checksum will be saved in our git-repo. For each run the md5-checksum will be calculated again and the testsuite checks it against the older value, a mismatch will cause a broken test. Unfortunately also a new line-break will cause a different checksum as well and the test will fail. And what you see is that a model is broken. You do no see which special part of its importer or data. But some model-importers have more than 1000 lines of code …
  • This regression test suite was part of our CI-service. Without a successful regression-run it was possible to merge new pull-requests into our master-branch. In other words: we were blocked!
  • We are doing manual tests …

So I started to redesign this kind of strategy. A lot of people are spending hours and hours to support us and a lot of software is using Asset-Importer-Lib. So for me as a software-engineer it is my duty to guaratee that we are not breaking all your code with every change we are doing.

So I will change some things …

The idea

  • Measuring our legacy: how much code is really under test-coverage. I started this last week and the result ( 17% ) is nothing I am proud of.
  • Add one unittest for each Importer / Exporter:
      • For this task I have added a new test-class called AbstractImportExportBase:
    class AbstractImportExportBase : public ::testing::Test {
    public:
        virtual ~AbstractImportExportBase();
        virtual bool importerTest() = 0;
    };
    

    All Importer tests will be derived from this class. As you can see the importerTest-method must be implemented for each new importer test. So we will get a starting point when you want to look into a special importer issue: look for the right unittest: if you can find one: start your investigations. If not: build you own test fixure, derive it from AbstractImportExportBase and make sure, that there is an importer test. At the moment this is just a simple Assimp::Importer::ReadFile()-call. But to make sure that the results will not change there is another new class.

  • Introducing SceneDiffer: This class is used to evaluate if the result of an import-process creates the expected result. You can declare which results you are expecting. So in each unittest this result will be checked against the expected data. When something breaks you can recognize this in executing our unit-test-suite. And the best part: you will see will part of the model data has changed.
  • Use Static-Code-Analysis via Coverity: A cron-job will running a Coverity-Analysis once a week to make sure that new commits or pull-requests haven’t introduce too much new issues.
  • Run the Regression-Test-Suite once a week: the same cron-job, who will trigger the coverity-run will run the regression test suite.  When some files will generate different results or just crashes you can see it and investigate this model in our unittest-suite. The regression-test suite was moved into a separate repo to make it more easy to deal with it.
  • Run the Performance-Test-Suite once a week: I introduced a new repository with some bigger models. The same cron-job for the static-code-analysis and for the regression-test-suite will trigger this import as well. The idea is to measure the time to import a big-big file. When after a week the time increases, someone introduced some buggy ( for importer code slow code is buggy code ) code and we need to fix it.
  • Release every to weeks: in the last couple of months I got a lot of new issues, which were reports of already solved issues solved on our current master branch. But the released version was outdated ( 2-3 months behind the latest master version ). To avoid this one release every two weeks could help our users to keep up-to-date without getting all the issues when looking on an experimental branch. The release process shall be run automatically. Until now there is no Continuous-Delivery-Service to generate the source release, the binary release for our common platforms and some installer for windows. Special the several deliveries to different platforms generated most of our new issues after releasing a new version. So doing this automatically will test our devilervy-process as well.

I already started to implements the unit-tests and the SceneDiffer-class. And I am using our unittests to reproduce new issues. When fixing them the test to reproduce the underlying issue is checked in as well.

Hopefully these things will help you Assimp-users to get a better User-Experience with Asset-Importer-Lib.

Feel free to give me any kind of feedback …

Getting starting with a Legacy-Code-Project


Day zero

Imagine the following situation: you are starting a new job, you are looking forward to your bright future. Of course you are planning to use the newest technologies and frameworks. And then you are allowed to take a first look into the source you have to work with. No tests, no spec, which fit to the source, of course no doc but a lot of  ( angry ) customers, which are strongly coupled to this mess. And now you are allowed to work with this kind of … whatever.

We call this Legacy-Code and I guess this situation is a common one, every developer or most of them will face a situation like this during his/her career. So what can we do to get out of this? I want to show you some base-techniques which will help you.

Accept the fact: this is the code to work on and it currently solves real problems!

No developer is planning to create legacy code. There is always a reason like: we needed to get in buisness or we had failed. Or the old developers had not enough resources to solve all upcoming issues or develop automatic tests. 10 years ago I faced this situation again and again: nobody want to write automatic tests because it costs a lot of time and you need some experience how to design your architecture in a way that its testable. And there were not so much tools out in those days.

The code is there for a reason and you need to accept this: this working legacy code ensures that you got the job. So even when its hard try to be polite when reading the code. Someone invested a lot of lifetime to keeps it up and running. And hopefully this guy is still in the company and you can ask him some questions.

You can kill him later ;-).

Check, if there is any source-control-management

The first thing you should check is the existence of an Source-Control-Management-Tool like Subversion, Git or Perforce. If not: get one, learn how to use it and put all your legacy code into source control! Do it now, do not discuss. If any of the other developers are concerned about using one install a SCM-tool on you own developer-pc and use it there. I promise: it will save your life some day. One college accidentally killed  his project-files after 6 weeks of work. He forgot the right name of his backup-folder and removed the false on, the one containing the current source. He tried to save disk-space, even in those old day disk-space was much cheaper than manpower.

To avoid errors like this: use a SCM-tool.

Check-in all your files!

Now you have a working SCM-tool check if all source-files, scripts and Makefiles are checked-in. If not: start doing this. The target of this task is just to get a reproducible build for you. Work on this until you are able to build from scratch after checking out your product. And when this works write a small KickStarter-Doc how to build everything from scratch after a clean checkout. Of course this will not work in the beginning. Of course you will face a lot of issues like a broken build, wrong paths or a different environment. But this is also a sign of legacy-code: not reproducible builds. Normally not all related files like special Makefiles are checked in. Or sometimes the environment differs between the different developer-PCs. And this causes a lot of hard to reproduce issues.

Do you know the phrase: “It worked on my machine?” after facing a new bug. Sometimes the developer was right. The issue was caused by different environments between the developer machines ( for instance different compiler version, different IDE, different kernel, different whatever … ).

When you have checkin all you files try to ensure that everyone is using the same tools: same compiler version, same libs, same IDE and document this in your KickStarter-Doc. Let’s try other guy’s to work with this and fix all upcoming issues.

This can slow down the ongoing development tasks. To avoid this you can learn how to work with branches with your SCM-tool ( for instance this doc shows how to do branches in git: https://git-scm.com/book/en/v2/Git-Branching-Basic-Branching-and-Merging ).