# Let's check the qdEngine game engine, part two: simplifying C\+\+ code

In this article, we'll discuss how the PVS\-Studio static analyzer encourages developers to refactor their code\. After all, the shorter, simpler, and clearer the code is, the fewer errors it contains\.

![1121_qdEngine_check_2/image1.png](https://import.viva64.com/docx/blog/1121_qdEngine_check_2/image1.png)

In the previous article — "[Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS\-Studio](https://pvs-studio.com/en/blog/posts/cpp/1119/)" — we've already looked at an interesting example of code refactoring that was encouraged by the PVS\-Studio warning message \(see the tenth code fragment\)\. Let's look at some more examples of how we can enhance the code using a static analyzer\.

## Overcomplicated 'restore' function

```cpp
class qdCamera 
{
  ....
  sGridCell *Grid;
  ....
};

bool qdCamera::restore(sGridCell *grid,
                       int sx, int sy,
                       int csx, int csy)
{
  if (Grid)
    delete[] Grid;
  Grid  = new sGridCell[sx * sy];
  if (!Grid)
    return false;
  memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
  
  GSX = sx;
  GSY = sy;
  cellSX = csx;
  cellSY = csy;
  
  return true;
}
```

Here we have a function that reallocates a buffer of the required size\. The analyzer spots this code because it detects a meaningless check of the pointer returned by the operator _new\[\]_\. I'm talking about the following lines:

```cpp
Grid  = new sGridCell[sx * sy];
if (!Grid)
  return false;
```

The PVS\-Studio warning: [V668](https://pvs-studio.com/en/docs/warnings/v668/) There is no sense in testing the 'Grid' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. qd\_camera\.cpp 910

Indeed, if there isn't enough memory to create an array, the operator _new\[\]_ throws an exception of the [_std::bad\_alloc_](https://en.cppreference.com/w/cpp/memory/new/bad_alloc) type\. You can get a null pointer only using [_new \(std::nothrow\) \[\]_](https://en.cppreference.com/w/cpp/memory/new/nothrow), but this isn't our case\.

Here's a function where an unnecessary check has been removed:

```cpp
bool qdCamera::restore(sGridCell *grid,
                       int sx, int sy,
                       int csx, int csy)
{
  if (Grid)
    delete[] Grid;
  Grid = new sGridCell[sx * sy];
  memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
  
  GSX = sx;
  GSY = sy;
  cellSX = csx;
  cellSY = csy;
  
  return true;
}
```

Now the _restore_ function always returns _true_\. It either throws an exception or works properly\. Returning a success status \(_true_ or _false_\) no longer makes sense\. We can simplify the function by removing the return value:

```cpp
void qdCamera::restore(sGridCell *grid,
                       int sx, int sy,
                       int csx, int csy)
{
  if (Grid)
    delete[] Grid;
  Grid  = new sGridCell[sx * sy];
  memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
  
  GSX = sx;
  GSY = sy;
  cellSX = csx;
  cellSY = csy;
}
```

As a bonus, we no longer need to check what the function returned in other code fragments\.

What else can we enhance? There's no point in checking the pointer before calling the operator _delete\[\]_\. The operator handles null pointers correctly \(in this case, it does nothing\)\.

Someone may argue that such a check is a micro\-optimization, because if the pointer is null, you don't need to call the operator\. That's nonsense\. The function call time can be neglected when there are heavy operations such as memory allocation and deallocation\. I've covered the topic in the following article: "[Shall we check pointer for NULL before calling free function?](https://pvs-studio.com/en/blog/posts/cpp/1100/)" As you can see from the title, the article is about the _free_ function, but it's the same with the operator _delete_\.

The code without unnecessary checks:

```cpp
void qdCamera::restore(sGridCell* grid,
                       int sx, int sy,
                       int csx, int csy)
{
  delete[] Grid;
  Grid = new sGridCell[sx*sy];
  memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
  
  GSX = sx;
  GSY = sy;
  cellSX = csx;
  cellSY = csy;
}
```

We've significantly shortened the code without losing anything\. Unfortunately, both the old and the new code still have a weakness:

```cpp
delete[] Grid;
Grid = new sGridCell[sx * sy];
....
qdCamera::~qdCamera()
{
  if (GSX)
  {
    delete[] Grid;
  }
}
```

If an exception occurs when the operator _new_ is called, the stack unwinding and destruction of various objects begin\. The destructor of the _qdCamera_ object is also called\. And the _Grid_ pointer may not be null, but it's no longer valid\. The array is destroyed just before the operator _new_ is called\. This results in undefined behavior\.

The simplest fix is to add pointer zeroing:

```cpp
delete[] Grid;
Grid = nullptr;
Grid = new sGridCell[sx * sy];
```

That's a bad fix, though\. We're wasting our energy on a lost cause\. The main issue in the code is manual memory management\. It causes programmers to write long, buggy code\.

The correct solution is to use a smart pointer or _std::vector_\. Let's start with the [_std::unique\_ptr_](https://en.cppreference.com/w/cpp/memory/unique_ptr) smart pointer, as [_std::vector_](https://en.cppreference.com/w/cpp/container/vector) isn't always practical and efficient\.

```cpp
class qdCamera
{
  ....
  std::unique_ptr<sGridCell[]> Grid;
  ~qdCamera() = default;
  ....
};

void qdCamera::restore(sGridCell *grid,
                       int sx, int sy,
                       int csx, int csy)
{
  Grid = std::make_unique<sGridCell[]>(sx * sy);
  std::copy(grid, grid + sx * sy, Grid.get());
  
  GSX = sx;
  GSY = sy;
  cellSX = csx;
  cellSY = csy;
}
```

This is much better:

1. we've shortened the function code;
1. we've got rid of low\-level _new_/_delete_ calls thanks to a smart pointer and _std::make\_unique_;
1. we've secured the code;
1. now we don't need a destructor at all\.

Finally, here's an example of using _std::vector_\.

```cpp
class qdCamera
{
  ....
  std::vector<sGridCell> Grid;
  ~qdCamera() = default;
  ....
};

void qdCamera::restore(sGridCell *grid,
                       int sx, int sy,
                       int csx, int csy)
{
  Grid.clear();
  Grid.reserve(sx * sy);
  std::copy(grid, grid + sx * sy, std::back_inserter(Grid));
  
  GSX = sx;
  GSY = sy;
  cellSX = csx;
  cellSY = csy;
}
```

However, if you switch to containers during refactoring, the interface of many functions changes too\. This is what the _restore_ function is likely to look like:

```cpp
void qdCamera::restore(const std::vector<sGridCell> &grid,
                       int sx, int sy,
                       int csx, int csy)
{
  Grid = grid;
  GSX = sx;
  GSY = sy;
  cellSX = csx;
  cellSY = csy;
}
```

Although, I'm getting way off the point of the story\. Obviously, during extensive refactoring, the code may become completely different\. The _restore_ function, for example, may disappear or transform into something else\.

The main idea is that the PVS\-Studio analyzer warnings help you write shorter and safer code\.

## C\-style memory management in arButton class

In the _arButton_ class, manual memory management is even more apparent than in the previous case\. The class creates copies of strings and can return pointers to them\.

There are string copy functions:

```cpp
class arButton
{
  ....
  void set_obj(const char* p)
    { if(obj_name) free(obj_name);
      obj_name = strdup(p); }
  void set_obj_regvalue(const char* p)
    { if(obj_name_regvalue) free(obj_name_regvalue);
      obj_name_regvalue = strdup(p); }
  void set_cmdline(const char* p)
    { if(cmd_line) free(cmd_line); cmd_line = strdup(p); }
  void set_regkey(const char* p)
    { if(reg_key) free(reg_key); reg_key = strdup(p); }
  void set_reg_exec_path(const char* p)
    { if(reg_exec_path_value) free(reg_exec_path_value);
      reg_exec_path_value = strdup(p); }
  void set_checkstr(const char* p)
    { if(check_after_exec) free(check_after_exec); 
      check_after_exec = strdup(p); }
  ....
  char *obj_name;
  char *obj_name_regvalue;
  char *cmd_line;
  char *reg_key;
  char *reg_exec_path_value;
  char *check_after_exec;
};
```

Each string has a its own getter, but we don't care about that for now\. Here's just one example:

```cpp
char* get_obj(void) { return obj_name; }
```

The analyzer issues six similar warnings about potential memory leaks for this class\. Here's just one such warning from PVS\-Studio, so as not to make the article too long: [V773](https://pvs-studio.com/en/docs/warnings/v773/) The 'obj\_name' pointer was not released in destructor\. A memory leak is possible\. ar\_button\.cpp 50

The thing is, the class destructor doesn't do anything\.

```cpp
arButton::~arButton(void)
{
}
```

Strings created using [_strdup_](https://en.cppreference.com/w/c/experimental/dynamic/strdup) aren't destroyed in _set\_\*_ functions\. Memory leaks are possible\.

We could fix the code by refining the destructor, but would we really want to take that intermediate step? The point is that we not only want to fix the code, we'd like to enhance it right away\. Let's look at one of the _set_ functions again:

```cpp
void set_obj(const char* p)
  { if(obj_name) free(obj_name);
    obj_name = strdup(p); }
```

Here are the weaknesses in the code:

1. The check before calling _free_ is [unnecessary](https://pvs-studio.com/en/blog/posts/cpp/1100/)\. The _free_ function does nothing if _NULL_ is passed to it\.
1. There's no check for the pointer returned by the _strdup_ function\. So, the check is moved to the external code\. And we can't determine why the _get\_\*_ function returns _NULL_\. There are two options\. The first one is that the _set\_\*_ function hasn't been called\. The second option is that a memory allocation error has occurred, and the application has been disrupted\.
1. As we have already seen above, such code with manual memory management causes errors\.

We can rewrite the code using smart pointers\. However, this is pointless, as the best way is to use _std::string_\. After all, the program is written in C\+\+, not in C\.

```cpp
class arButton
{
  ....
  void set_obj(const char* p)           { obj_name = p; }
  void set_obj_regvalue(const char* p)  { obj_name_regvalue = p; }
  void set_cmdline(const char* p)       { cmd_line = p; }
  void set_regkey(const char* p)        { reg_key = p; }
  void set_reg_exec_path(const char* p) { reg_exec_path_value = p; }
  void set_checkstr(const char* p)      { _after_exec = p; }
  ....
  std::string obj_name;
  std::string obj_name_regvalue;
  std::string cmd_line;
  std::string reg_key;
  std::string reg_exec_path_value;
  std::string check_after_exec;
};
```

Depending on the refactoring extent, the getter functions can look like this:

```cpp
const char* get_obj(void) const { return obj_name.c_str(); }
```

Or like this:

```cpp
const std::string& get_obj(void) const { return obj_name; }
```

Let's get back to the _set_ functions\. They're not "good" yet, but without external context, it's hard to say what's the best way to refine them\. The functions have the following flaws so far:

1. It's not clear what's the best way to handle cases where _nullptr_ is passed to the function\. Is that an "empty string" or "oh no, it's time to throw an exception"?
1. Copying strings is inefficient\. Before allocating a buffer for a string, it's necessary to calculate its length first\.

Here's an abstract option for a possible enhancement:

```cpp
....
void set_cmdline(std::string s) noexcept { cmd_line = std::move(s); }
void set_regkey(std::string s)  noexcept { reg_key = std::move(s); }
....
```

Let's leave it at that, otherwise the question will follow: do we really need a layer of _get_/_set_ functions? The code may need even more extensive refactoring, so we may end up with a completely different class structure\.

## Handy stuff for experienced programmers

Some readers may be a bit puzzled and thinking:


> "What was that all about? We already know that manual memory management is bad, and it causes errors\\\. Why would you mention the analyzer? We don't need it to see that the shown code needs to be rewritten"\\\.

I agree, but please note the following points:

1. It was the analyzer that drew attention to the bad code\.
1. There's a reason not to do some abstract refactoring, but to fix bugs\. If you always lack time for refactoring, give it a go\! A good reason to justify the need to edit the code is to fix bugs/weak code fragments\. You can also make the surrounding code look nicer :\)

Thank you for reading\! Download [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) to enhance your code\.