3
\$\begingroup\$

I had an assignment to create a table that will be stored in ROM (read only) and will be used in some algorithms (to convert numbers to strings). However sometimes a user will want to edit the table a bit before using it in the algorithms. So the user must be able to copy the table to RAM, change it and then use it.

With this in mind I have come up with the following code. Note: This is a simplified version of my code to illustrate the solution. In reality RomClass has more members.

Class representing a row in the table:

struct Row
{
    constexpr Row() = default;
    constexpr Row(float f, int i) : num_float(f), num_int(i) {}

    float num_float = 0;
    int num_int     = 0;
};

Class representing the table in ROM (read only):

class RomClass
{
public:
    template <std::size_t N>
    constexpr RomClass(const Row (&table)[N])
    : m_table(table),
      m_size(N)
    {
    }


    std::size_t size() const                    { return m_size; }
    const Row& row(std::size_t i) const         { assert(m_size > i);  return m_table[i]; }

private:
    const Row* m_table  = nullptr;
    std::size_t m_size  = 0;


    friend class RamClass;
};

Class used to edit the table. It copies the table to RAM and allows editing. It also has a member that returns a reference to RomClass so it can be used with algorithms.

class RamClass
{
public:
    RamClass(const RomClass& rom);

    RamClass(const RamClass& rhs);
    RamClass& operator= (RamClass rhs);

    void swap(RamClass& rhs);

    const RomClass& romClass() const        { return m_rom; }  // for algorithms that work with RomClass
    std::size_t size() const                { return m_table.size(); }
    Row& row(std::size_t i)                 { return m_table[i]; }


private:
    RomClass m_rom;
    std::vector<Row> m_table;
};

inline void swap(RamClass& lhs, RamClass& rhs)
{
    lhs.swap(rhs);
}


RamClass::RamClass(const RomClass& rom)
: m_rom(rom)  // naredimo kopijo
{
    m_table.resize(rom.m_size);                                             
    std::copy(&rom.m_table[0], &rom.m_table[rom.m_size], &m_table[0]);      

    m_rom.m_table = &m_table[0];
}

RamClass::RamClass(const RamClass& rhs)
: m_rom(rhs.m_rom),
  m_table(rhs.m_table)
{
    const Row*& table = *const_cast<const Row**>(&m_rom.m_table);         
    table = &m_table[0];
}

RamClass& RamClass::operator=(RamClass rhs)
{
    this->swap(rhs);
    return(*this);
}

void RamClass::swap(RamClass& rhs)
{
    using std::swap;

    swap(m_table, rhs.m_table);

    auto lhsTable = m_rom.m_table;
    auto rhsTable = rhs.m_rom.m_table;
    swap(lhsTable, rhsTable);
}

Algorith function would look something like:

void algorithm(const RomClass& rom)
{
    for (auto i = 0; i < rom.size(); ++i)
    {
        // do sth
    }
}

Usage:

constexpr Row romTable[] =
{
    Row(19.99F, 2),
    Row(2.3F, 4),
    Row(),  // end element
};
constexpr RomClass romClass =
{
    romTable
};

int main()
{
    RamClass ramClass = romClass;
    ramClass.row(0).num_float = 9000;    // allows editing

    romClass.row(0).num_float = 9000;    // error

    RomClass romClass2(romTable);
    romClass2.row(0).num_float = 9000;  // also an error

    return 0;
}

Ideally I would want to ditch the RamClass and just have a RomClass that would also have non-const member functions that would expose the table and allow editing. But then the following code would not produce an error:

RomClass romClass2(romTable);
romClass2.row(0).num_float = 9000;  // not an error, but it should be

I would appreciate any improvements that I could do with this code while achieve the goals I posted above and be as user friendly as possible - not allow to accidentally edit read only data.

\$\endgroup\$
9
  • 1
    \$\begingroup\$ Why not just have a single class and declare the ROM version constexpr? That would be both much simpler and more clear. \$\endgroup\$
    – Edward
    Feb 29 '16 at 15:56
  • \$\begingroup\$ @Edward Because RomClass has a pointer to the table. If you create a non constexpr object and put a pointer to a constexpr table inside, you allow the user to change a readonly object, which is a runtime error. \$\endgroup\$
    – rozina
    Mar 1 '16 at 7:20
  • \$\begingroup\$ Not a problem if you write an appropriate copy constructor. \$\endgroup\$
    – Edward
    Mar 1 '16 at 7:47
  • \$\begingroup\$ @Edward I don't think that matters. If you write RomClass ramObject(romTable); and then change romTable through ramObject, that would be a runtime error. No copy constructor was called yet. \$\endgroup\$
    – rozina
    Mar 1 '16 at 7:52
  • \$\begingroup\$ What you just wrote is an invocation of a copy constructor. If you write one that actually makes a copy, you can't possibly alter the original const object with the copy. \$\endgroup\$
    – Edward
    Mar 1 '16 at 8:12
1
\$\begingroup\$

I see a number of things that may help you improve your code.

Improve naming

The names Row and m_table are good because they are descriptive, but num_float and num_int are not very good. It would be better if the variable names described their significance rather than their format. Similarly, while RomClass and RamClass may be salient to you, the implementer, they're not really a description of what is actually in those classes.

Throw an exception rather than using assert

The code for RomClass currently has this member function:

const Row& row(std::size_t i) const { 
    assert(m_size > i);  
    return m_table[i]; 
}

However, it would probably be better to throw a std::out_of_range error and let the caller figure out the appropriate action.

Avoid raw access to class internals

As the code is written right now, internal Row data can be altered by anything that has access to the containing class. This might be convenient but it's generally better to allow access to internal class data only through explicit accessor functions. Additionally, it's generally safer to return a copy of a member (such as a Row in this code) rather than a reference.

Use the same class for both purposes

The difference between your RomClass and RamClass is exactly the same as the difference between a const variable and a non-const variable. Since this notion of const-ness already exists and is explicitly supported by the standard, it's better to simply use that instead of having two different classes for essentially the same thing. This comes close (but don't use it!) and is a simple combination of your two classes as a single class:

class Table
{
public:
    template <std::size_t N>
    explicit constexpr Table(const Row (&table)[N])
    : m_size(N),
      m_table(const_cast<Row *>(table))
    {
    }
    Table(const Table& other) 
    : m_size(other.m_size)
    {
      m_table = new Row[m_size]();
    }
    std::size_t size() const { return m_size; }
    const Row& row(std::size_t i) const { 
        if (m_size <= i) { 
            throw std::out_of_range("Table index out of bounds");
        }
        return m_table[i]; 
    }
    Row& row(std::size_t i) { 
        if (m_size <= i) { 
            throw std::out_of_range("Table index out of bounds");
        }
        return m_table[i]; 
    }
private:
    std::size_t m_size  = 0;
    Row* m_table  = nullptr;
};

We can now use this as follows:

static constexpr Row romTable[] 
{
    Row(19.99F, 2),
    Row(2.3F, 4),
    Row(),  // end element
};
constexpr Table romClass{romTable};

int main()
{
    Table ramClass{romClass};
    ramClass.row(0).num_float = 9000;    // allows editing

//    romClass.row(0).num_float = 9000;    // error
    assert(ramClass.row(0).num_float == 9000);
    assert(romClass.row(0).num_float == 19.99F);

    const Row myrow = romClass.row(1);
    assert(myrow.num_int == 4);
    assert(myrow.num_float == 2.3F);

    const Table romClass2(romTable);
//    romClass2.row(0).num_float = 9000;  // also an error

}

It doesn't use all of the suggestions above (for example, it still returns a &Row) but it's close to what you want. There is one significant problem with this, however, which is that there is no destructor and so this version will leak memory. There are a number of ways to fix this, but a simple one is to derive a new class from Table called WritableTable. Now everything works and we don't leak memory:

class Table
{
public:
    template <std::size_t N>
    explicit constexpr Table(const Row (&table)[N])
    : m_size(N),
      m_table(const_cast<Row *>(table))
    {
    }
    std::size_t size() const { return m_size; }
    const Row& row(std::size_t i) const { 
        if (m_size <= i) { 
            throw std::out_of_range("Table index out of bounds");
        }
        return m_table[i]; 
    }
    Row rowCopy(std::size_t i) const {
        if (m_size <= i) { 
            throw std::out_of_range("Table index out of bounds");
        }
        return m_table[i]; 
    }
protected:
    std::size_t m_size;
    Row* m_table = nullptr;
};

class WritableTable : public Table
{
public:
    WritableTable(const Table& other) 
    : Table(other)
    {
        m_table = new Row[m_size]();
        for (size_t i=0; i < m_size; ++i) {
            m_table[i] = other.rowCopy(i);
        }
    }
    Row& row(std::size_t i) { return m_table[i]; }
    ~WritableTable() {
        delete[] m_table;
    }
};

static constexpr Row romTable[] 
{
    Row(19.99F, 2),
    Row(2.3F, 4),
    Row(),  // end element
};

static constexpr Table romClass{romTable}; 

int main()
{
    WritableTable ramClass{romClass};
    ramClass.row(0).num_float = 9000;    // allows editing

//    romClass.row(0).num_float = 9000;    // error
    assert(ramClass.row(0).num_float == 9000);
    assert(romClass.row(0).num_float == 19.99F);

    const Row myrow = romClass.row(1);
    assert(myrow.num_int == 4);
    assert(myrow.num_float == 2.3F);

    const Table romClass2(romTable);
//    romClass2.row(0).num_float = 9000;  // also an error
    WritableTable ramClass2{romClass};
    ramClass2.row(0).num_int = 42;
    assert(ramClass2.row(0).num_int == 42);

}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the answer. The code that I posted was simplified production code, so naming was done quickly to show the concept :) I also forgot to mention this is an embedded project and we do not use exceptions, that is why I only use asserts. \$\endgroup\$
    – rozina
    Mar 2 '16 at 7:21
  • \$\begingroup\$ Regarding your Table class. I like the idea, but the reason I implemented RamClass as a seperate class is to not allow the user to create a runtime error, which can be done with a class like Table. The user can create write Table ramClass(romTable) (romTable being an array of Row objects) and now has a non-const Table and can edit a read only romTable. I like the idea that WritableTable derives from Table. I guess RamClass could derive from RomClass and then be directly usable in algorithm functions. \$\endgroup\$
    – rozina
    Mar 2 '16 at 7:25

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.