2
\$\begingroup\$

I have made a function which takes a string which can be any of "triangle", "square" or "rectangle".

Depending on the string I return a pointer to an instance of class Form. My class Form has subclasses Triangle, Square and Rectangle.

My constraints were:

  • use C++98
  • no STL library (no map, vector etc)
  • no friend, and no using namespace std;
#include <iostream>
#define NB_FORMS 3

/**/

class Form
{
  public:
    Form() { }
    virtual ~Form() { }
};

class Square : public Form
{
  public:
    Square() { std::cout << "Squareee!" << std::endl; }
     static Form *createForm(void)
     {
       return (new Square());
     }
};

class Rectangle : public Form
{
  public:
    Rectangle() { std::cout << "Rectangleee!" << std::endl; }
    static Form *createForm(void)
    {
      return (new Rectangle());
    }
};

class Triangle : public Form
{
  public:
    Triangle() { std::cout << "Triangleee!" << std::endl; }
    static Form *createForm(void)
    {
      return (new Triangle());
    }
};

struct        s_makeForms
{
    std::string        formName;
    Form            *(*makeForm)();
};

/* */

Form *func(std::string name)
{
  struct s_makeForms allForms[3] =
  {
      {"triangle",  Triangle::createForm}, 
      {"rectangle", Rectangle::createForm},
      {"square", Square::createForm}
  };
  for (int i = 0; i < NB_FORMS; i++)
  {
    if (name == allForms[i].formName)
      return (allForms[i].makeForm());
  }
  return (NULL);
}

int main()
{
  Form *myform;

  if (!(myform = func("square")))
  {
    std::cerr << "wrong name" << std::endl;
  }
  delete myForm;
}
\$\endgroup\$
1
  • 5
    \$\begingroup\$ It would be better to explain here what problem you are trying to solve instead of making us follow the link to read the question and answer and then come back here. \$\endgroup\$ – Teepeemm May 7 at 13:38
10
\$\begingroup\$

Use all the power of the standard library

In your post on StackOverflow, you mentioned that you couldn't use std::map because you were not allowed to use it. Outside of the classroom, you should not limit yourself this way, and instead use the appropriate functions from the standard library that help you write concise and efficient code. The function func() could look like:

Form *func(std::string_view name)
{
    static const std::unordered_map<std::string_view, std::function<Form*()>> forms = {
        {"triangle",  []{return new Triangle;}},
        {"rectangle", []{return new Rectangle;}},
        ...
    };

    if (auto found = forms.find(name); found != forms.end()) {
        return found->second();
    } else {
        return nullptr;
    }
}

And instead of raw pointers, you should probably return std::unique_ptr<Form>. In the rest of the review I'm going to assume you are limited to what you can use.

Prefer using = default

It's usually best to let the compiler generate default constructors, destructors and other operators. In some cases, you have to explicitly tell it to add one of those, for example when you want to ensure the destructor of Form is virtual. In that case, use = default instead of {}:

class Form
{
public:
    virtual ~Form() = default;
};

Use \n instead of std::endl

Prefer using \n instead of std::endl, the latter is equivalent to the former but also forces a flush of the output, which might be bad for performance if you are printing a lot.

Avoid unnnecessary parentheses and void

You can simplify the createForm() functions a bit:

static Form *createForm()
{
    return new Square;
}

The unnecessary parentheses and void in the argument list are just unnecessary distractions here.

Move s_makeForms into func()

You can move the definition of struct s_makeForm into func(), so it doesn't clutter the global namespace. In fact, you don't even have to give this struct a name, you can write:

Form *func(std::string &name)
{
    struct {
        std::string name;
        Form *(*createForm)();
    } forms[] = {
        "triangle", Triangle::createForm},
        ...
    }

    ...
}

Prefer const std::string & when passing string arguments

To avoid unnecessary copying of strings, prefer passing strings as const references:

Form *func(const std::string &name)
{
    ...
}

Avoid having to #define NB_FORMS

You can get the size of the array allForms in various ways, the typical trick that works both in C and all versions of C++ is:

struct ... allForms[] = {...};
size_t numForms = sizeof allForms / sizeof *allForms;

for (size_t i = 0; i < numForms; ++i)
{
    ...
}

You can also use a range-based for-loop to iterate over the array:

struct ... allForms[] = {...};

for (auto &form: allForms)
{
    if (name == form.formName)
        return form.makeForm();
}

Memory leak

There is a memory leak in your program, since you forgot to call delete myform. These kinds of errors can be avoided by using smart pointers, such as std::unique_ptr.

\$\endgroup\$
7
  • 3
    \$\begingroup\$ Don't use the ancient sizeof trick to get an array size. That can malfunction in certain situations. Instead, use template argument deduction. \$\endgroup\$ – JDługosz May 6 at 22:24
  • 2
    \$\begingroup\$ @JDługosz Sure, we have std::size() in C++17 that does exactly what we want. But the sizeof trick should work just fine in this case. \$\endgroup\$ – G. Sliepen May 7 at 6:25
  • 1
    \$\begingroup\$ Ask your teacher to clarify: declaring the constructor using =default is defining it in the class. See if your teacher even knows about that ... is the textbook pre-2011? Is your instructor familiar with (or even know about) the Standard Guidelines as I showed you in the post of mine you read earlier? \$\endgroup\$ – JDługosz May 7 at 13:59
  • 2
    \$\begingroup\$ @G.Sliepen The problem with the sizeof trick is that it will compile just fine if the array has decayed into a pointer, but it will give you the wrong answer. std::size() (or a home-rollled version of it) will fail to compile in situations where they aren't correct. While it's fine here, it's good to teach beginners how to do things safely. \$\endgroup\$ – Miles Budnek May 7 at 23:10
  • 1
    \$\begingroup\$ @MilesBudnek: Or: std::array<3, s_makeForms>? \$\endgroup\$ – Mooing Duck May 7 at 23:48
7
\$\begingroup\$

Design review

I’m not going to bother with a code review, because I see @G-Sliepen has already done an excellent job of that. Instead, I’m going to focus on a high-level design review.

The name of the pattern you are using is the Factory pattern. The Factory pattern uses a single “make” function that takes an argument, and returns a different object type depending on the argument.

For example:

class Base
{
public:
    virtual ~Base() = default;
};

class A : public Base {};
class B : public Base {};
class C : public Base {};

enum class object_type
{
    type_a,
    type_b,
    type_c,
};

auto factory_function(object_type type) -> std::unique_ptr<Base>
{
    switch (type)
    {
    case object_type::type_a:
        return std::make_unique<A>();
    case object_type::type_b:
        return std::make_unique<B>();
    case object_type::type_c:
        return std::make_unique<C>();
    }
}

That’s basically exactly the problem you want to solve, except you don’t want to use if or switch, you want to use a map instead. That’s no problem; it doesn’t really change anything about the basic pattern.

So the first thing you need to do is make your map. You’ve chosen to use a custom struct and a C array. Okay, first, forget the C array; that’s not just not up to par in modern C++. C arrays are broken in a lot of ways.

So let’s start instead with your custom struct and a proper C++ array:

constexpr auto allForms = std::to_array<s_makeForms>({
    {"triangle",  Triangle::createForm},
    {"rectangle", Rectangle::createForm},
    {"square",    Square::createForm}
});

Now, the one major annoyance with this design as-is is that if you want to add a new shape, you have to write a boilerplate createForm() function for it. Every shape needs a createForm() function, even though it’s just brainless repetition. Unfortunately, it’s not just tedious, it’s also dangerous. Consider this:

class Ellipse : public Form
{
public:
    Ellipse() { std::cout << "Ellipseee!\n"; }

    static auto createForm(void) -> std::unique_ptr<Form>
    {
        return std::make_unique<Ellipse>();
    }
};

class Circle : public Ellipse
{
    Circle() { std::cout << "A circle is just a special case of ellipse.\n"; }

    // oops! forgot to make the createForm() function!
}

// ...

constexpr auto allForms = std::to_array<s_makeForms>({
    // ...
    {"ellipse",   Ellipse::createForm},
    {"circle",    Circle::createForm}
});

// ...

auto main() -> int
{
    std::cout << "making a circle: ";
    auto p_circle = func("circle");
}

// output:
//  making a circle: Ellipseee!

And that’s just one of many, many, many ways you could screw up while having to duplicate the createForm() machinery over and over again.

There is no reason that every class needs to have a createForm() function. (At least, none that apply to the current code.) The only place that function is needed is in that map, so there’s no need to bog down the creation of new form types with the boilerplate.

You could do this:

class Form
{
public:
    virtual ~Form() = default;
};

class Rectangle : public Form
{
public:
    // Rectangle needs only the functions necessary to be a rectangle
    Rectangle() { std::cout << "Rectangleee!\n"; }
};

class Square : public Form
{
public:
    // Square needs only the functions necessary to be a square
    Square() { std::cout << "Squareee!\n"; }
};

class Triangle : public Form
{
public:
    // ... and so on...
    Triangle() { std::cout << "Triangleee!\n"; }
};

class Ellipse : public Form
{
public:
    Ellipse() { std::cout << "Ellipseee!\n"; }
};

class Circle : public Form
{
public:
    Circle() { std::cout << "Circleee!\n"; }
};

// detail namespace to hide implementation details
namespace detail_ {

auto create_rectangle() -> std::unique_ptr<Form> { return std::make_unique<Rectangle>(); }
auto create_square()    -> std::unique_ptr<Form> { return std::make_unique<Square>(); }
auto create_triangle()  -> std::unique_ptr<Form> { return std::make_unique<Triangle>(); }
auto create_ellipse()   -> std::unique_ptr<Form> { return std::make_unique<Ellipse>(); }
auto create_circle()    -> std::unique_ptr<Form> { return std::make_unique<Circle>(); }

} // namespace detail_

auto func(std::string_view name) -> std::unique_ptr<Form>
{
    using create_func_t = auto (*)() -> std::unique_ptr<Form>;

    struct value_t
    {
        std::string_view name;
        create_func_t func;
    };

    static constexpr auto all_forms = std::to_array<value_t>({
        {"rectangle", detail_::create_rectangle},
        {"square",    detail_::create_square},
        {"triangle",  detail_::create_triangle},
        {"ellipse",   detail_::create_ellipse},
        {"circle",    detail_::create_circle},
    });

    if (auto p = std::ranges::find(all_forms, name, &value_t::name); p != all_forms.end())
        return (p->func)();
    else
        return nullptr;
}

But that’s still too much repetition. So rather than using free functions (albeit in a detail namespace), let’s use lambdas:

class Form
{
public:
    virtual ~Form() = default;
};

class Rectangle : public Form
{
public:
    // Rectangle needs only the functions necessary to be a rectangle
    Rectangle() { std::cout << "Rectangleee!\n"; }
};

class Square : public Form
{
public:
    // Square needs only the functions necessary to be a square
    Square() { std::cout << "Squareee!\n"; }
};

class Triangle : public Form
{
public:
    // ... and so on...
    Triangle() { std::cout << "Triangleee!\n"; }
};

class Ellipse : public Form
{
public:
    Ellipse() { std::cout << "Ellipseee!\n"; }
};

class Circle : public Form
{
public:
    Circle() { std::cout << "Circleee!\n"; }
};

auto func(std::string_view name) -> std::unique_ptr<Form>
{
    using create_func_t = auto (*)() -> std::unique_ptr<Form>;

    struct value_t
    {
        std::string_view name;
        create_func_t func;
    };

    static constexpr auto all_forms = std::to_array<value_t>({
        {"rectangle", [] { return std::unique_ptr<Form>{new Rectangle{}}; }},
        {"square",    [] { return std::unique_ptr<Form>{new Square{}}; }},
        {"triangle",  [] { return std::unique_ptr<Form>{new Triangle{}}; }},
        {"ellipse",   [] { return std::unique_ptr<Form>{new Ellipse{}}; }},
        {"circle",    [] { return std::unique_ptr<Form>{new Circle{}}; }},
    });

    if (auto p = std::ranges::find(all_forms, name, &value_t::name); p != all_forms.end())
        return (p->func)();
    else
        return nullptr;
}

Adding a new form is now virtually foolproof. Just:

  1. Create the new class. Be sure to publicly inherit from Form, either directly or indirectly, but that’s all you need to do; there are no other functions you need to implement.
  2. Add a new line in the all_forms array. Just a string and a simple lambda: [] { return std::unique_ptr<Form>{new CLASS_NAME_HERE{}}; }.

While I generally abhor macros, you could even make use of one here to make things even simpler:

#define YOUR_PREFIX_FORM(name, class) {name, [] { return std::unique_ptr<Form>{new class{}}; }}

auto func(std::string_view name) -> std::unique_ptr<Form>
{
    using create_func_t = auto (*)() -> std::unique_ptr<Form>;

    struct value_t
    {
        std::string_view name;
        create_func_t func;
    };

    static constexpr auto all_forms = std::to_array<value_t>({
        YOUR_PREFIX_FORM("rectangle", Rectangle),
        YOUR_PREFIX_FORM("square",    Square),
        YOUR_PREFIX_FORM("triangle",  Triangle),
        YOUR_PREFIX_FORM("ellipse",   Ellipse),
        YOUR_PREFIX_FORM("circle",    Circle)
    });

    if (auto p = std::ranges::find(all_forms, name, &value_t::name); p != all_forms.end())
        return (p->func)();
    else
        return nullptr;
}

#undef YOUR_PREFIX_FORM

Now adding a new form is even more trivial. Just create the class (make sure Form is one of its public bases), then add a new line in the all_forms array that’s just “YOUR_PREFIX_FORM(name, class)”. That’s it.

If the list of forms got huge, and you wanted to get clever, you could have the all_forms array compile-time sorted, and then use std::ranges::lower_bound() rather than std::ranges_find(). At the same time you’re sorting, you could also do other compile-time checks, like making sure that all the strings are unique. But that’s getting way beyond the scope of things here.

Let me repeat the code I’ve suggested all together here, just to have it all in one place:

// base class
class Form
{
public:
    virtual ~Form() = default;

    // add any other interface functions here as pure virtual functions
};

// all concrete forms just need to derive from Form, and implement any of the
// pure virtual functions (there are none now, but there could be some)

// a simple concrete form:
class Rectangle : public Form {};

// yes, the above is all you need to create a new concrete form (at least for
// now, as long as there are no pure virtual functions to implement)
//
// but of course, you could add custom constructors, and any other stuff to
// the concrete form classes that you like

// and this is your factory function:
#define YOUR_PREFIX_FORM(name, class) {name, [] { return std::unique_ptr<Form>{new class{}}; }}

auto func(std::string_view name) -> std::unique_ptr<Form>
{
    using create_func_t = auto (*)() -> std::unique_ptr<Form>;

    struct value_t
    {
        std::string_view name;
        create_func_t func;
    };

    static constexpr auto all_forms = std::to_array<value_t>({
        YOUR_PREFIX_FORM("rectangle", Rectangle),

        // add as many cases here as you like
        //
        // you could even do stuff like have aliases (like both "square" and
        // "regular quadrilateral" will return a new Square), or other tricks
        // (like "rectangle" and "square" both return new Rectangles...
        // because a square *is* a rectangle... except "square" returns a new
        // Rectangle with all sides equal; you'd need to tweak the macro to do
        // stuff like this, but it's not too challenging to do)
    });

    if (auto p = std::ranges::find(all_forms, name, &value_t::name); p != all_forms.end())
        return (p->func)();
    else
        return nullptr;
}

// always clean up your macro mess
#undef YOUR_PREFIX_FORM

Note that the code as I’ve written it uses some C++20 stuff… but it doesn’t really need to. You could implement the whole thing even in C++98, but it would be slower, it wouldn’t work at compile-time, you’d have to reimplement a lot of the stuff yourself, and everything would be much, much more verbose. But you could do it. (You could also do it in C++11, C++14, or C++17, and the more modern the version you use, the less work you’ll have to do.)

\$\endgroup\$
9
  • \$\begingroup\$ I hope your use of right-return syntax doesn't alienate or confuse the OP too much! \$\endgroup\$ – JDługosz May 6 at 22:14
  • \$\begingroup\$ Thank you but that was just a simple test code, and not a real code, I appreciate your efforts to show me the factory pattern! thanks bro \$\endgroup\$ – sapristi12 May 7 at 12:46
  • 1
    \$\begingroup\$ @DanielSchepler First, your concern about exception safety is no longer relevant (since C++17); unique_ptr{new T} is exactly as safe as make_unique<T>()… and the former is faster in some cases (hence make_unique_for_overwrite<T>()). Second, look closer. It’s not unique_ptr<Class>{new Class} (which would be redundant; you could just do unique_ptr{new Class}). It’s unique_ptr<Base>{new Derived}. To get consistent deduction with the lambda return, you either have to do that, or make_unique() then cast… unique_ptr<Base>{make_unique<Derived>()}… which would be silly. \$\endgroup\$ – indi May 8 at 17:12
  • 1
    \$\begingroup\$ Note that class Circle : public Ellipse is a common trap that OO beginners fall into: because "Circle is-a Ellipse" in mathematics, they think that means circle should be a subclass of ellipse. But in OO design, the relationship is closer to the other way around (ellipse is a circle with additional behaviour). But neither are a good fit, and being sibling classes is usually best. Geometrical shapes are actually quite a poor teaching aid for class design! \$\endgroup\$ – Toby Speight May 13 at 12:40
  • 2
    \$\begingroup\$ @JDługosz If you want a standard reference, it’s [[expr.call]/8](timsong-cpp.github.io/cppwp/expr.call#8)… but of course it’s standardese, so it’s going to be opaque; you need to understand what “indeterminately sequenced” means. For that, there’s [[intro.execution]/8](timsong-cpp.github.io/cppwp/intro.execution#8), which explains in a note: “cannot overlap, but either can be executed first”. See here (“Stricter order of expression evaluation”) to verify that the change came in C++17. \$\endgroup\$ – indi May 13 at 20:33
3
\$\begingroup\$
Form *func(std::string name)
{
  std::vector<s_makeForms> allForms =
  {
      {"triangle",  Triangle::createForm}, 
      {"rectangle", Rectangle::createForm},
      {"square", Square::createForm}
  };
  for (const auto& form : allForms)
  {
    if (name == form.formName)
      return (form.makeForm());
  }
  return nullptr;
}

since you don't want maps, and whatever other unsaid restrictions you're under... this is all I'll contribute

some general advice.... modern c++ does not, and should not look like c. The languages are different and have different philosophies. what is good in c is not always good in c++. so try to look out for things like that (eg. avoid raw arrays, use nullptr not NULL, little stuff like that.... to start with... this rabbit hole goes deep)

PS. I'm looking forward to other people's answers, this promises to be entertaining...

PPS. Also, take a look at factory design pattern and its variants (abstract factory, etc.) Actually, look up design patterns in general, it's interesting stuff, though YMMV... (edit) nice @indi's got it covered

\$\endgroup\$
1
  • \$\begingroup\$ yeah you're right for the nullptr I forgot ^^ I'm not allowed to use vector neither \$\endgroup\$ – sapristi12 May 7 at 12:48
2
\$\begingroup\$

It looks like others already covered everything I noticed, except for the way you position the * in a declaration.

From the beginning, C++ has used the style of putting the pointer (or ref) mark with the type not with the variable.

Form* myform;

And looking at the main function where this example came from, notice that it is not being initialized, though it is assigned on the next line. Always initialize variables when they are declared, if you can.

Form *myform = func("square");
if (!myform)
{
    std::cerr << "wrong name\n";
}

The return keyword is not a function call! Don't put parens around the entire return value. This has gone from a matter of style (redundant parens in an expression that don't change the meaning) to a real issue, as the compiler recognises a simple bare local variable and automatically invokes move semantics. So return val; is correct and return (val); defeats this automatic optimization.


Don't use the C NULL macro. In C++ there is a keyword nullptr as a proper part of the language.

Look at your function:

Form *func(std::string name)
{
  struct s_makeForms allForms[3] =
  {
      {"triangle",  Triangle::createForm}, 
      {"rectangle", Rectangle::createForm},
      {"square", Square::createForm}
  };
  for (int i = 0; i < NB_FORMS; i++)
  {
    if (name == allForms[i].formName)
      return (allForms[i].makeForm());
  }
  return (NULL);
}

You are passing in name by value when there is no reason to copy the string! This should be const std::string& as a matter of course! However, you can use string_view for parameters and get greater efficiency for passing arguments of either string or lexical string literals (like "rectangle") without having to copy that into a string first.

Form* func(std::string_view name)

You don't need the 3 when sizing the array. The compiler will count the initializers. Also, the structure should be moved into the function, as it is only used there and has no purpose for any other code. Oh, and you don't need struct in front of the name when using the type! Oh again, the array should be static so you're not making it again on every call, and const since you don't change it, but together you just use constexpr.

    struct s_makeForms
    {
        std::string formName;
        Form* (*makeForm)();
    };
    constexpr s_makeForms allForms[] =  {
       {"triangle",  Triangle::createForm}, 
       {"rectangle", Rectangle::createForm},
       {"square", Square::createForm}
    };

Now you don't need the number of elements in the array for the loop, either:

    for (auto& rec : allForms)
    {
        if (name == rec.formName)
           return rec.makeForm();
    }

Notice that you don't need to repeat the allForms[i] as you do in the original. In general you don't want to repeat the navigation into a collection, though in the case of a primitive array the compiler might optimize it anyway, especially if you had made it const.

Finally, if you drop out of the loop:

return nullptr;

no extra parens, and use the built-in keyword, not the ancient C macro.

These issues can of course be combined with other improvements, such as using lambdas directly in the initializer rather than separate static members for each Form.

\$\endgroup\$
9
  • \$\begingroup\$ For the place of the pointer, is it different from the C ? I was used to do int *a; because we could do int *a, *b, int *c For the nullptr, yes you're right thanks ! For the auto &rec; I didn't know this method that's cool thanks, I'll try to use it in the future, and the const &string, also ! Thanks bro I already answered to all the previous posts so I don't have a lot of things to tell to you, but in general thank you for asking me to post my code here, so that I could see all the mistakes ! Thank you bro ! see you soon \$\endgroup\$ – sapristi12 May 7 at 12:52
  • \$\begingroup\$ Pointers: don't do that in C++. Write them as separate lines (with initializers). int* a = foo(); int* b= nullptr; int* c= whatever; Yes, in the original 1987 book Stroustrup intentionally did it differently than in C. \$\endgroup\$ – JDługosz May 7 at 13:50
  • \$\begingroup\$ If you didn't know about auto and/or the range-based for loop, I suspect your textbook or tutorial you're following is simply too old. You need teaching material made after 2011. C++11 is substantially different and lends itself to different best practices. \$\endgroup\$ – JDługosz May 7 at 13:54
  • \$\begingroup\$ i'm currently working on c++98 \$\endgroup\$ – sapristi12 May 10 at 12:13
  • \$\begingroup\$ Why are you using a compiler that's at least 10 years old? BTW, the next update to the standard was C++03, so maybe you meant "pre-2011" rather than something that didn't get patches and bug fixes since 2003. \$\endgroup\$ – JDługosz May 10 at 13:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.