Question

My question regards coding style and the decomposition of complicated expressions in C++.

My program has a complicated hierarchy of classes composed of classes composed of classes, etc. Many of the program's objects hold pointers to, or indices into, other objects. There are a few namespaces. As I said, it's complicated—by which I mean that it is a pretty typical 10,000-line C++ program.

A problem of scale emerges. My code is starting to have lots of unreadable expressions like p->a(b.c(r).d()).q(s)->f(g(h.i())). (As I said, it's unreadable. I have trouble reading it, and I was the one who wrote it! You can just look at it to catch the mood.) I have tried rewriting such expressions as

{
    const I  ii = h.i();
    const G &gg = g(ii);
    const C &cc = b.c(r);
    // ..
    T *const qq = aa.q(s);
    qq->f(gg);
}

All those locally scoped symbols arguably make the code more readable, but I admit that I do not care for the overall style. After all, some of the local symbols are const & references, while others represent copies of data. What if I accidentally omitted one of the &, thereby invoking an unnecessary copy of some large object? My compiler would hardly warn me.

Besides, the version with the local symbols is verbose.

Neither solution suits. Does there not exist a more idiomatic, less bug-prone way to decompose unreadable expressions in C++?

ILLUSTRATION

If a minimal, compilable illustration of the problem helps, then here is one.

#include <iostream>

class A {
  private:
    int m1;
    int n1;
  public:
    int m() const { return m1; }
    int n() const { return n1; }
    A(const int m0, const int n0) : m1(m0), n1(n0) {}
};

class B {
  private:
    A a1;
  public:
    const A &a() const { return a1; }
    B(const A &a0) : a1(a0) {}
};

B f(int k) {
    return B(A(k, -k));
}

int main() {

    const B  &my_b = f(15);

    {
        // Here is a local scope in which to decompose an expression
        // like my_b.a().m() via intermediate symbols.
        const A  &aa = my_b.a();
        const int mm = aa.m();
        const int nn = aa.n();
        std::cout << "m == " << mm << ", n == " << nn << "\n";
    }

    return 0;

}

ADDITIONAL INFORMATION

I doubt that it is relevant to the question, but in case it is: My program defines several templates, but does not presently use any run-time polymorphism.

AN ACTUAL EXAMPLE

One of the commenters has kindly requested an actual example out of my code. Here it is:

bool Lgl::is_order_legal_for_movement(
    const Mb::Mapboard &mb, const size_t ix, Chains *const p_chns1
) {

    if (!mb.accepts_orders()) return false;
    const Basic_mapboard &bmb = mb.basic();
    if (!is_location_valid(bmb, ix, false)) return false;
    const Space            &x = mb.spc(ix);
    if (!x.p_unit()) return true;
    const size_t           cx = x.i_cst_occu();
    const Basic_space     &bx = x.basic();
    const Unit             &u = x.unit();
    const bool           army = u.is_army();
    const bool          fleet = u.is_fleet();
    const Order         order = u.order();
    const size_t           is = u.source();
    const Location         lt = u.target_loc();
    const size_t           it = lt.i_spc;
    const size_t           ct = lt.i_cst;
    // ...

    {   
        const Space        &s = mb.spc(is);
        const Basic_space &bs = s.basic();
        result = ( 
            (army_s && (
                bs.nbor_land(it) || count_chains_if(
                    Does_chain_include(ix), chns_s, false
                )   
            )) || (fleet_s && (
                // By rule, support for a fleet can name a coast,
                // but need not.
                ct == NO_CST
                ? is_nbor_sea_no_cst(bs, cs, it) 
                : is_nbor_sea       (bs, cs, lt) 
            ))  
        ) && is_nbor_no_cst(army, fleet, bx, cx, it);
    }   

    // ...

}
Was it helpful?

Solution

For your actual code example, I can see why you'd like to make it more readable. I'd probably recode it something like this:

if (army_s) {
    result = bs.nbor_land(it) || 
            count_chains_if(Does_chain_include(ix), chns_s, false);
} else if (fleet_s) {
    // By rule, support for a fleet can name a coast,
    // but need not.
    if (ct == NO_CST)
        result = is_nbor_sea_no_cst(bs, cs, it);
    else 
        result = is_nbor_sea(bs, cs, lt);
}
result &= is_nbor_no_cst(army, fleet, bx, cx, it);

It executes the same logic, including short-circuit evaluations, but is a little better for human interpretation, I think. I have also encountered compilers that also generate better code with this style versus the very complex compound line the original code contained.

Hope that helps.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top