New to C++ - right approach so far?

Question

0.00/5 (No votes)

See more:

Hi,

As part of a personal project with Unreal Engine 4, I'm learning c++. After much deliberation I think I'm finally starting to understand some new concepts which you simply don't see with managed languages. My goals here are as follows;

1. To manage all memory properly to avoid any memory leaks
2. To have all code as compact as possible while not being ugly
3. To abide by c++ standards as much as possible

If you could point out any changes you'd make to this code I've written, please let me know. Once I know how I should be writing the code properly, I can confidently apply that to the rest of my project as I write it.

This class is called ContainerAttributes, and is effectively a dictionary of names and values, for example capacity is 100, health is 75 etc. As different objects will have different storage information, eg. some items may be invincible and may have no health attributes, I'm not hardcoding anything in, rather I'm allowing the programmer to specify the attributes and values dynamically.

.h:

C++

#pragma once

#include "DebugHelper.h"

#include "CoreMinimal.h"

class NOCTURNALSYNERGY_API ContainerAttributes
{
public:
    ContainerAttributes();
    ~ContainerAttributes();
	void SetValue(string name, int value);
    int GetValue(string name);

    void Debug();

private:
    map<string, int> values;

};

As I mentioned, I have no experience with memory management but I understand that anything created with 'new' returns a pointer to the object which must be deleted to free memory using 'delete'. To the best of my knowledge as well, ~ methods are deconstructors, which would come about in the following situation;

C++

obj->containerAttributes = new ContainerAttributes();
obj->whatever();
delete obj;

(then inside obj)
obj::~obj()
{
delete containerAttributes;
}

This should consequently call the deconstructor in ContainerAttributes which can free its memory (the values map).

.cpp:

C++

#include "ContainerAttributes.h"

ContainerAttributes::ContainerAttributes()
{
    values = new map<string, int> {};
}

ContainerAttributes::~ContainerAttributes()
{
    delete values;
}

void ContainerAttributes::SetValue(string name, int value)
{
    auto it = values.find(name);

    if(it != values.end())
    {
        values[name] = value;
    }
    else
    {
        values.insert(pair<string, int>(name, value));
    }
}

int ContainerAttributes::GetValue(string name)
{
    auto it = values.find(name);

    if(it != values.end())
    {
        return it->second;
    }
    else
    {
        DebugHelper::Debug("Error in ContainerAttributes; map does not contain name '" + name + "'");
    }
}

void ContainerAttributes::Debug()
{
    map<string, int>::iterator it;

    for(it = values.begin(); it != values.end(); it++)
    {
        DebugHelper::Debug(it->first + ": " + it->second);
    }
}

A few other questions regarding this, DebugHelper::Debug accepts an FString. Is it even possible to concatenate strings in this way using '+' or do I have to use another method? I'm happy to create an additional method to accept std::string as well, but I'm not sure whether '+' will work there either.

Finally, is it best to use

auto it = values.find(name);

or should I explicitly specify something like this?

pair<string, int> it = values.find(name);

Thanks for any help you can give.

What I have tried:

--------------------------------------------------------------------------

Posted 22-May-18 1:41am

User 13204940

Updated 22-May-18 2:43am

v6

Add a Solution

1 solution

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Jochen Arndt · Answer 1 · 2018-05-22T02:41:00

Solution 1

If you avoid using allocated member variables, the destructor of members is called automatically reducing the chance of having memory leaks. With your obj example it looks like you can do that by changing the member from *ContainerAttributes to ContainerAttributes.

You might also think about making your ContainerAttributes class std::map based:

class NOCTURNALSYNERGY_API ContainerAttributes : public std::map<std::string, int>
{
public:
    ContainerAttributes();
    ~ContainerAttributes();
    void SetValue(const std::string& name, int value)
    {
        auto it = find(name);
        if (it != end())
            it->second = value;
        else
            insert(std::pair<std::string, int>(name, value));
    }
    int GetValue(const std::string& name) const
    {
        const auto it = find(name);
        return it == end() ? -1 : it->second;
    }
};

However, this will make access to the map public. Note also that I have changed the name parameters to be const and passed by reference which is the appropriate passing here.

According to the FString | Unreal Engine[^] documentation it provides a += operator (which probably is just calling Append() internally). So you can use the + operator. But in your case

DebugHelper::Debug(it->first + ": " + it->second);

the operation is performed on (and as) std::string and not on FString.

When to use auto and when not is your decision and commonly choosen depending on the context. Just search the web for something like "c++ auto or not". A probably good read: Don't use <auto> unless you mean it — Joseph Mansfield[^].

Posted 22-May-18 2:41am

Jochen Arndt

Comments

[no name] 22-May-18 8:47am

Very useful, thank you. Just a few things, what do you mean by changing the member from *ContainerAttributes to ContainerAttributes?

Also, is the idea of 'const auto it' mainly to display intent, which is possible because it->second isn't being modified, only read?

Jochen Arndt 22-May-18 9:09am

The member of your obj class is actually a pointer (using new). But if you have only a single instance (not an array), there is no need to use a pointer / allocated instance. Just use a single instance as member:

class obj
{
// ...
// was:
//ContainerAttributes* containerAttributes;
// better:
ContainerAttributes containerAttributes;
};

Then the constructor is called from within the obj constructor. And similar for the destructor. No need to call
delete containerAttributes;
in the obj destructor. The ContainerAttributes destructor is called automatically deleting the std::map.

const instructs the compiler to not allow changing the object. It should be always used when an object is not changed.

[EDIT]
In this special case it is not really necessary to use const for the iterator because I have made the function GetValue() const. As a result, the compiler will automatically choose the find() overload that returns a const iterator when using auto. So it is a good example for an advantage of using auto (within const member functions).
[/EDIT]

CPallini 22-May-18 9:11am

What about

void SetValue(const std::string& name, int value)
{
this->operator[](name) = value;
}

?

Jochen Arndt 22-May-18 9:17am

My virtual 5!

[no name] 22-May-18 10:01am

CPallini, why do you use const in that situation for the std::string but not for the int?

CPallini 22-May-18 16:03pm

Const doesn't make sense for a parameter passed by value.

[no name] 23-May-18 4:20am

Ok, so what I understand from your example is that you're passing a std::string by reference which must not be changed. Why wouldn't you just pass it by value?

CPallini 23-May-18 4:39am

Because it would be more expensive (a fresh copy of the passed string would be made).

Jochen Arndt 23-May-18 4:48am

If you pass it by value, a copy has to be created (which includes calling a construtor and the destructor which call new() and delete with dynamic objects like std::string) while passing a reference (or pointer) just passes an address (an integer). So using a reference is faster, produces less code, and consumes less memory.

Because a function might change the content of a variable passed by reference or pointer, the const avoids that (compilation error) and informs also the developer using that function that the passed variable is not changed.

General rule:
Pass fundamental types like bool, char, wchar_t, int, long, float, and double by value and all other by reference (preferred) or pointer. Use const with references and pointers when the value is not changed.

[no name] 23-May-18 5:05am

Ok great that makes sense, I've modified my code now. Do all of the parameters look correct?

the

char*&

is intended to mean a char array passed by reference, used such as:

DebugHelper::Debug("this is a char*");

// Fill out your copyright notice in the Description page of Project Settings.

#pragma once

#include "ConversionHelper.h"
#include <string>
#include "Runtime/Engine/Classes/Engine/Engine.h"

#include "CoreMinimal.h"

class NOCTURNALSYNERGY_API DebugHelper
{
public:
	static void Debug(const FString& f);
	static void Debug(const std::string& s);
	static void Debug(const char*& c);
	static void Debug(int i);

private:
	static void _Debug(const FString& f);

};

// Fill out your copyright notice in the Description page of Project Settings.

#include "DebugHelper.h"

void DebugHelper::Debug(const FString& f)
{
	DebugHelper::_Debug(f);
}

void DebugHelper::Debug(const std::string& s)
{
	DebugHelper::_Debug(ConversionHelper::StringToFString(s));
}

void DebugHelper::Debug(const char*& c)
{
	DebugHelper::_Debug(ConversionHelper::StringToFString(c));
}

void DebugHelper::Debug(int i)
{
	DebugHelper::_Debug(ConversionHelper::IntToFString(i));
}

void DebugHelper::_Debug(const FString& f)
{
	GEngine->AddOnScreenDebugMessage(-1, 10.0f, FColor::Red, f);
	UE_LOG(LogTemp, Log, TEXT("DEBUG | %s"), *f);
}

Jochen Arndt 23-May-18 5:20am

It usually makes no sense to pass a reference to a pointer. In such cases just pass the pointer (because it as an address which is an integer which is a fundamental type):
void DebugHelper::Debug(const char* s)

[no name] 23-May-18 5:58am

I was trying to declare an array of char, as

const char[] s

caused a syntax error

Jochen Arndt 23-May-18 6:20am

That is not a valid C/C++ code line.
For function parameters char* and char[] can be used (they are equivalent).

[no name] 23-May-18 7:50am

Ok, this is one of those weird things which is a c++ learning curve. Correct me if I'm wrong, but what I understand is that char* is both a pointer to a character, and a char 'array' but it actually points to the first char in the array.

Otherwise I don't see the difference between a char pointer and char array

Edit: I've just seen an example and the [] is after the parameter name, not the type, so

const type[] whatever

is wrong whereas

const type whatever[]

is correct.

Also I wouldn't be passing by reference here because char is a native type, would I?

Jochen Arndt 23-May-18 8:52am

The type does not matter. An array is always passed as pointer behind the scenes.
See https://www.cs.bu.edu/teaching/cpp/string/array-vs-ptr/

[no name] 23-May-18 9:24am

Ok, so going back to what was said about passing by reference to avoid a copy if the type is not a native type, should I be using

const char* charArray;

or

const char*& charArray

?

I assume the latter is the exact same as

const char& charArray[]

Jochen Arndt 23-May-18 9:36am

Just use char* (or array but that is not so common).

As already said:
Pointers are fundamental types and don't need to be passed as reference and arrays passed as function parameters are passed as pointers.

While passing a reference to a pointer is possible it makes reading code harder and is not common.

It is also rather uncommon to use the array syntax for function parameters (more with C++ than with C) but it is sometimes used when having a multi-dimensional array.

[no name] 22-May-18 10:03am

User-2223753, surely if the constructor for containerAttributes is called automatically, there's no control over when it's constructed. With other languages, ContainerAttributes containerAttributes would simply create the object but not construct it; how can you control that here?

Jochen Arndt 22-May-18 10:51am

There is control: It is created when an instance of the object having it as member is created.

In your case:
When an obj instance is created, the constructor of that class calls the constructors of all members (if present). That requires that those members have no constructor or at least one without parameters.

[no name] 22-May-18 11:36am

I mean if you construct obj, you'd have no control over separately constructing the member at any time, as it'd happen immediately. This is why I'd rather use

ContainerAttributes* containerAttributes;

and whenever I like,

containerAttributes = new ContainerAttributes();

Jochen Arndt 22-May-18 12:11pm

There is usually no need to construct it later when having a single instance (not an array of variable size) or the object is quite large.

One of your goals was to avoid memory leaks. And not using new() here is doing that. With pointers you have also to ensure that they are initialised with nullptr (in the constructor) and all accesses should be guarded checking for a valid pointer (at least for debug builds). Not using new() results also in less code (in the source as well as generated) and faster execution.

So don't use new() when there is no need to use it.