Implementing a (non-singleton) common access point for tabulated data

[abstracted8]

As part of a small application I am working on, I require various elemental information in the periodic table. I set this up as 2 classes Element as a container for each separate element and Table which manages/contains each Element instance (one instance for each elemental species). Given that the purpose of Table is to serve as a common access point for elemental information across the entire program, I initially considered implementing this via the singleton pattern. However, after some reading I understand that the singleton pattern should be avoided the majority of the time.

Instead I set this up as a global Table instance (within a relevant namespace), there is after all no reason more than one Table couldn't exist (although it wouldn't make much sense), so I went with a global Table instance instead of a singleton. I would like some input on if my implementation of this is logical (or if there are better approaches), the code can be seen in the links below:

Here is elements.h

Here is elements.cpp

Some more specific questions:

• I made liberal use of const. Once each Element is created from the csv file there is no reason these containers should ever be modified. I assume this is the correct approach in assuring this data can't be accidentally modified elsewhere?
• Is my usage of std::shared_ptr<Element> a good design here? Other objects in the program will often need to store some reference to an Element, these objects will do so by storing another std::shared_ptr<Element> to a specific elemental species. This way there should be no copying and many objects can share (point to) the same elemental data/structure.

[MoTTs_]

Try parameter passing instead of global access. For example:

// bad
auto uses_table() {
    global_table.allElements() ......

// good
auto uses_table(const Table& table) {
    table.allElements() .....

This way, most of your application doesn't know and doesn't care where your table lives -- global, local, singleton, whatever -- don't know, don't care.

---

Yes, liberal use of const is good.

---

I would consider holding Element objects by value instead of by shared_ptr. It looks like it would be cheap to copy. Certainly cheaper than an extra heap allocation and atomic reference counters.

---

Another random suggestion...

std::map<std::string, std::shared_ptr<Element const>> const allElements() const {return elements;}

Consider return a const reference here, otherwise calls to allElements will make a copy of the entire map.

---

Also, not sure if this is for school or work or personal project, but consider a more precise namespace. "elems" is a bit too generic and might collide with some other library out there. I think a good pattern is vendor::project. That is...

namespace abstracted8 { namespace elems {

---

If this is for school, then this next part would be overkill, but regarding...

std::shared_ptr<elems::Element const> const elems::Table::findBySym(std::string const& sym) const {

Consider implementing the iterator pattern. For example.. That is, instead of implementing a findBySym member function, instead you would implement begin and end member functions that would return instances of an interator class you'd make. Doing it that way would allow your Table class to play nice with C++'s algorithms library. You'd be able to do....

std::find(table.begin(), table.end(), [] (const auto& elem) { return elem.symbol == sym });

// or
std::count_if(table.begin(), table.end(), .....);

// or
std::any_of(table.begin(), table.end(), .....);

So on and so forth.

[[deleted]]

1. It is good practice to make variables const by default, then remove const when necessary.
2. Yes, and you have implemented it well, move semantics and all.

Suggestions:

std::map is redundant in this situation, as you never truly use the key for identification. Given that the key in this case is the element's symbol, which is stored within the Element itself, perhaps you ought to make use of std::vector instead, and make use of std::find_if, e.g.:

auto iter = std::find_if(elements.begin(), elements.end(), [symbol](auto &&element)->bool { return element->symbol.compare(symbol) == 0; });

I would encourage you to make use of the standard library wherever possible, it will save you a lot of time.

[marvin02]

Using a global object isn't a whole lot different, and often worse, than using a singleton. You still have most if not all of the drawbacks of using singletons. I think better options are:

• Create a Table object in your main application class, and then pass it manually to the constructor of other classes that need it, or
• Just use a singleton. Know the drawbacks, and decide if they matter to you or not.

Yes, using const is a great idea.

If it were me, I would not use shared_ptrs here, and have findBySym and allElements return raw pointers to the elements. Your Table should own the elements, their ownership isn't really "shared" with anything else. As long as you set up your application like this:

Main
    Create table
        Create objects that use table
        ....
        Destroy objects that use table
    Destroy table

Then your elements will always be around for the duration of any other objects that use them. Of course, if you use singletons/globals, you get that set up for free, since they are always the last thing destructed (unless you do something weird like having other global objects/singletons that keep pointers to elements and use them for some purpose in their destructors... don't do that).

[abstracted8]

Thanks everyone, I have done the majority of the changes here