Sugestions on Macro Usage

Question

Originally most lines of code were lengthy. I decided to use macros to be short and clear. I was wandering if using macros like this is bad practice. I personally think it looks cleaner with the macros, but with the macros hiding names and functions some may be confused as to how I have implemented my methods.

Original Code

...
namespace ConwaysGameOfLife
{
    class Grid
    {
    private:
    //Members    
        ...
        using set_of_ints  = std::unordered_set<int>;
        using set_of_sizes = std::unordered_set<std::size_t>;
        //Members to be used in Macros
        set_of_sizes cells_at_t_minus_one;
        set_of_sizes cells_at_t;
        ...
    private:
    //Functions
        ...
        //Function showing Lengthy Conditionals 
        void rule(const std::size_t& cell_position)
        {
            std::size_t live_neighbors{0};

            for(const auto& neighbor_offset : neighbor_offsets)
                /*! Total Neighbors !*/
            {
                //Lengthy Conditional 
                if(cells_at_t_minus_one.find(cell_position + neighbor_offset) != cells_at_t.end())
                {
                    live_neighbors++;
                }
            }
            //Lengthy Conditional
            if(cells_at_t_minus_one.find(cell_position) != cells_at_t.end() and live_neighbors < 2)
                /*! Underpopulation !*/
            {
                cells_at_t.erase(cell_position);
            }
            //Lengthy Conditional
            else if(cells_at_t_minus_one.find(cell_position) != cells_at_t.end() and (live_neighbors == 2 or live_neighbors == 3))
                /*! Aging of a Cell !*/
            {
                cells_at_t.insert(cell_position);
            }
            //Lengthy Conditional
            else if(cells_at_t_minus_one.find(cell_position) == cells_at_t.end() and live_neighbors == 3)
                /*! Birth of a Cell !*/
            {
                cells_at_t.insert(cell_position);
            }
            //Lengthy Conditional
            else if(cells_at_t_minus_one.find(cell_position) != cells_at_t.end() and live_neighbors > 3)
                /*! Overpopulation !*/
            {
                cells_at_t.erase(cell_position);
            }
        }
        public:
        ...
    };
}
...

Code with Macros

...
#define neighbor cells_at_t_minus_one.find(cell_position + neighbor_offset)
#define cell cells_at_t_minus_one.find(cell_position)
#define dead cells_at_t.end()
#define is_live != dead
#define is_dead == dead
#define result second

namespace ConwaysGameOfLife
{
    class Grid
    {
    private:
    //Members    
        ...
        using set_of_ints  = std::unordered_set<int>;
        using set_of_sizes = std::unordered_set<std::size_t>;
        //Members used in Macros
        set_of_sizes cells_at_t_minus_one;
        set_of_sizes cells_at_t;
        ...
    private:
    //Functions
        ...
        void rule(const std::size_t& cell_position)
        {
            std::size_t live_neighbors{0};

            for(const auto& neighbor_offset : neighbor_offsets)
                /*! Total Neighbors !*/
            {
                //Macros used
                if(neighbor is_live)
                {
                    live_neighbors++;
                }
            }
            //Macros used
            if(cell is_live and live_neighbors < 2)
                /*! Underpopulation !*/
            {
                cells_at_t.erase(cell_position);
            }
            //Macros used
            else if(cell is_live and (live_neighbors == 2 or live_neighbors == 3))
                /*! Aging of a Cell !*/
            {
                cells_at_t.insert(cell_position);
            }
            //Macros used
            else if(cell is_dead and live_neighbors == 3)
                /*! Birth of a Cell !*/
            {
                cells_at_t.insert(cell_position);
            }
            //Macros used
            else if(cell is_live and live_neighbors > 3)
                /*! Overpopulation !*/
            {
                cells_at_t.erase(cell_position);
            }
        }
        public:
        ...
    };
}

#undef neighbor
#undef cell
#undef dead
#undef is_live
#undef is_dead
#undef result
...

Answer 1

This:

if (neighbor is_live) { ... }

is absolutely impossible to understand. It looks like ill-formed code, and anybody reading it will first and foremost think that there's something wrong there. Forget your peers, this is the sort of code that you will come back to in a few months and not understand for quite some time.

It also yields a lot of problems in that things like neighbor, dead, cell, result, and is_live are reasonable names to use as identifiers for this particular program. So if you happen to use them as identifiers, you will end up with fairly inscrutable errors.

Consider the alternative:

if (is_live(cell_position + neighbor_offset)) { ... }

where we just have a function is_live that just does the right thing:

bool is_live(size_t idx) { return !cells_at_t_minus_one.count(idx); }

This is far superior because:

1. It actually looks like syntactically correct code.
2. All the variables we're using are clear and visible in the code.
3. I can use whatever identifiers I want, wherever I want them, and I don't have to worry about the preprocessor overwriting them.
4. My variable names aren't set in stone. If I want to change neighbor_offset to just be offset, that name change only has effects for the scope of neighbor_offset (which in your code doesn't even appear to be used). I don't have to then change the definition of my macro!

---

Side-note, your is live check is comparing iterators from one container (cells_at_t_minus_one) with another container (cells_at_t).