8
\$\begingroup\$

I have an image class and a table class. To each image a single table can be "attached". Each <image, table> pair should be identified with an ID, which can later be used to get the pointer of image or table associated with that ID. Images in this "map" should be unique. Below is my solution, please let me know if it can be improved. Thanks.

browserInfo.h

#include <vector>
#include <tuple>

class BrowserInfo
{
public:
    // Returns a unique ID for the <image, table> pair
    /*
        @param  img The image file pointer
        @param  tbl     The table view pointer
        @return         A unique ID for the input pair
    */
    unsigned getId(image *img, table *tbl) const;

    /// Returns a image pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to image file if ID exists, otherwise nullptr
    */
    image *getImage(unsigned uId) const;

    /// Returns a table pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to table if ID exists, otherwise nullptr
    */
    table *getTable(unsigned uId) const;

private:
    // alias for the type
    using imageTableToId = std::tuple<image *, table *, unsigned>;

    // This vector keeps track of all unique <image, table> pair IDs
    mutable std::vector<imageTableToId> m_vecImageTableIds;

    // The current ID
    mutable unsigned m_iCurrentId = 0;

}; // class BrowserInfo

browserInfo.cpp

unsigned BrowserInfo::getId(image *img, table *tbl) const
{
    // first, try to see if we have worked with the provided image before
    for (auto &tuple : m_vecImageTableIds)
    {
        if (std::get<0>(tuple) == img)
        {
            // we support a single table view for each image.
            // therefore, if we find that the image is already stored
            // in our vector, we just need to update the corresponding
            // table pointer and return a new unique ID for this pair
            std::get<1>(tuple) = tbl;
            std::get<2>(tuple) = ++m_iCurrentId;

            return m_iCurrentId;
        }
    }

    // if we got here it means the image pointer wasn't stored before
    // so we can just insert a new tuple into the vector
    m_vecImageTableIds.push_back(std::make_tuple(img, tbl, ++m_iCurrentId));
    return m_iCurrentId;
}

image *BrowserInfo::getImage(const unsigned uId) const
{
    for (const auto &tuple : m_vecImageTableIds)
    {
        if (std::get<2>(tuple) == uId)
            return std::get<0>(tuple);
    }

    return nullptr;
}

table *BrowserInfo::getTable(const unsigned uId) const
{
    for (const auto &tuple : m_vecImageTableIds)
    {
        if (std::get<2>(tuple) == uId)
            return std::get<1>(tuple);
    }

    return nullptr;
}
\$\endgroup\$
  • \$\begingroup\$ Why do you need a numeric ID here instead of modelling the relationship directly in code? Using numeric IDs for such things is virtually always a mistake (I’d call it an anti-pattern), unless you need to communicate the ID to an outside API that doesn’t know about your objects. \$\endgroup\$ – Konrad Rudolph May 27 at 10:49
  • \$\begingroup\$ basically, that's what i need. to determine these objects by the ID by an outside API. \$\endgroup\$ – user3132457 May 27 at 11:39
7
\$\begingroup\$

While your solution works, it can be made even more understandable.

Instead of using a vector of tuples, consider using an std::map:

std::map<unsigned, std::tuple<image *, table *>> imageTablePairs;

This is the most natural expression of the problem statement: mapping an ID to an image-table pair. It will also simplify the logic of your getId function. Here's an algorithm for what you need to do:

Cycle through the map and check if the image exists. If it does, delete its record from the map (using std::map::erase).

Then, simply do:

imageTablePairs[++m_iCurrentId] = std::make_tuple(img, tbl);

This covers both the case when the image exists (in which case its old record gets deleted per the algorithm above) and the case when the image does not exist (in which case we simply make a new record).

For the other two getter functions, we obviously can't assume that the ID being passed in exists in the map, so we can create a private helper function that takes an ID and returns true if it exists and false otherwise. Then, the logic becomes:

  • getImage: if the ID exists, return std::get<0>(imageTablePairs[uId]).

  • getTable: if the ID exists, return std::get<1>(imageTablePairs[uId]).


If I misunderstood the problem statement and this solution is not possible, please let me know.


Edit: Here's the code I'd use. Tested in Visual Studio 2017 and confirmed that it compiles and runs as expected (I used empty image and table structs for testing).

browserInfo.h

#pragma once
#include <map>
#include <tuple>

class BrowserInfo
{
public:
    // Returns a unique ID for the <image, table> pair
    /*
        @param  img The image file pointer
        @param  tbl     The table view pointer
        @return         A unique ID for the input pair
    */
    unsigned getId(image *img, table *tbl) const;

    /// Returns a image pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to image file if ID exists, otherwise nullptr
    */
    image *getImage(unsigned uId) const;

    /// Returns a table pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to table if ID exists, otherwise nullptr
    */
    table *getTable(unsigned uId) const;

private:

    bool idExists(unsigned uId) const;

    mutable std::map<unsigned, std::tuple<image *, table *>> imageTablePairs;

    // The current ID
    mutable unsigned m_iCurrentId = 0;

}; // class BrowserInfo

browserInfo.cpp

#include "browserInfo.h"


unsigned BrowserInfo::getId(image * img, table * tbl) const
{
    for (auto &record : imageTablePairs)
    {
        if (std::get<0>(record.second) == img)
        {
            imageTablePairs.erase(record.first);
            break;
        }
    }

    imageTablePairs[++m_iCurrentId] = std::make_tuple(img, tbl);
    return m_iCurrentId;
}

image * BrowserInfo::getImage(unsigned uId) const
{
    if (idExists(uId))
    {
        return std::get<0>(imageTablePairs[uId]);
    }

    return nullptr;
}

table * BrowserInfo::getTable(unsigned uId) const
{
    if (idExists(uId))
    {
        return std::get<1>(imageTablePairs[uId]);
    }

    return nullptr;
}

bool BrowserInfo::idExists(unsigned uId) const
{
    std::map<unsigned, std::tuple<image*, table*>>::iterator it = imageTablePairs.find(uId);
    return it != imageTablePairs.end();
}

\$\endgroup\$
  • \$\begingroup\$ this is better, thanks! i was also worried about the two members being mutable. what about that? \$\endgroup\$ – user3132457 May 26 at 20:19
  • \$\begingroup\$ It's fine, but it suggests a design flaw. The "getter" methods may need to be renamed and their const-ness revoked (if possible), because otherwise they don't really enforce it on anything. \$\endgroup\$ – AleksandrH May 26 at 20:30
  • \$\begingroup\$ i don't feel like the constness needs to be revoked; to the user, they don't modify the class state so they need to be const \$\endgroup\$ – user3132457 May 26 at 20:45
  • \$\begingroup\$ It's up to you, but I figured the skeleton of the BrowserInfo class had been set up for you and you were filling in the .cpp, so I left that as is. The truth is that having const and mutable at the same time makes const obsolete. When applied to a method, const ensures that compilation will fail if a method is attempting to modify internal data. Mutable, on the other hand, overrides this protection. In this case, since the only data in your class is marked mutable, it doesn't make sense to have const methods. Additionally, I would consider changing the names—getters don't usually modify data. \$\endgroup\$ – AleksandrH May 26 at 22:27
  • \$\begingroup\$ another option i was thinking over is to have one method to insert and another one to get. in that case everything falls into its place. \$\endgroup\$ – user3132457 May 27 at 3:10
6
\$\begingroup\$

As every id identifies a tuple of unique image, optional table, why over-complicate things?

Select one alternative from here:

  1. Add a table* to image.
  2. Use a std::map or std::unordered_map from image* to table*.

And one from here:

  1. Make the ids image*s.
  2. Add a (potentially optional) id to image. Just ensure that you can search them by id.
  3. Use a map from arbitrary id to image* (and optionally in reverse).

Presto, you are done, and this answer will be far longer and more complicated than the solution.

As an alternative, if you want multiple indices into the same collection, look at Boost.MultiIndex.

\$\endgroup\$
  • \$\begingroup\$ from oop point of view, image and table are unrelated things so i can't tie them like that. hence, i achieve decoupling with the introduction of this layer between them. \$\endgroup\$ – user3132457 May 27 at 3:04
  • 3
    \$\begingroup\$ @user3132457 No, they are clearly related, otherwise you wouldn’t have asked this question. OOP models code objects, not real-world objects. And the objects are related in your code’s model. \$\endgroup\$ – Konrad Rudolph May 27 at 10:51

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.