Help with memory management question using C++11

[Paul]

Below, I will post a question that I would like some help with (if possible).
The best type of help would be something like "Think about using..." rather
than just giving the answer. I would like to avoid using post-C++11 features
if possible.
People sometimes get (rightly) homework-suspicious when they see posts
like these. This question is for self-study purposes only. I am not
a student, and am not enrolled in any course. It is also not part of
any external evaluation process or interview process.
The source of the original question is
https://www.testdome.com/questions/cpp/multiple-choice-test/9808?visibility=1&skillId=7

I have tried the very basic solution of using delete[] in the destructor
without using smart pointers. That is not the intended solution.
I tried using unique pointers with release(). This compiled but was also
not the intended solution. reset() wasn't what they wanted either.

Many thanks for your help,

Paul

// CODE BELOW
/*Multiple choice test has several multiple choice questions. Each question can have only one correct answer. Additionally, timed multiple choice test can specify the time allowed for solving each question in the test.

The code below satisfies this specification, but the customer complained that the memory usage of the program constantly increases. Fix this problem.*/

#include <iostream>
#include <string>

class MultipleChoiceTest
{
public:
MultipleChoiceTest(int questionsCount)
{
this->questionsCount = questionsCount;
answers = new int[questionsCount];
for (int i = 0; i < questionsCount; i++)
{
answers[i] = -1;
}
}

void setAnswer(int questionIndex, int answer)
{
answers[questionIndex] = answer;
}

int getAnswer(int questionIndex) const
{
return answers[questionIndex];
}

protected:
int questionsCount;

private:
int* answers;
};

class TimedMultipleChoiceTest : public MultipleChoiceTest
{
public:
TimedMultipleChoiceTest(int questionsCount)
: MultipleChoiceTest(questionsCount)
{
times = new int[questionsCount];
for (int i = 0; i < questionsCount; i++)
{
times[i] = 0;
}
}

void setTime(int questionIndex, int time)
{
times[questionIndex] = time;
}

int getTime(int questionIndex) const
{
return times[questionIndex];
}

private:
int* times;
};

#ifndef RunTests
void executeTest()
{
MultipleChoiceTest test(5);
for (int i = 0; i < 5; i++)
{
test.setAnswer(i, i);
}

for (int i = 0; i < 5; i++)
{
std::cout << "Question " << i + 1 << ", correct answer: " << test.getAnswer(i) << "\n";
}
}

int main()
{
for (int i = 0; i < 3; i++)
{
std::cout << "Test: " << i + 1 << "\n";
executeTest();
}
}
#endif

[Öö Tiib]

On Saturday, 27 October 2018 12:19:19 UTC+3, Paul wrote:
> Below, I will post a question that I would like some help with (if possible).
> The best type of help would be something like "Think about using..." rather
> than just giving the answer. I would like to avoid using post-C++11 features
> if possible.
> People sometimes get (rightly) homework-suspicious when they see posts
> like these. This question is for self-study purposes only. I am not
> a student, and am not enrolled in any course. It is also not part of
> any external evaluation process or interview process.
> The source of the original question is
> https://www.testdome.com/questions/cpp/multiple-choice-test/9808?visibility=1&skillId=7
>
> I have tried the very basic solution of using delete[] in the destructor
> without using smart pointers. That is not the intended solution.
> I tried using unique pointers with release(). This compiled but was also
> not the intended solution. reset() wasn't what they wanted either.
>
> Many thanks for your help,

Seems that it just has all the typical problems: constructors are
not exception-safe, resources are acquired but destructors don't
release those, base class destructor is neither virtual nor protected,
also rule of five is violated. Basics. Why C++ was designed to allow
that silently ... I don't know, but so it is.

[Öö Tiib]

Oh I forgot to give "Think about using...containers of standard library"
advice. ;)

[Sam]

Paul writes:

> Below, I will post a question that I would like some help with (if possible).

Unfortunately, a very careful and detailed inspection of what you posted
found no evidence, whatsoever, of an actual, specific question. The most
fundamental aspect of a basic question, namely the presence of a question
mark, was nowhere to be found. If you were to review what was written, with
a fresh mind and no initial knowledge of anything, you will be forced to
admit this to be so.

It's true that it's possible to avoid explicitly asking a question by
describing specifically the nature of the problem, the expected behavior you
expect from your code, and the actual behavior you're seeing, thus implying
a basic WTF type of question. But those necessary ingredients, for this
approach, were also not there.

You started off with some general description, or a dissertation of some
kind. That was followed by a code dump. And that was it. That's all I read.
Maybe the gods of Usenet ate a part of the post. There was something
mentioned about using delete in a destructor, but what exactly was the
problem with that, that was sadly left unmentioned:

> I have tried the very basic solution of using delete[] in the destructor
> without using smart pointers.

But a detailed description or an explanation of the observed or the alleged
problem for which this is claimed to be an attempted solution, seems to be
curiously absent.

It's true that the shown code lacked some aspects of correctly implementing
dynamic memory allocation in classes – such as a lack of a proper
constructor and assignment operator in order to correctly manage the dynamic
memory allocation that the class uses – which could be the actual problem
you were trying to communicate about, but without knowing that for a fact,
that this is the issue at hand, writing a detailed explanation of what these
problems are would probably be a wasted effort, I'm afraid.

[Chris Vine]

On Sat, 27 Oct 2018 02:19:07 -0700 (PDT)
Paul <peps...@gmail.com> wrote:
> Below, I will post a question that I would like some help with (if possible).
> The best type of help would be something like "Think about using..." rather
> than just giving the answer. I would like to avoid using post-C++11 features
> if possible.
> People sometimes get (rightly) homework-suspicious when they see posts
> like these. This question is for self-study purposes only. I am not
> a student, and am not enrolled in any course. It is also not part of
> any external evaluation process or interview process.
> The source of the original question is
> https://www.testdome.com/questions/cpp/multiple-choice-test/9808?visibility=1&skillId=7
>
> I have tried the very basic solution of using delete[] in the destructor
> without using smart pointers. That is not the intended solution.
> I tried using unique pointers with release(). This compiled but was also
> not the intended solution. reset() wasn't what they wanted either.

What do you mean "is not the intended solution". Adding destructors
which call delete[] does satisfy the online compiler/test runner,
although it is not a very elegant solution: possibly you didn't make the
destructors virtual, because one of the not-fully-revealed tests is to
use a TimedMultipleChoiceTest object as a MultipleChoiceTest object,
presumably by constructing it on free store and accessing it and
then deleting it through a MultipleChoiceTest pointer.

If you used unique_ptr, (i) why are you calling its release() method
(that seems completely wrong), and (ii) are you using the
unique_ptr<int[]> form?

However, as another poster has said "consider the standard
containers". One container in particular is intended for this kind of
usage.

[Jorgen Grahn]

On Sat, 2018-10-27, Öö Tiib wrote:
> On Saturday, 27 October 2018 12:19:19 UTC+3, Paul wrote:

>> Below, I will post a question that I would like some help with (if
>> possible).

>> The best type of help would be something like "Think about
>> using..." rather than just giving the answer. I would like to
>> avoid using post-C++11 features if possible.

...

>> The source of the original question is
>> https://www.testdome.com/questions/cpp/multiple-choice-test/9808?visibility=1&skillId=7
>>
>> I have tried the very basic solution of using delete[] in the destructor
>> without using smart pointers. That is not the intended solution.

There is an *intended* solution?

>> I tried using unique pointers with release(). This compiled but was also
>> not the intended solution. reset() wasn't what they wanted either.
>>
>> Many thanks for your help,
>
> Seems that it just has all the typical problems: constructors are
> not exception-safe, resources are acquired but destructors don't
> release those, base class destructor is neither virtual nor protected,
> also rule of five is violated. Basics. Why C++ was designed to allow
> that silently ... I don't know, but so it is.

Perhaps that's intentional, but the code you're supposed to fix is
wrong on so many levels. (Which is basically what Tiib wrote, I see
now.)

On the other hand, fixing bugs in terrible code without rewriting it
from scratch is a useful skill to have.

/Jorgen

--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .

[Alf P. Steinbach]

On 27.10.2018 11:19, Paul wrote:
> Below, I will post a question that I would like some help with (if possible).
> The best type of help would be something like "Think about using..." rather
> than just giving the answer. I would like to avoid using post-C++11 features
> if possible.
> People sometimes get (rightly) homework-suspicious when they see posts
> like these. This question is for self-study purposes only. I am not
> a student, and am not enrolled in any course. It is also not part of
> any external evaluation process or interview process.
> The source of the original question is
> https://www.testdome.com/questions/cpp/multiple-choice-test/9808?visibility=1&skillId=7
>
> I have tried the very basic solution of using delete[] in the destructor
> without using smart pointers. That is not the intended solution.
> I tried using unique pointers with release(). This compiled but was also
> not the intended solution. reset() wasn't what they wanted either.

Not sure exactly what you're asking. Presumably that's explained at the
site you link to, maybe? Not looking there.

> Many thanks for your help,
>
> Paul
>
> // CODE BELOW
> /*Multiple choice test has several multiple choice questions. Each question can have only one correct answer. Additionally, timed multiple choice test can specify the time allowed for solving each question in the test.
>
> The code below satisfies this specification, but the customer complained that the memory usage of the program constantly increases. Fix this problem.*/
>
> #include <iostream>
> #include <string>
>
> class MultipleChoiceTest
> {

I like to place the member variables here, up front.

That way things become more concrete and easy to grasp.

> public:
> MultipleChoiceTest(int questionsCount)
> {
> this->questionsCount = questionsCount;
> answers = new int[questionsCount];
> for (int i = 0; i < questionsCount; i++)
> {
> answers[i] = -1;
> }
> }

Should use member initializers like so:

Multiple_choice_test( const int n_questions ):
m_answers( n_questions, -1 )
{}

... where `m_n_questions` is declared as a data member of type
`vector<int>`.

With use of raw arrays, like a student exercise with “don't use
`std::vector`!” requirement, it could look like this:

Multiple_choice_test( const int n_questions ):
m_n_questions( n_questions ),
m_answers( new int[n_questions] )
{
fill_n( m_answers, n_questions, -1 );
}

... where `fill_n` is `std::fill_n` from `<algorithm>`.

> void setAnswer(int questionIndex, int answer)
> {
> answers[questionIndex] = answer;
> }

Naming: consider using names that indicate the roles of the arguments in
a call, so as to make the /calls/ readable and clear and bug-free.

E.g. `set_answer_at` indicates that the first argument is an index.

Or you can go further and define appropriately named argument types like

struct Answer( int value; };

... then support /named arguments/ in the calls,

void set_at( int i, const Answer answer )

... which gives calls like

mct.set_at( 3, Answer{ 42 } );

Even better, as an alternative, make the raw array a raw array of `int`,
and just provide named collection indexing support,

auto answers( const int i )
-> Answer&
{ return m_answers[i]; }

... which gives calls like

mct.answers( 3 ) = 42;

... still clear as to the roles of `3` and `42`, but more concise.

I wouldn't go as far as to decouple the implementation from the
interface by letting `answers()` (or e.g. an `operator[]`) return a
proxy, but some fairly smart people have argued in that direction at
times. I think a class would have to be both much used and with the
clear possibility of a radical implementation change, for that amount of
decoupling overhead to make sense. But that's my view.

> int getAnswer(int questionIndex) const
> {
> return answers[questionIndex];
> }

Naming: in C++ prefix `get` for a simple getter or value-producing
function is usually just negative value verbosity. In e.g. Java it has a
value for use with tools that use introspection of the code; they
identify getter+setter pairs. C++ doesn't yet support introspection.

Consider: get_sin(x) + 2get_cos(x)

With the last option exemplified above, this getter would be

auto answers( const int i ) const
-> int
{ return m_answers[i]; }


> protected:
> int questionsCount;

Oh that's bad.

`protected`, granting direct access to derived classes, is seldom a good
choice. It's in the language for a reason, but. Instead of that access
level, an accessor for the value should be defined, like

auto n_questions() const
-> int
{ return m_questions; }


> private:
> int* answers;
> };

I'd put that at the start of the class, with implicit `private`
accessibility.

Notably lacking in this class: taking charge of cleanup and logical copying.

Cleanup is a destructor, logical copying is a copy constructor and
possibly a move constructor to support also moving.

Check out the original C++03 “rule of three”, the later C++11 “rule of
five” and the Good Robot's “rule of zero”.

Essentially, consider first using collection classes; if that doesn't
cut it use smart pointers (rule of zero, use types that already take
care of memory management for you); and if you absolutely must do things
manually, then define or declare all five of copy constructor, move
constructor, copy assignment, move assignment and destructor.

At the design level there is also a serious problem: that this class
directly models a general dynamic size array but is needlessly
restricted, via its naming, to use for multiple choice tests.

It would be better for general code reuse to define the dynamic length
array as a class template, e.g. called `Array_`, and then use that as a
data member in the implementation of `Multiple_choice_test`.

> class TimedMultipleChoiceTest : public MultipleChoiceTest
> {
> public:
> TimedMultipleChoiceTest(int questionsCount)
> : MultipleChoiceTest(questionsCount)
> {
> times = new int[questionsCount];
> for (int i = 0; i < questionsCount; i++)
> {
> times[i] = 0;
> }
> }

Even in C++03 one had value initialization. Zeroing by using a loop is
silly.

> void setTime(int questionIndex, int time)
> {
> times[questionIndex] = time;
> }
>
> int getTime(int questionIndex) const
> {
> return times[questionIndex];
> }
>
> private:
> int* times;
> };

The same problems as already mentioned for the first class.

Additional problem: with this derived class it's possible to set an
answer without setting the time for it. In particular it's possible to
update an answer without updating the associated time. So it models a
test where times /may/ be available for some or all answers, and where
it's entirely unclear what an associated time, if there is one, refers
to: original first answer, last update, or something else.

If the intent was that every answer should have a time, i.e. that this
class models an array of struct{ int answer; int time; }, then the
derived class relationship is wrong. It's an unnatural thing probably
stemming from a wish to reuse. The mentioned generalization of the first
class to an `Array_` class template would support more meaningful reuse.

Indeed, with the standard library an instance of the above is better
replaced with

struct Choice{ int value; };
struct Time{ int value; };
struct Answer{ Choice choice; Time time; };

const int n = ...;
vector<Answer> mct( n, Answer{ Choice{-1}, Time{0} } );

[snip]


Cheers & hth.,

- Alf

[Juha Nieminen]

Paul <peps...@gmail.com> wrote:
> I have tried the very basic solution of using delete[] in the destructor
> without using smart pointers. That is not the intended solution.

Adding a destructor that has a delete[] is not enough. Google
"C++ rule of three".

You can bypass the requirements of the "rule of three" if you
simply use std::vector or a similar container (which does implement
the rule of three appropriately).

Although it's useful to know that rule of thumb anyway, if you
ever need to do something for which the standard data containers
aren't enough.

[Vir Campestris]

On 28/10/2018 09:31, Juha Nieminen wrote:
> Adding a destructor that has a delete[] is not enough. Google
> "C++ rule of three".

I'm sure Öö will say "and then rule of five" - see upthread.

Andy

[bitrex]

the problem is as far as I can tell the task the example code is trying
to accomplish doesn't actually require heap allocation but they're
shoe-horning in some heap allocations anyway and then intentionally
screwing it up so the student "learns"....what, exactly.

The example is ill-motivated.

[Juha Nieminen]

Wasn't it so that if you follow the "rule of three" even in C++11, nothing
breaks. You can follow the "rule of five" if you want more efficiency
(essentially, copying temporaries around becomes much more efficient).

[Öö Tiib]

Yes. Compiler won't implicitly define move constructors and assignments
when one of other four is user-declared. So when we want to use move
then we either have to declare those or to declare none of the five.

Also implicitly declared copy constructors and copy assignments when
one of other two is user-defined is deprecated. However compilers
do not issue any diagnostics about that deprecation so it has no effects
in practice.