Nano-node: Coding remarks

[paulmelis]

I have seen quite a bit of C++ over the years, but RaiBlocks' coding style really makes me stretch my brain to comprehend what is going on in the sources (in rai/). Below are some notes I made while looking through the code.

I don't have a specific point to make (or issue to fix) here, but I was wondering if it is only me that is having trouble with this. For some of these things I can see the benefit, but my god, it makes me cringe thinking about the discipline required to type and read some of this stuff in this way (although a reformatting tool might be involved). Also, as there doesn't seem to be a readme on the code (or something like that) I have to guess a bit in certain areas as to why things are the way they are.

• Virtually no comments, nor any high-level descriptions of how things are organized
• No empty lines in method bodies and class definitions anywhere, to separate things like variable declarations from method calls using them
• rai:: namespace prefixes, even for method definitions and variable types that are clearly part of that namespace

• Constructor calls instead of assignments, e.g.
  auto node_l (shared_from_this ());

  for (auto i (peers.begin ()), n (peers.end ()); i != n; ++i)

• A space directly before the opening parenthesis of a method call and bracket of a template specialization, making complex statement especially hard to parse, as whitespace can't be trusted to separate logical elements, while also mimicing the frequently used C++ lambda functions, e.g.
  endpoint_l = rai::endpoint (boost::asio::ip::address_v6::v4_mapped (endpoint_l.address ().to_v4 ()), endpoint_l.port ());

  auto existing (blocks.get <1> ().find (hash));

  send_buffer (buffer_a->data (), buffer_a->size (), endpoint_a, [buffer_a, node_w, endpoint_a] (boost::system::error_code const & ec, size_t size) {...}

• Unexplained suffixes on variables, _a ("a ..."?) and _l (local?)

• In constructor definitions member initializations before the body are not indented, leading to hard to parse method definitions, as the constructor definition doesn't stand out. e.g.
  rai::network::network (rai::node & node_a, uint16_t port) : socket (node_a.service, rai::endpoint (boost::asio::ip::address_v6::any (), port)), resolver (node_a.service), node (node_a), bad_sender_count (0), on (true), insufficient_work_count (0), error_count (0) { }
• Methods often return false in case of success. The result variable that is returned from methods is therefore best to think of as being called "failure". For example,
  bool rai::wallet_store::rekey (MDB_txn * transaction_a, std::string const & password_a) { bool result (false); <--- by default, method signals it succeeded by returning false if (valid_password (transaction_a)) <--- valid_password() returns value as expected by method name { <--- when this block is executed a valid password was present rai::raw_key password_new; derive_key (password_new, transaction_a, password_a); rai::raw_key wallet_key_l; wallet_key (wallet_key_l, transaction_a); rai::raw_key password_l; password.value (password_l); password.value_set (password_new); rai::uint256_union encrypted; encrypted.encrypt (wallet_key_l, password_new, salt (transaction_a).owords [0]); entry_put_raw (transaction_a, rai::wallet_store::wallet_key_special, rai::wallet_value (encrypted, 0)); } else { result = true; <--- here, NO valid password was found, but method result is TRUE } return result; }
• In many cases an early-out of the result value can be used, instead of assigning a value to result in multiple places and having only a single return at the end of the method, e.g.
  bool rai::parse_address_port (std::string const & string, boost::asio::ip::address & address_a, uint16_t & port_a) { auto result (false); auto port_position (string.rfind (':')); if (port_position != std::string::npos && port_position > 0) { std::string port_string (string.substr (port_position + 1)); try { uint16_t port; result = parse_port (port_string, port); if (!result) { boost::system::error_code ec; auto address (boost::asio::ip::address_v6::from_string (string.substr (0, port_position), ec)); if (ec == 0) { address_a = address; port_a = port; } else { result = true; } } else { result = true; } } catch (...) { result = true; } } else { result = true; } return result; }
• Lots of nesting of if-statements, where it seems using early-outs (by testing the negative value) would make the code much more readable
• Curly braces around single-line statements. this makes sense for defending against incorrectly missing grouping of statements when adding code, but leads to quite a bit of extra lines for simple code, e.g.
  if (result != end) { if (rai::uint256_union (result->first.uint256 ()) == key) { return result; } else { return end; } } else { return end; }

• Else clauses that only contain a comment but no actual code, e.g.

          if (!error)
          {
              node_a.background ([wallet] ()
              {
                  wallet->enter_initial_password ();
              });
              items [id] = wallet;
          }
          else
          {
              // Couldn't open wallet
          }
  

[paulmelis]

As far as the meaning of the result variable is concerned there's apparently also cases where it really means "result of the method", for example in rai::peer_container::not_a_peer():

bool rai::peer_container::not_a_peer (rai::endpoint const & endpoint_a)
{
    bool result (false);
    if (endpoint_a.address ().to_v6 ().is_unspecified ())
    {
        result = true;
    }
    else if (rai::reserved_address (endpoint_a))
    {
        result = true;
    }
    else if (endpoint_a == self)
    {
        result = true;
    }
    return result;
}

[Dekoze]

Agreed. Lots can be improved through support of a formatter but the structural changes such as early exits are worth it in my opinion as nesting will only get worse and parsing code flow should be as straightforward as possible to minimize bugs.

Comments and information for devs new to the codebase are severely lacking which is not good for on-boarding or preventing ambiguity. Using a documentation generator like doxygen would enable a well-defined comment requirement and do the majority of the legwork for documentation of architecture/hierarchy.

I'm curious what @clemahieu thinks. Adding more documentation would definitely increase his workload since any questions of ambiguity would be directed towards him but at the same time decreasing the friction for new devs as soon as possible also has many benefits.

[clemahieu]

I'm fine with making whatever changes needed for wider adoption, understanding etc.

We can do early-exits, I tend to avoid putting returns in the middle of code because it makes it harder to extract lines containing them in to new functions. It's not a hard rule for me because I know deep nesting is also distracting, that's just the reason why it's there currently.

If you guys could point me to areas that need the most documentation I can work on it. Doxygen sounds fine unless someone has a better recommendation. I'll have to learn its formatting.

[rwolande]

Increased/improved documentation for set up would be really useful, IMO.

[paulmelis]

+1 for providing Doxygen-based documentation (or at least a Doxygen file in the sources so we can build our own). Tip: check out doxywizard (included in doxygen) for easy setup of a config file.

Some suggestions on what to document (talking mostly about the stuff in rai/ here):

• How code in the different subdirs (lib, node, qt, qt_system, etc) relate to each other. This is already mostly clear from the names, but might not hurt to make it explicit with a short overview
• A few sentences per major C++ class (or source file), describing what its function is, relations to other classes, runtime aspects (like "runs it own thread"), etc. The class names are already pretty descriptive, but for example a class like rai::work_pool is a bit opaque in what it provides exactly, while other classes might do more than their name suggests.
• Chosen coding style/structure w.r.t. return values of methods, suffixes _l and _a, how values are returned from methods (e.g. always in the first argument(s))
• Places to look for in the code for important sections of the 2014 and 2017 white papers, although I'm not sure if the 2014 one is still entirely accurate?
• Description of some of the concepts used in the code that are not in the whitepapers, like bootstrapping and frontier (= account?)

[timower]

Also any documentation of the peer to peer protocol would be nice.
Every class really should have at least some documentation of what it does.