Using builder pattern and facade pattern in real project

Question

The code below is used to upgrade the firmware of three different types of cameras.

I am not good at design patterns. I wonder if it is suitable to use builder pattern and facade pattern here.

Since the constructor needs many parameters, I think it's better to use builder pattern to set the property step by step. Is the factory method pattern better here since all the steps should be set?

Since there are many class whose relation is a little complex to the user, I think the facad class could make the user easier to master it.

#include <string>
#include <iostream>
#include <fstream>
#include <map>
#include <vector>
#include <array>
#include <thread>
#include <type_traits>
#include <stdexcept>
#include <errno.h>
#include <memory>
#define LOG_FUNC_IS_CALLED std::cout << __PRETTY_FUNCTION__ << " has been called" << std::endl;
class I2c_manager
{
public:
 I2c_manager(std::string dev_name, unsigned int slave_addr) : m_dev_name(dev_name), m_slave_addr(slave_addr){};
 I2c_manager(const I2c_manager&) = delete;
 I2c_manager& operator=(const I2c_manager&) = delete;
 
 int init(){
 LOG_FUNC_IS_CALLED
 return 0;
 }
 
 int write(uint16_t reg, char* data, int len){
 LOG_FUNC_IS_CALLED
 //do some work with fd
 return 0;
 }
 
 int read(uint16_t reg, char* data, int len){
 LOG_FUNC_IS_CALLED
 //do some work with fd
 return 0;
 }
private:
 int m_fd;
 const std::string m_dev_name;
 const unsigned int m_slave_addr;
};
//similar to a single-pole three throw switch
class ModuleSwitch 
{
public:
 ModuleSwitch(I2c_manager&);
 int switch2front();
 int switch2middle();
 int switch2back();
 int create_nodes();
private:
 I2c_manager& m_i2c_ops;
 uint8_t enb_status;
};
ModuleSwitch::ModuleSwitch(I2c_manager& i2c_ops):m_i2c_ops(i2c_ops),enb_status(0){}
int ModuleSwitch::create_nodes()
{
 //make the i2c fd of cameras could be used by the programmer
 LOG_FUNC_IS_CALLED
 return 0;
}
int ModuleSwitch::switch2front()
{
 //if enabled, return. otherwise, do some work with m_i2c_ops;
 LOG_FUNC_IS_CALLED
 return 0;
}
int ModuleSwitch::switch2middle()
{
 //if enabled, return. otherwise, do some work with m_i2c_ops;
 LOG_FUNC_IS_CALLED
 return 0;
}
int ModuleSwitch::switch2back()
{
 //if enabled, return. otherwise, do some work with m_i2c_ops;
 LOG_FUNC_IS_CALLED
 return 0;
}
class CamUpgrade
{
public:
 virtual int upgrade(const std::vector<uint8_t>& vec_firmware, uint32_t blk_size) const =0;
 virtual ~CamUpgrade() = default;
};
class FhUpgrade final: public CamUpgrade
{
public:
 FhUpgrade(ModuleSwitch& mod_swt, I2c_manager& i2c_mgr);
 int upgrade(const std::vector<uint8_t>& vec_firmware, uint32_t blk_size=4096) const override;
 ~FhUpgrade() = default;
public:
 ModuleSwitch& m_mod_swt;
 I2c_manager& m_i2c_ops;
 //omit some member variables
};
FhUpgrade::FhUpgrade(ModuleSwitch& mod_swt, I2c_manager& i2c_mgr): m_mod_swt(mod_swt), m_i2c_ops(i2c_mgr){
 LOG_FUNC_IS_CALLED
}
int FhUpgrade::upgrade(const std::vector<uint8_t>& vec_firmware, uint32_t blk_size) const
{
 //do some work with m_i2c_ops and m_mod_swt;
 LOG_FUNC_IS_CALLED
 return 0;
}
class SC_Upgrade : public CamUpgrade
{
protected:
 SC_Upgrade(ModuleSwitch& mod_swt, I2c_manager& sensor_ops, I2c_manager& serdes_ops, I2c_manager& eeprom_ops);
 ~SC_Upgrade() override = default;
private:
 int upgrade(const std::vector<uint8_t>& vec_firmware, uint32_t blk_size = 16) const override;
 ModuleSwitch& m_mod_swt;
 I2c_manager& m_sensor_ops;
 I2c_manager& m_serdes_ops;
 I2c_manager& m_eeprom_ops;
 
 std::map<std::string, std::vector<std::pair<uint16_t, uint8_t>>> m_to_write_dat;
};
SC_Upgrade::SC_Upgrade(ModuleSwitch& mod_swt, I2c_manager& sensor_ops, I2c_manager& serdes_ops, I2c_manager& eeprom_ops):m_mod_swt(mod_swt), m_sensor_ops(sensor_ops), m_serdes_ops(serdes_ops), m_eeprom_ops(eeprom_ops){
//init common part of m_to_write_dat;
}
int SC_Upgrade::upgrade(const std::vector<uint8_t>& vec_firmware, uint32_t blk_size) const
{
 LOG_FUNC_IS_CALLED
 return 0;
}
class Sc220Upgrade final : public SC_Upgrade //back oms
{
public:
 Sc220Upgrade(ModuleSwitch& mod_swt, I2c_manager& sensor_ops, I2c_manager& serdes_ops, I2c_manager& eeprom_ops);
 ~Sc220Upgrade() override = default;
};
Sc220Upgrade::Sc220Upgrade(ModuleSwitch& mod_swt, I2c_manager& sensor_ops, I2c_manager& serdes_ops, I2c_manager& eeprom_ops) : SC_Upgrade(mod_swt, sensor_ops, serdes_ops, eeprom_ops)
{
 LOG_FUNC_IS_CALLED
 //insert item for m_to_write_dat;
}
class Sc232Upgrade final : public SC_Upgrade //DMS
{
public:
 Sc232Upgrade(ModuleSwitch& mod_swt, I2c_manager& sensor_ops, I2c_manager& serdes_ops, I2c_manager& eeprom_ops);
 ~Sc232Upgrade() override = default;
};
Sc232Upgrade::Sc232Upgrade(ModuleSwitch& mod_swt, I2c_manager& sensor_ops, I2c_manager& serdes_ops, I2c_manager& eeprom_ops) : SC_Upgrade(mod_swt, sensor_ops, serdes_ops, eeprom_ops)
{
 LOG_FUNC_IS_CALLED
 //insert item for m_to_write_dat;
}
class FmwUpgBuilder
{
public:
 virtual std::unique_ptr<CamUpgrade> generate() const = 0;
 virtual ~FmwUpgBuilder() = default;
 FmwUpgBuilder(const FmwUpgBuilder&) = default;
 FmwUpgBuilder& operator=(const FmwUpgBuilder&) = default;
 void set_mod_swt(ModuleSwitch&);
 void set_sensor_i2c_ops(I2c_manager&);
protected:
 FmwUpgBuilder() = default;
 ModuleSwitch* m_mod_swt;
 I2c_manager* m_sensor_ops;
};
void FmwUpgBuilder::set_mod_swt(ModuleSwitch& mod_swt)
{
 m_mod_swt = &mod_swt;
}
void FmwUpgBuilder::set_sensor_i2c_ops(I2c_manager& i2c_ops)
{
 m_sensor_ops = &i2c_ops;
}
class FhUpgBuilder : public FmwUpgBuilder
{
public:
 FhUpgBuilder() = default;
 FhUpgBuilder(const FhUpgBuilder&);
 FmwUpgBuilder& operator=(const FhUpgBuilder&);
 ~FhUpgBuilder() override = default;
 std::unique_ptr<CamUpgrade> generate() const override;
};
FhUpgBuilder::FhUpgBuilder(const FhUpgBuilder& other):FmwUpgBuilder(other){};
FmwUpgBuilder& FhUpgBuilder::operator=(const FhUpgBuilder& other)
{
 if(this != &other)
 {
 FmwUpgBuilder::operator=(other);
 }
 return *this;
}
std::unique_ptr<CamUpgrade> FhUpgBuilder::generate() const
{
 return std::unique_ptr<FhUpgrade>(new FhUpgrade(*m_mod_swt, *m_sensor_ops));
}
class ScUpgBuilder : public FmwUpgBuilder
{
public:
 ~ScUpgBuilder() override = default;
 ScUpgBuilder(const ScUpgBuilder& other);
 ScUpgBuilder& operator=(const ScUpgBuilder& other);
 void set_serdes_i2c_ops(I2c_manager&);
 void set_eeprom_i2c_ops(I2c_manager&);
protected:
 ScUpgBuilder() = default;
 I2c_manager* m_serdes_ops;
 I2c_manager* m_eeprom_ops;
};
ScUpgBuilder::ScUpgBuilder(const ScUpgBuilder& other) : FmwUpgBuilder(other){}
ScUpgBuilder& ScUpgBuilder::operator=(const ScUpgBuilder& other)
{ 
 if(this != &other)
 {
 FmwUpgBuilder::operator=(other);
 m_serdes_ops = other.m_serdes_ops;
 m_eeprom_ops = other.m_eeprom_ops;
 }
 return *this;
}
void ScUpgBuilder::set_serdes_i2c_ops(I2c_manager& serdes_ops)
{
 m_serdes_ops = &serdes_ops;
}
void ScUpgBuilder::set_eeprom_i2c_ops(I2c_manager& eeprom_ops)
{
 m_eeprom_ops = &eeprom_ops;
}
class Sc220UpgBuilder : public ScUpgBuilder
{
public:
 Sc220UpgBuilder() = default;
 Sc220UpgBuilder(const Sc220UpgBuilder& other);
 Sc220UpgBuilder& operator=(const Sc220UpgBuilder& other);
 std::unique_ptr<CamUpgrade> generate() const override;
}; 
Sc220UpgBuilder::Sc220UpgBuilder(const Sc220UpgBuilder& other) : ScUpgBuilder(other){}
Sc220UpgBuilder& Sc220UpgBuilder::operator=(const Sc220UpgBuilder& other)
{
 if(this != &other)
 {
 ScUpgBuilder::operator=(other);
 }
 return *this;
}
std::unique_ptr<CamUpgrade> Sc220UpgBuilder::generate() const
{
 return std::unique_ptr<Sc220Upgrade>(new Sc220Upgrade(*m_mod_swt, *m_sensor_ops, *m_serdes_ops, *m_eeprom_ops));
}
class Sc232UpgBuilder : public ScUpgBuilder
{
public:
 Sc232UpgBuilder() = default;
 Sc232UpgBuilder(const Sc232UpgBuilder& other); 
 Sc232UpgBuilder& operator=(const Sc232UpgBuilder& other); 
 std::unique_ptr<CamUpgrade> generate() const override;
}; 
Sc232UpgBuilder::Sc232UpgBuilder(const Sc232UpgBuilder& other) : ScUpgBuilder(other){}
Sc232UpgBuilder& Sc232UpgBuilder::operator=(const Sc232UpgBuilder& other)
{
 if(this != &other)
 {
 ScUpgBuilder::operator=(other);
 }
 return *this; 
}
std::unique_ptr<CamUpgrade> Sc232UpgBuilder::generate() const
{
 return std::unique_ptr<Sc232Upgrade>(new Sc232Upgrade(*m_mod_swt, *m_sensor_ops, *m_serdes_ops, *m_eeprom_ops));
}
//facade
class UpgFcd 
{
public:
 using i2c_addr_type = uint8_t;
 UpgFcd();
 
 int upgrade(const std::string& type, const std::vector<uint8_t>& vec_firmware, uint32_t blk_size) const;
 
 int init();
private:
 std::unique_ptr<CamUpgrade> m_fh_ugd;
 std::unique_ptr<CamUpgrade> m_sc220_ugd;
 std::unique_ptr<CamUpgrade> m_sc232_ugd;
 std::unique_ptr<ModuleSwitch> m_mod_swt;
 std::unique_ptr<I2c_manager> m_mod_swt_ops;
 std::unique_ptr<I2c_manager> m_fh_sensor_ops;
 std::unique_ptr<I2c_manager> m_sc_sensor_ops;
 std::unique_ptr<I2c_manager> m_sc_serdes_ops;
 std::unique_ptr<I2c_manager> m_sc220_eeprom_ops;
 std::unique_ptr<I2c_manager> m_sc232_eeprom_ops;
 std::vector<std::pair<std::string, i2c_addr_type>> m_i2c_addr_map;
};
UpgFcd::UpgFcd(){
 //init m_i2c_addr_map
}
int UpgFcd::init()
{
 for(const auto& pair:m_i2c_addr_map)
 {
 std::unique_ptr<I2c_manager> i2c_ops_ptr(new I2c_manager(pair.first, pair.second));
 if(i2c_ops_ptr->init() != 0)
 {
 return -1;
 }
 else
 {
 if(pair.first == "mod_swt")
 {
 m_mod_swt_ops = std::move(i2c_ops_ptr);
 m_mod_swt = std::unique_ptr<ModuleSwitch>(new ModuleSwitch(*m_mod_swt_ops));
 if(0 != m_mod_swt->create_nodes())
 {
 return -2;
 }
 }
 else if(pair.first == "fh_sensor")
 {
 m_fh_sensor_ops = std::move(i2c_ops_ptr);
 }
 else if(pair.first == "sc_sensor")
 {
 m_sc_sensor_ops = std::move(i2c_ops_ptr);
 }
 else if(pair.first == "sc_serdes")
 {
 m_sc_serdes_ops = std::move(i2c_ops_ptr);
 }
 else if(pair.first == "sc220_eeprom")
 {
 m_sc220_eeprom_ops = std::move(i2c_ops_ptr);
 }
 else if(pair.first == "sc232_eeprom")
 {
 m_sc232_eeprom_ops = std::move(i2c_ops_ptr);
 }
 else
 {
 return -3;
 }
 }
 }
 FhUpgBuilder fh_upg_builder;
 fh_upg_builder.set_mod_swt(*m_mod_swt);
 fh_upg_builder.set_sensor_i2c_ops(*m_fh_sensor_ops);
 m_fh_ugd = fh_upg_builder.generate();
 Sc220UpgBuilder sc220_upg_builder;
 sc220_upg_builder.set_mod_swt(*m_mod_swt);
 sc220_upg_builder.set_sensor_i2c_ops(*m_sc_sensor_ops);
 sc220_upg_builder.set_eeprom_i2c_ops(*m_sc220_eeprom_ops); 
 sc220_upg_builder.set_serdes_i2c_ops(*m_sc_serdes_ops);
 m_sc220_ugd = sc220_upg_builder.generate();
 Sc232UpgBuilder sc232_upg_builder;
 sc232_upg_builder.set_mod_swt(*m_mod_swt);
 sc232_upg_builder.set_sensor_i2c_ops(*m_sc_sensor_ops);
 sc232_upg_builder.set_eeprom_i2c_ops(*m_sc220_eeprom_ops); 
 sc232_upg_builder.set_serdes_i2c_ops(*m_sc_serdes_ops);
 m_sc232_ugd = sc232_upg_builder.generate();
 return 0;
}
int UpgFcd::upgrade(const std::string& type, const std::vector<uint8_t>& vec_firmware, uint32_t blk_size) const
{
 int ret = 0;
 if(type == "front")
 {
 if(!m_fh_ugd)
 {
 ret = -1;
 }
 else
 {
 if(0 != m_fh_ugd->upgrade(vec_firmware, blk_size))
 {
 ret = -2;
 }
 }
 }
 else if(type == "middle")
 {
 if(!m_sc232_ugd)
 {
 ret = -1;
 }
 else
 {
 if(m_sc232_ugd->upgrade(vec_firmware, blk_size) != 0)
 {
 ret = -2;
 }
 }
 }
 else if(type == "back")
 {
 if(!m_sc220_ugd)
 {
 ret = -1;
 }
 else
 {
 if(m_sc220_ugd->upgrade(vec_firmware, blk_size) != 0)
 {
 ret = -2;
 }
 }
 }
 else
 {
 ret = -3;
 }
 LOG_FUNC_IS_CALLED
 return ret;
}
int main()
{
 std::vector<uint8_t> fmw_data(1000);
 UpgFcd upg_fcd;
 if(upg_fcd.init()!=0)
 {
 std::cout << "faild to init" << std::endl;
 return -1;
 }
 
 upg_fcd.upgrade("front", fmw_data, 16);
}

Here is the ouput:

FhUpgrade::FhUpgrade(ModuleSwitch&, I2c_manager&) has been called
Sc220Upgrade::Sc220Upgrade(ModuleSwitch&, I2c_manager&, I2c_manager&, I2c_manager&) has been called
Sc232Upgrade::Sc232Upgrade(ModuleSwitch&, I2c_manager&, I2c_manager&, I2c_manager&) has been called
virtual int FhUpgrade::upgrade(const std::vector<unsigned char>&, uint32_t) const has been called
int UpgFcd::upgrade(const std::string&, const std::vector<unsigned char>&, uint32_t) const has been called

• Davislor: You very rarely want a factory method other than std::make_unique, whose return value you can assign to a (smart) pointer to an abstract base class. Such as: const std::unique_ptr<Interface> something = std::make_unique<ConcreteImplementation>(foo, bar);.
• John: @Davislor Sorry for my poor English. 1. I have read your comment several times to fully understand it. What you emphasise is that using std::unique_ptr<Interface> as the return type in the factory method pattern is a good & commonly seen practice. Am I right? If I am wrong. please let me know. 2. I have used std::unique_ptr<Interface> as in the builder pattern which could be seen in my question, too. It seems that both the aforementioned design patterns are commonly seen use it as the return type. Therefore, you can't make the choice between the two patterns solely based on it.
• Davislor: I can understand you just fine; don’t worry. 1. That’s correct. In some situations, you might want a std::shared_ptr instead. Since either of these deletes the object through a base-class pointer, it is very important that the base class has a virtual destructor, which you’ve done. I often make this protected:, so that the compiler will not allow the abstract base class to be instantiated, but will allow its derived classes to be.
• Davislor: 2. If you have Builder::generate(), std::unique_ptr<CamUpgrade> is the right return type for it. You actually don’t need a generate() function at all, though; you want a factory pattern when the implementation chooses a concrete implementation for the client, transparently, and the client only needs to know that it implements the interface. In that case, the factory always knows the context. If the client chooses the class, they should call make_unique or create a local object and pass it around by reference. I wouldn’t implement UpgFcd that way, if you need it at all.

Answer 1

When to use design patterns

I wonder if it is suitable to use builder pattern and facade pattern here.

The patterns from the book Design Patterns rely exclusively on object oriented features, and are often quite heavy handed if you follow them to the letter. Sometimes that is exactly what you need, but there are also problems where you can use generic programming or other programming paradigms, or where you can use a simpler form of a pattern.

Consider the builder pattern: yes, this can help create an object which otherwise needs lots of parameters to its constructor, but maybe the type of the object you want to create can just have set_*() functions, instead of having those in a separate builder type?

As for the façade pattern: it is so common to write a function or a class that acts as a simple interface for more complex things, almost no one will think of this as a specific design pattern. However, it makes sense in the context of a pure object oriented language where you can't even have free functions, and everything must be defined inside a class. In that case, the façade pattern just says to put the simple interface into a separate class, instead of overloading a more complex class with even more functions (which could lead to the god object antipattern).

Naming things

The components in each design pattern are given names in the book, but they are rather abstract names, and they don't add much value to real code. In particular, don't use Fcd or Facade in your program, just name it FirmwareUpgrader.

Avoid unnecessary abbreviations, but if you really want to abbreviate, at least be consistent. What does m_fh_ugd mean? Why is there ugd in the name of that variable, but upg in the name of the builder for it?

Return values

It seems like you use the C way of returning a status code, where 0 means success and negative numbers mean an error. But what is the difference between -1, -2 and -3? They mean different things for different functions, and I have to read those functions to understand the meaning.

At the very least, make sure give names to status codes, for example by creating an enum, or create custom std::error_codes.

You can also consider using exceptions, although I know that is frowned upon in embedded code. C++23 introduces std::expected, which is a better way to return either a value or an error code from a function. If you cannot use C++23 yet, it's quite easy to find an implementation of it that works on C++11, or you can create one yourself.

Also note that main() shouldn't return negative numbers; use EXIT_FAILURE instead.

Prefer creating a new struct over using std::pair

While std::pair has its uses, prefer creating a new struct if you need to bundle two values together. The advantage is that you can give the struct and its members your own names, instead of being stuck with the rather non-descriptive std::pair<>, first and second.

Avoid code duplication

Avoid repeating the same code multiple times; it saves you typing, avoids bugs and makes your code more maintainable. Consider having to add support for two more cameras: now you have to go and find all the places where you had separate lines of code for each camera, and make sure you duplicate and modify them correctly in all those places. Instead, you could do something like this:

class FirmwareUpgrader {
public:
 enum CameraPosition {
 front,
 middle,
 back,
 count
 };
 int upgrade(CameraPosition position, const std::vector<std::uint8_t>& firmware, std::uint32_t block_size) const;
 ...
private:
 std::unique_ptr<CameraUpgrader> m_upgraders[CameraPosition::count];
 ...
};
int FirmwareUpgrader::upgrade(CameraPosition position, const std::vector<std::uint8_t>& firmware, uint32_t std::block_size) const {
 if (!m_upgraders[position]) {
 return -1;
 }
 if (0 != m_upgraders[position]->update(firmware, block_size)) {
 return -2;
 }
 return 0;
}

Of course, you now might need to add some checks to avoid accessing the array out-of-bounds. Still, you can now easily add more cameras without needing to modify upgrade() at all. It is also more efficient than comparing the string type against all possible camera positions.

• John: Thank you for the detailed guidance. maybe the type of the object you want to create can just have set_*() functions, instead of having those in a separate builder type? If I understand you correctly.You mean add set* methods in the FhUpgrade, ScUpgrade itself without the builder classes for the aforemetioned classes. Am I right?
• G. Sliepen: Yes, that's what I meant.