0
\$\begingroup\$

File - DatabaseOperations.h

This file contain classes representing database operations for three different types of databases (relational, document based and graph based) like establishing connections, executing statements, and committing and closing the connections.

There are four abstract classes with one pure virtual functions each.

  1. The class DatabaseConnection contains pure virtual function connect() - this class is inherited by three concrete classes representing three different types of databases each one of those implement the connect() function.

  2. The class ExecuteStatements contains one pure virtual function Execute() - this class is again inherited by three concrete classes representing three different types of databases and each one of those implement the Execute() function.

  3. The class CommitDisconnect contains one pure virtual function commit() - this class is yet again inherited by three concrete classes representing three different types of databases and each one of those implement the commit() function.

  4. The class DatabaseOperation which aggregates all the operations and contains three pure virtual functions, connect, execute and commit. All these three virtual functions are implemented in three respective classes.

#include <iostream>
#include <string>

/****************************************************************
* Connection
*****************************************************************/
class DatabaseConnection
{
public:
    virtual void connect() = 0;
};

class RelationalDatabaseConnection : public DatabaseConnection
{
public:
    void connect() override
    {
        std::cout << "Connection to RDBMS\n";
    }
};

class DocumentDatabaseConnection : public DatabaseConnection
{
public:
    void connect() override
    {
        std::cout << "Connection to Document DBMS\n";
    }
};

class GraphDatabaseConnection : public DatabaseConnection
{
public:
    void connect() override
    {
        std::cout << "Connection to Graph DBMS\n";
    }
};

/****************************************************************
* Execute Statements
*****************************************************************/


class ExecuteStatements
{
public:
    virtual void Execute() = 0;
};

class RelationalDatabaseExecute : public ExecuteStatements
{
public:
    void Execute() override
    {
        std::cout << "Executing ANSI SQL\n";
    }
};

class DocumentDatabaseExecute : public ExecuteStatements
{
public:
    void Execute() override
    {
        std::cout << "Executing JSON/Parquet etc.\n";
    }
};

class GraphDatabaseExecute : public ExecuteStatements
{
public:
    void Execute() override
    {
        std::cout << "Creating Nodes and Edges\n";
    }
};

/****************************************************************
* Commit & Disconnection
*****************************************************************/
class CommitDisconnect
{
public:
    virtual void commit() = 0;
};

class RelationalCommitDisconnect : public CommitDisconnect
{
public:
    void commit() override
    {
        std::cout << "Committing and closing RDBMS connection\n";
    }
};

class DocumentCommitDisconnect : public CommitDisconnect
{
public:
    void commit() override
    {
        std::cout << "Committing and closing Document DBMS connection\n";
    }
};

class GraphCommitDisconnect : public CommitDisconnect
{
public:
    void commit() override
    {
        std::cout << "Committing and closing Graph DBMS connection\n";
    }
};

class DatabaseOperation
{
public:
    virtual DatabaseConnection* connect() = 0;
    virtual ExecuteStatements* execute() = 0;
    virtual CommitDisconnect* commit() = 0;
};

class Relational : public DatabaseOperation
{
public:
    DatabaseConnection* connect() override
    {
        return new RelationalDatabaseConnection();
    }
    ExecuteStatements* execute() override
    {
        return new RelationalDatabaseExecute();
    }
    CommitDisconnect* commit() override
    {
        return new RelationalCommitDisconnect();
    }
};
class Document : public DatabaseOperation
{
public:
    DatabaseConnection* connect() override
    {
        return new DocumentDatabaseConnection();
    }
    ExecuteStatements* execute() override
    {
        return new DocumentDatabaseExecute();
    }
    CommitDisconnect* commit() override
    {
        return new DocumentCommitDisconnect();
    }
};
class Graph : public DatabaseOperation
{
public:
    DatabaseConnection* connect() override
    {
        return new GraphDatabaseConnection();
    }
    ExecuteStatements* execute() override
    {
        return new GraphDatabaseExecute();
    }
    CommitDisconnect* commit() override
    {
        return new GraphCommitDisconnect();
    }
};

file - main.cxx

#include <iostream>
#include "DatabaseOperations.h"

int main()
{
    int value = -1;
    DatabaseOperation* operation = nullptr;
    std::cout << "Relational: 1, Document: 2 and Graph: 3\n" << "Enter: ";
    std::cin >> value;
    switch (value)
    {
    case 1:
        operation = new Relational();
        break;
    case 2:
        operation = new Document();
        break;
    case 3:
        operation = new Graph();
    default:
        break;
    }

    if (operation != nullptr)
    {
        operation->connect()->connect();
        operation->execute()->Execute();
        operation->commit()->commit();
    }
    return 0;
}

Does the above code implements the Abstract Factory design pattern correctly?

\$\endgroup\$
1

1 Answer 1

3
\$\begingroup\$

Does the above code implements the Abstract Factory design pattern correctly?

No.

First, it is wildly overcomplicated. It seems ridiculous to need four different class hierarchies just to do a database transaction.

Second, it is just plain bad C++, because it leaks memory all over the place. You use new like it’s going out of style (which it is), but you never once use delete.

And third, and perhaps most importantly, you don’t actually make a factory. A factory is kinda important if you want to implement the abstract factory pattern.

The abstract factory pattern only requires a single abstract base, and a single factory (which is a function). For your case, it might look something like this:

// database.hpp ================================================

// abstract base
class database
{
public:
    virtual ~database() = default;

    virtual auto connect() -> void = 0;
    virtual auto execute() -> void = 0;
    virtual auto commit() -> void = 0;
};

// factory
[[nodiscard]] auto create_database(int id) -> std::unique_ptr<database>;

// database.cpp ================================================

#include "database.hpp"

// concrete class
//
// actual implementation could be in another file, but declaration must be
// visible to the factory function
class relational_database : public database
{
public:
    auto connect() -> void override { std::cout << "Connection to RDBMS\n"; }
    auto execute() -> void override { std::cout << "Executing ANSI SQL\n"; }
    auto commit() -> void override { std::cout << "Committing and closing RDBMS connection\n"; }
};

// and the same thing for the other database types

// factory implementation
[[nodiscard]] auto create_database(int id) -> std::unique_ptr<database>
{
    switch (id)
    {
    case 1:
        return std::unique_ptr{new relational_database{}};
    case 2:
        return std::unique_ptr{new document_database{}};
    case 3:
        return std::unique_ptr{new graph_database{}};
    default:
        // should really throw an exception here, but whatevs
        return nullptr;
    }
}

// main.cpp ====================================================

#include "database.hpp"

// usage in main()
auto main() -> int
{
    std::cout << "Relational: 1, Document: 2 and Graph: 3\n" << "Enter: ";

    auto input = -1;
    std::cin >> input;

    if (auto operation = create_database(input); operation != nullptr)
    {
        operation->connect();
        operation->execute();
        operation->commit();
    }
}

To use the factory, you only need to be able to see the abstract base (database), and the factory (create_database()). All of the concrete classes could be hidden (though, of course, they need to be visible to the factory.

Now as for some specific problems in the code…

I don’t see any sense in breaking the connect, execute, and commit/close functions into three different class hierarchies. All of these functions are tightly connected: the mechanism for connecting to a database is intimately connected to the mechanism for closing that connection. It makes no sense at all for those two things to be in two entirely different classes. And to actually do anything with the database, you need the connection handle… so how are you supposed to get the connection handle opened in one class… over to the execute class… then finally over to the commit/close class. That whole mess is just bonkers.

class Relational : public DatabaseOperation
{
public:
    DatabaseConnection* connect() override
    {
        return new RelationalDatabaseConnection();
    }

No. Just no. We don’t use raw pointers for ownership in C++ anymore. That was bad practice even back in C++98 (hence, std::auto_ptr). This is just plain unacceptable in modern C++.

But the real problem here is that your entire interface is just terrible. Your base class looks like this:

class DatabaseOperation
{
public:
    virtual DatabaseConnection* connect() = 0;
    virtual ExecuteStatements* execute() = 0;
    virtual CommitDisconnect* commit() = 0;
};

In order to implement this interface correctly you would have to do something like this:

class CorrectDatabase : public DatabaseOperation
{
    std::unique_ptr<DatabaseConnection> _connection;
    std::unique_ptr<ExecuteStatements> _statements;
    std::unique_ptr<CommitDisconnect> _commit;

public:
    // note we have to do all the allocations in the constructor, and hold
    // the pointers as data members
    CorrectDatabase()
        : _connection{new RelationalDatabaseConnection}
        , _statements{new RelationalDatabaseExecute}
        , _commit{new RelationalCommitDisconnect}
    {}

    DatabaseConnection* connect() override { return _connection.get(); }
    ExecuteStatements* execute() override { return _statements.get(); }
    CommitDisconnect* commit() override { return _commit.get(); }
};

The way you tried to do it—allocating each class in the actual function—is not only clunky, it is wrong, because there is no possible way to do that without leaking memory.

But while the code above is now correct, it’s still terrible. Because to use it, I have to do this:

auto db = CorrectDatabase{};

db.connect()->connect();
db.execute()->execute();
db.commit()->commit();

That’s just silly. Why not:

auto db = CorrectDatabase{};

db.connect();
db.execute();
db.commit();

That looks a lot more reasonable. And all you’d have to do is this:

class DatabaseOperation
{
public:
    virtual void connect() = 0;
    virtual void execute() = 0;
    virtual void commit() = 0;
};

class CorrectDatabase : public FixedDatabaseOperation
{
    std::unique_ptr<DatabaseConnection> _connection;
    std::unique_ptr<ExecuteStatements> _statements;
    std::unique_ptr<CommitDisconnect> _commit;

public:
    // note we have to do all the allocations in the constructor, and hold
    // the pointers as data members
    CorrectDatabase()
        : _connection{new RelationalDatabaseConnection}
        , _statements{new RelationalDatabaseExecute}
        , _commit{new RelationalCommitDisconnect}
    {}

    void connect() override { return _connection->connect(); }
    void execute() override { return _statements->execute(); }
    void commit() override { return _commit->commit(); }
};

But of course, this is still terrible design. What happens if someone forgets to call connect() before they call execute()? What happens if they forget to call commit() after?

A good C++ interface is easy to use, and hard to use incorrectly. For example:

auto db = create_database(database_type); // the factory function

// the database is *already* connected to when it was constructed, so you can
// just go ahead and use it

db->execute(statement_1);
db->execute(statement_2);
db->execute(statement_3);

// you can manually commit, if you want

db->commit(); // commits the transaction consisting of the previous 3 statements

// more statements

db->execute(statement_4);
db->execute(statement_5);

// you don't need to manually commit, the destructor will commit automatically

// the destructor will also close the connection

There is no way to use that kind of interface wrong. That’s what good C++ looks like. (You could go even further and have RAII transaction objects, that will auto-commit in smaller chunks.)

The bottom line:

  1. No, you have not implemented the factory pattern… mostly because you didn’t actually implement a factory.
  2. Your code is just plain wrong because it leaks memory all over the place.
  3. You have wildly over-engineered the whole thing, to the point of absurdity. I don’t even see how it could possibly work in practice. Try it with a real database, and see if you can actually get it to work.

I would suggest throwing the whole thing out, and starting from scratch with a much simpler design. I would suggest starting with the absolute basic abstract factory pattern, if that’s what you’re interested in (an example of the absolute basic abstract factory is below). I would also suggest trying it out with a real database (just use SQLite as a test database, for example), to see what’s practical, because what seems to make sense in the abstract just won’t work in reality. (For example, how the hell are you going to handle the connection handle across three distinct class hierarchies?)

Example of absolute basic abstract factory:

// thing.hpp ===================================================

// the only header you need to include to use the factory

#include <memory>
#include <string_view>

// abstract base
class thing_base
{
public:
    virtual ~thing_base() = default;

    virtual void func() = 0;
};

// factory declaration
[[nodiscard]] auto create_thing(std::string_view id) -> std::unique_ptr<thing_base>;

// thing.cpp ===================================================

#include "thing.hpp"

#include "thing_concrete.hpp"

// factory implementation
[[nodiscard]] auto create_thing(std::string_view id) -> std::unique_ptr<thing_base>
{
    if (id = "thing_concrete")
        return std::unique_ptr{new thing_concrete};
    // handle error for unknown ID
}

// thing_concrete.hpp ==========================================

// example concrete type, can be a private header

#include "thing.hpp"

class thing_concrete : public thing
{
public:
    void func() override;
};

// thing_concrete.cpp ==========================================

#include "thing_concrete.hpp"

void thing_concrete::func() override
{
    // ...
}

// main.cpp ====================================================

// usage

#include "thing.hpp"

auto main() -> int
{
    auto t = create_thing("thing_concrete");
}
```
\$\endgroup\$
3
  • \$\begingroup\$ "like it’s going out of style (which it is)," - I like what you did there! \$\endgroup\$ Aug 3, 2021 at 7:30
  • \$\begingroup\$ Thanks for pointing these problems out. However, I was just trying to implement the design pattern so did not care about the memory leaks, but now when I come to think of it a good design must take care of those as well. Much Thanks. \$\endgroup\$ Aug 4, 2021 at 2:37
  • \$\begingroup\$ what if there are changes in connect()/execute()/commit() for any data base type. In original design, database would not be changed in this case. A database was using connection, statement executer, and committer were member variable. Any change in these 3 would not have impacted database operation or database class. \$\endgroup\$ May 9, 2022 at 18:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.