Factory to create Finite Impulse Response filters in C++

Question

I've recently begun a project in digital signal processing and am using C++ as the programming language. In this project, I have implemented several finite impulse response (FIR) filters. Many of these FIR filters have the same interface, but are implemented differently depending on what can be optimized. To remove the hassle of requiring a user to know what filter to use, I have created an 'FIRFactory' class which is responsible for creating a filter that returns the optimal implementation for the given user input.

Referring to the code below, I have the class separated into a .h and .cpp file. This code works perfectly fine, however, I would like to take a step back and make sure I'm not violating any 'best practices'.

I'm interested in several items:

• Style. Is there anything with how this class is laid out that just looks 'odd'?
• Pass by value or reference. Now in C, when you pass an array into a function, you're actually just passing a pointer, thus you don't really care about the whole 'pass by value or reference'. However in C++, is it considered 'ok' that I'm passing a vector by value? Is this an expensive thing to do?
• Move semantics. I'm still trying to understand the whole thing surrounding move semantics (how expensive is it to return a value). Would I be violating anything in this instance?
• Do you have any personal 'irks' in the code? (i.e. I can't stand it when people do x)
• And in the end, I'm really just curious if anything 'wrong' sticks out.

Here is the link to my Github repo where this code resides. If you're curious about the other code components being called, you can look in your own interest into the repo. However, I'm primarily interested just in the code posted below.

firfactory.h

#ifndef FIRFACTORY_H
#define FIRFACTORY_H

#include "firfilter.h"
#include <memory>
#include <vector>

class FIRFactory
{
    public:
        static std::unique_ptr<FIRFilter> getFilter(std::vector<double> impulse_response);

    private:
        static bool isLinearPhaseType1(std::vector<double> impulse_response);
        static bool isLinearPhaseType2(std::vector<double> impulse_response);
        static bool isLinearPhaseType3(std::vector<double> impulse_response);
        static bool isLinearPhaseType4(std::vector<double> impulse_response);
};

#endif // FIRFACTORY_H

firfactory.cpp

#include "firfactory.h"
#include "linearphasetype1.h"
#include "linearphasetype2.h"
#include "linearphasetype3.h"
#include "linearphasetype4.h"
#include "directformfirfilter.h"

/**
 * @brief Identifies the FIR filter type (type 1, 2, 3, 4) and returns the appropraite FIR object pointer
 * @param Impulse response of FIR filter
 * @return Returns a unique (smart) pointer to an appropriate FIR filter type object
 */
std::unique_ptr<FIRFilter> FIRFactory::getFilter(std::vector<double> impulse_response)
{
    if(isLinearPhaseType1(impulse_response))
        return std::unique_ptr<LinearPhaseType1>{new LinearPhaseType1(impulse_response)};
    if(isLinearPhaseType2(impulse_response))
        return std::unique_ptr<LinearPhaseType2>{new LinearPhaseType2(impulse_response)};
    if(isLinearPhaseType3(impulse_response))
        return std::unique_ptr<LinearPhaseType3>{new LinearPhaseType3(impulse_response)};
    if(isLinearPhaseType4(impulse_response))
        return std::unique_ptr<LinearPhaseType4>{new LinearPhaseType4(impulse_response)};
    else
        return std::unique_ptr<DirectFormFIRFilter>{new DirectFormFIRFilter(impulse_response)};
}

/**
 * @brief Checks to see if the impulse response is FIR Type 1
 * @param impulse_response Impulse response of FIR filter
 * @return Returns 'TRUE' if FIR is Type 1, returns false otherwise
 */
bool FIRFactory::isLinearPhaseType1(std::vector<double> impulse_response)
{
    if(!(impulse_response.size()%2)) // check for odd number of coefficients
        return false;

    for(unsigned i = 0; i < impulse_response.size()/2; i++)
    {
        if(impulse_response[i] != impulse_response[impulse_response.size()-1-i])
            return false;
    }

    return true;
}

/**
 * @brief Checks to see if the impulse response is FIR Type 2
 * @param impulse_response Impulse response of FIR filter
 * @return Returns 'TRUE' if FIR is Type 2, returns false otherwise
 */
bool FIRFactory::isLinearPhaseType2(std::vector<double> impulse_response)
{
    if(impulse_response.size()%2) // check for even number of coefficients
        return false;

    for(unsigned i = 0; i < impulse_response.size()/2; i++)
    {
        if(impulse_response[i] != impulse_response[impulse_response.size()-1-i])
            return false;
    }

    return true;
}

/**
 * @brief Checks to see if the impulse response is FIR Type 3
 * @param impulse_response Impulse response of FIR filter
 * @return Returns 'TRUE' if FIR is Type 3, returns false otherwise
 */
bool FIRFactory::isLinearPhaseType3(std::vector<double> impulse_response)
{
    if(!(impulse_response.size()%2)) // check for odd number of coefficients
        return false;

    for(unsigned i = 0; i < impulse_response.size()/2; i++)
    {
        if(impulse_response[i] != -1*impulse_response[impulse_response.size()-1-i])
            return false;
    }

    return true;
}

/**
 * @brief Checks to see if the impulse response is FIR Type 4
 * @param impulse_response Impulse response of FIR filter
 * @return Returns 'TRUE' if FIR is Type 4, returns false otherwise
 */
bool FIRFactory::isLinearPhaseType4(std::vector<double> impulse_response)
{
    if(impulse_response.size()%2) // check for even number of coefficients
        return false;

    for(unsigned i = 0; i < impulse_response.size()/2; i++)
    {
        if(impulse_response[i] != -1*impulse_response[impulse_response.size()-1-i])
            return false;
    }

    return true;
}

Answer 1

Overall code is good & seems maintainable. Some suggestions:

1. Pass std::vector by const reference. You are not modifying these so const will help. Also, copy is not needed so by reference is more efficient.

2. Your 4 cases mainly differ between odd/even and positive/negative impulse checks. Perhaps these 2 conditions could be added as param to the function so you have 1 function instead of 4 (but this will not work if in the future there are additional non-trivial checks that go into determining filter type, so is a judgement call based on domain knowledge). You could further optimize this by having 2D array look-up for odd/even as first index & positive/negative implulse match resultas second dimension index (but this is not always easy to read for new maintainers); This will reduce the nested if-else.

3. Everything is static in the class (& no member variables), perhaps it should be a namespace instead of a class? Also, whenever I see static private, I feel the header file/class definition doesn't need to expose it to users. You should be able to move these to functions in .cpp file in the default namespace without issues.