-4
\$\begingroup\$

Hello I wrote the following code. It allows me to use objects as keys in a map. I have a constraint though:

  • In addElement function both arguments must be pointers

// Example program
#include <iostream>
#include <string.h>
#include <map>


typedef struct {
    std::string name;
} Test;

class Tester {
    /*I want the TestCompare to be in class Tester*/
    struct TestCompare {
      bool operator()(const Test & LQuery, const Test & RQuery) const {
        return LQuery.name.compare(RQuery.name) < 0;
      }
    };
    public:
      Tester() {};
      ~Tester() {};
      void addElement(const Test *key, const Test *value) {
          m.insert(std::make_pair((*key),value));
      }

      void printElement() {
          for(auto &e:m){
              std::cout<< e.first.name << " " << e.second->name << std::endl;
          }
      }
    private:
      std::map<const Test, const Test*,TestCompare> m;
};

int main()
{
  Test k1 = {"1"};
  Test v1 = {"1"};

  Test k2 = {"2"};
  Test v2 = {"2"};

  Test k3 = {"3"};
  Test v3 = {"3"};

  Tester tester;
  tester.addElement(&k1, &v1);
  tester.addElement(&k2, &v2);
  tester.addElement(&k3, &v3);
  tester.printElement();
  return 0;
}

Questions

  • Is the TestCompare implemented correctly?
  • Does it bother you on how I dereference the pointer? I MUST have a pointer to object in the arguments
  • Is TestCompare placed correctly inside the class Tester?
\$\endgroup\$
  • 1
    \$\begingroup\$ Is there a reason you can't simply use a std::map<std::string, std::string> (or a std::map<std::string_view, std::string_view>)? \$\endgroup\$ – hoffmale Jul 19 '18 at 13:31
  • \$\begingroup\$ yes @hoffmale the map must contain objects. I don't have a choice \$\endgroup\$ – Hani Gotc Jul 19 '18 at 13:33
  • \$\begingroup\$ @Incomputable excuse me sorry again for the question but what you are saying where does it apply? \$\endgroup\$ – Hani Gotc Jul 19 '18 at 14:18
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ – Hani Gotc Jul 19 '18 at 14:21
2
\$\begingroup\$
#include <string.h>

I'm sure you either mean <string> or <cstring>, probably the former.

typedef struct {
    std::string name;
} Test;

This is not C; there is no sensible reason for defining a type like this in C++. What you probably mean is:

struct Test {
    std::string name;
};

On to the Tester class....

bool operator()(const Test & LQuery, const Test & RQuery) const {
  return LQuery.name.compare(RQuery.name) < 0;
}

Since the compare() method is noexcept, you could make the whole function noexcept.

But is there any real reason to use compare() explicitly? Why not just LQuery.name < RQuery.name?

Of course, you don't even need TestCompare at all, but I assume since 2⁄3 of your questions are about TestCompare, you have some other reasons for using it.

Tester() {};
~Tester() {};

Neither of these is necessary, and in fact, defining them like this blocks certain optimizations. If you don't need to define implicitly generated functions, don't. If you really want to define them anyway, use default:

Tester() = default;
~Tester() = default;

But like I said, better to not even declare them at all unless you have to.

void addElement(const Test *key, const Test *value) {
  m.insert(std::make_pair((*key),value));
}

I'm going to take it for granted that you "MUST" take two pointers in this function. It's a terrible constraint, but it wouldn't be the first time I've seen a really dumb constraint.

But if you are going to take pointers, you need to clarify whether you will allow those pointers to be nullptr or not. If your function can take nullptr for either/both arguments, you need handle that case properly (for example, by throwing an exception, raising an assert, etc.). If your function will not accept nullptr, you need to document that fact somehow. Starting in C++20 you could use contracts. For now, at least a comment.

So:

  • If the user is allowed to pass nullptr: handle that case in the function.
  • If the user is not allowed to pass nullptr: document that fact somehow (at least with a comment).

That's just the dance you have to go through anytime you "MUST" take pointers as parameters to functions in C++.

Now as for the function body itself, it could be simplified quite a bit with emplace():

void addElement(const Test* key, const Test* value) {
  m.emplace(*key, value);
}

On to printElement()....

void printElement() {
    for(auto &e:m){
        std::cout<< e.first.name << " " << e.second->name << std::endl;
    }
}

First, this function's name isn't really correct. It doesn't print an element. It prints the whole container, keys, values, and all.

Second, there is no reason it couldn't be const.

Inside the function, it is generally preferred in modern C++ to keep the type information together, and the variable name separate. So auto& e is preferred over auto &e.

Since you're not modifying the map element, it might better be auto const& (or const auto&, whichever you prefer).

Even better, if you are using C++17, would be a structured binding:

for (auto const& [ key, value ] : m)
    std::cout << key.name << ' ' << value->name << '\n';

And finally, don't use std::endl.

std::map<const Test, const Test*,TestCompare> m;

There's no real benefit to using const for the key in a map, because it's const internally anyway. In other words, all you need is:

std::map<Test, const Test*, TestCompare> m;

Questions

  • Is the TestCompare implemented correctly?

    Sure.

  • Does it bother you on how I dereference the pointer? I MUST have a pointer to object in the arguments

    It bothers me that you're using pointers at all, but since you keep insisting that you "MUST" use pointers, then it bothers me that you don't give any indication of how you're handling null pointers. Whenever your function is taking pointers, you either have to deal with null pointers, or specify in the function's interface that they're not valid arguments. Even just a comment will do (until C++20).

  • Is TestCompare placed correctly inside the class Tester?

    Sure.

\$\endgroup\$
  • \$\begingroup\$ on top of that I will have another problem with the pointer. I have no idea how the pointer is initialized it can be a dynamic pointer Test * key = new Test; I will have to make sure that I delete it too. when I am done. But If i do that and the other user that doesn't know how my code works stills needs it he will be in trouble. Is that right? \$\endgroup\$ – Hani Gotc Jul 20 '18 at 8:23
  • \$\begingroup\$ I was reading some articles on what u said concerning the pointer argument. It's a MESS! as you said I should add the comment that it is not supposed to be null and I should also check it myself. It's a pain in the ass \$\endgroup\$ – Hani Gotc Jul 20 '18 at 11:46

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