1
\$\begingroup\$

I don't need a lot and I don't need for my code to handle everything. Even now, I'm just looking at handling serialization. I'm pretty much building on it as needed. I am trying to support vectors right now and am looking for an idea on how to extend what I have a little to support this. Basically I've created a property class that has a callback function when the property changes (keeping it simple with equals operator for now). This updates a dictionary object that can be serialized. Below is my code. It's my first time posting. If I didn't follow some rules by accident, I'd love some guidance. Working sample here: https://onlinegdb.com/H10iJVC3f

#include <iostream>
#include <functional>
#include <sstream>
#include <map>

using namespace std;

template <class T>
class Prop
{
public:
   Prop(const std::string &key, const std::function<void(std::string, std::string)> &changed);

private:
   std::string Key;
   T Value;
   std::function<void(std::string, std::string)> changed;

public:
   void operator=(const T &value) {
      this->Value = value;
      this->changed(this->Key, static_cast<ostringstream&>(ostringstream() << value).str());
   }
};

template <class T>
Prop<T>::Prop(const std::string &key, const std::function<void(std::string, std::string)> &changed)
{
   this->Key = key;
   this->changed = changed;
}

class TheList
{
private:
   std::map<std::string, std::string> theList;

protected:
   void Update(const std::string &key, const std::string &value);

public:
   std::string ToJson();
};

void TheList::Update(const std::string &key, const std::string &value)
{
   theList[key] = value;
}

// TODO: body of this method will change and will use a processing library.
std::string TheList::ToJson()
{
   std::string json = "{\n";

   for (auto it : theList)
   {
      json += "\t\"" + it.first + "\": " + "\"" + it.second + "\",\n";
   }

   json.pop_back();
   json.pop_back();

   json += "\n}";

   return json;
}

// A Serializable object.
class User : public TheList
{
public:
   User();

public:
   Prop<std::string> Name;
   Prop<short> Age;
   Prop<std::string> Address;
};

User::User() : Name("Name", std::bind(&User::Update, this, std::placeholders::_1, std::placeholders::_2)),
Age("Age", std::bind(&User::Update, this, std::placeholders::_1, std::placeholders::_2)),
Address("Address", std::bind(&User::Update, this, std::placeholders::_1, std::placeholders::_2))
{
}

int main()
{
    User user;

    user.Name = "Pamela";
    user.Age = 25;
    user.Address = "USA";

    std::string json = user.ToJson();

    cout << json << endl;

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! What task does this code accomplish? Please tell us, and also make that the title of the question via edit. Maybe you missed the placeholder on the title element: "State the task that your code accomplishes. Make your title distinctive.". Also from How to Ask: "State what your code does in your title, not your main concerns about it.". \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 25 '18 at 16:25
  • \$\begingroup\$ Thanks @SamOnela I hope I fixed it up based on the pointers you gave me. \$\endgroup\$ – Pittfall Apr 25 '18 at 17:49
  • \$\begingroup\$ The post is fine as currently written, with the code present. Others have included pointers to a code site also and I have found that useful. Don’t forget to upvote answers that are helpful to you. \$\endgroup\$ – JDługosz Apr 25 '18 at 19:45
1
\$\begingroup\$

Don’t say using namespace std;.

And you were qualifying names in std anyway! What gives?


std::function<void(std::string, std::string)> &changed)

Why must you pass the strings by value to the callback?

You have this std::function signature spelled out twice — you might want to make an alias for that.

using cbfunc = std::function<void(std::string, std::string)> changed)
  ⋮
⋯Prop(const std::string& key, const cbfunc& changed)

In C++, the style is to put the & modifier with the type, not the declared name. This has been true since Bjarne’s original book, where it is pointed out explicitly.


this->Key = key; this->changed = changed;

No…
Write your constructor initializers, not assignment on top of default-initialized members. Learn the difference between initialization and assignment.

: Key{key}, changed{changed}

and note that having the parameter with the same name as the data member is not a problem and is in fact idiomatic. If you ever need to write this-> a lot, you are doing something wrong.

In this particular case, take advantage of move semantics. In full:

Prop<T>::Prop(std::string key, cbfunc changed)
: Key{std::move(key)}, changed{std::move(changed)}
{ }

static_cast<ostringstream&>(ostringstream() << value).str()

What’s with the static_cast? Don’t do that.

Since you are modelling a value that does things behind the scenes when written to, using operator= for the name is probably OK in this case. But it should not be void. Either return *this or value.

Again, don’t use this-> to access members.

Prop& operator= (const T& value)
    {
    set(value);
    return *this;
    }

void set (const T& value)
    {
    Value= value;
    std::ostringstream& fmt;
    fmt << value;
    changed (Key, fmt.str());
    }

But, there is a handy package that already does the formatting exactly like this. So just write

    Value= value;
    changed (Key, boost::lexical_cast<std::string>(value));

Note that I made a normal-named function set and then defined operator= to call it. That will be much better for people who read your code later, since operator= carries certain expectations.


You did not provide the matching get function! Your values are write-only!

In keeping the callbacks as invisible as possible, the design would be to have a function get and then an operator T that calls it.

 int x;
 Prop<int> px {"px", some_function};
 px = 5;  // invisible in
 x = px;  // and invisible out.

std::bind is deprecated since C++11. Sometimes it is indeed the simplest way to write something. But with placeholders, not so much.

Since all your props use the exact same function, you should just set it up once.

// member of class User
const auto binderCB = [this](std::string left, std::string right)
    { Update (left, right); }

Then the constructor is

User::User()
: Name{cb("Name")}, Age{cb("Age")}, ⋯

(just be sure cb is the first member so it is initialized first!)


(almost always) use auto.
You don’t have any uses at all, so I’ll point it out with an example in your main:

auto json = user.ToJson();

The main might be a throw-away example, but I’d rather initialize the user object in one go, not assign to each prop individually.

User user { "Pamela", 25, "USA" };

for a plain struct, this would just work as an aggregate initializer. You need a constructor that takes three arguments (with defaults) to provide the same functionality.


Notes

I’d normally write all those as default initializers in-line and not give a constructor at all. But for this specific example, I worry about referring to other members. I’d need to look that up to see if it’s OK.

Rather than updating a map every time you change a value, you can use the same effort of setting up each member with an identifier and callback but provide a general way to explore the current values and types when you are ready to do something with them. That is, pull rather than push.

There are libraries that do this, most notably Boost::Fusion. But there are more modern ways — I think Boost::Hanna has that, but I don’t recall for sure. An any case, the thing to do is to supply a non-member get template so the object can be used just like a tuple`. This will let it interoperate with other code that follows this approach, including the built-in structured bindings.

Since your callback turns everything into a string representation, your Json rendering will not know what things were originally strings and what are numbers. Your code will output

"Age" : "25"

instead of

"Age" : 25

In your design (where everything is handed over as a string) you can include the quoting as part of the update callback. Use one for strings that add quotes and also escapes out needed characters, and another for numbers that does not. Then, don’t add the quotes in your outputing function.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, I appreciate it and and agreed with a lot of it. using std was a mistake by the way, default code from a site I was using. Why do you say "don't use this"? This to me sounds like preference. Do you have a good argument for that? \$\endgroup\$ – Pittfall Apr 25 '18 at 19:15
  • \$\begingroup\$ Writing this->x = 5; etc. for all member access is simply not the way C++ is done. Trying to read C++ written in a Python dialect is like trying to read a novel where some character’s forign accent is spelled out as “eye dialect”. Generally, we want to remove noise words and stuff is readable when it is idiomatic. Seeing this-> I am alerted to something funny/difficult going on here: name clash with local scope, or needs to be found in second-stage lookup. \$\endgroup\$ – JDługosz Apr 25 '18 at 19:43
  • 1
    \$\begingroup\$ Thanks for the added feedback, I definitely changed a few things based on it. Now I will work on strategies to implement the vector functionality I want to do next. \$\endgroup\$ – Pittfall Apr 27 '18 at 16:42

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.