1

Example taken from : Agile software development : principles, patterns and practices

A new employee is added by the receipt of an AddEmp transaction. This transaction contains the employee's name, address, and assigned employee number. The transaction has three forms:

AddEmp <EmpID> "<name>" "<address>" H <hourly-rate>
AddEmp <EmpID> "<name>" "<address>" S <monthly-salary>
AddEmp <EmpID> "<name>" "<address>" C <monthly-salary> <commission-rate>

proposed employee class enter image description here Add employee transaction enter image description here

My question would it be "better" if instead of subclassing AddEmployeeTransaction we have a factory for salary classification and pay schedule?

If better is too vague a term, what are disadvantages of my design over the one proposed by Uncle Bob?


Say inside EmployeeTransaction we have SalaryClassificationFactory and we call SalaryClassificationFactory.get(SalaryDetails d)

where,

enum SalaryId{H,S,C};


struct hourly
{
    int hourlyRate;
};

struct monthly
{
    int monthlySalary;
};

struct commissioned
{
    int salary;
    int commisssionRate;
};
struct SalaryDetails
{
    SalaryId kind;
    union SalaryUnion
    {
        hourly h;
        monthly s;
       commissioned c;
    }
};
q126y
  • 1,713
  • 3
  • 14
  • 24
  • 1
    The SalaryId is good, but not sufficient, because the union is needed. As a result this shifts the problem up the line (toward the client), while adding an (albeit small) layer or two of complexity ((a) the new factory, which still has the same amount of work to do as in 19-2, and (b) the union). If you standardize on SalaryId's like H1, H2, H3, S1, S2, S3, C1, C2 and remove the union, you can make the factory pattern work (at the potentially high cost of constraining the choices like salary amount, and forgoing some Openness for Extension as mentioned by @Euphoric). – Erik Eidt Jan 15 '16 at 17:14

2 Answers2

2

Typical factory patterns defer creation of some object(s) to some factory, allowing the factory to choose the final type or object, but they do this from a common, shared (usually simple, often abstract) interface that requests an object. For example, (from http://www.oodesign.com/factory-pattern.html)

Product ProductFactory.createProduct(ProductId id)

Here the factory can create the appropriate subclass of Product by consulting the ProductId, and presumably a product catalog. All the different types that the factory can create are requested using the same method.

It seems to me that the parameters needed to accomplish each of the three salary classification transactions are substantially different from each other. Thus, a traditional factory method pattern probably would not apply across all three.

You could hide the existing subclass-based implementation behind a some other class, but you would still need three methods, each with different parameters, and as a result, that hiding class would not, in my opinion look like a factory.

Unless you could unite the various salary classification descriptions under a common type, say as in:

AddEmployeeTransaction 
    AddEmployeeFactory.createAddEmployee ( EmployeeCompensationClassification ecc, 
                                           EmployeeInfo who )

Now this method looks like a factory pattern, but we haven't accomplished anything because all we have done is defer the same kind of problem to creating the EmployeeCompensationClassification.

If you could reduce all salary compensation classifications to that of a simple code (e.g. a classification name, or, a classification id number), I think a factory pattern would have merit.

Erik Eidt
  • 33,282
  • 5
  • 57
  • 91
1

One of the design decisions Uncle Bob made when creating his design is that Payment Classification should be Open for Extension. Which means that adding new payment classifications shouldn't require changing nor recompiling existing code. Your design breaks that design decision, because to add new payment classification, you have to add new value to SalaryId enum, new structure and add it to SalaryUnion, forcing recompile of all code that depends on SalaryId and SalaryDetails.

But that is Uncle Bob's decision. If you yourself decide that you don't want to accept complexity created by making the design Open for Extension, you can go with your design.

Euphoric
  • 36,735
  • 6
  • 78
  • 110
  • But the code that uses `SalaryDetails` will have to recompile when new payement classification is added, even in Uncle Bob's approach. The only additional code that will require modification in my approach is `SalaryClassificationFactory`, but factories have to be changed on addition of new types. Please correct if I am wrong. I am learning. – q126y Jan 15 '16 at 09:13
  • @Q126y No. In Uncle Bob's design, new payment classifications can be added without recompile. For example it can be in new library. – Euphoric Jan 15 '16 at 09:35
  • But the old code where new payment classification will be used, will need to be recompiled[because different payment classifications take different parameters], isn't it? – q126y Jan 15 '16 at 10:20
  • This question is tagged C++, where full build (i.e. full recompiles) (e.g. for doing a release) are the norm anyway, I think, even still today. I think Open for Extension applies though relates more to touching source code that wouldn't otherwise need to be. – Erik Eidt Jan 15 '16 at 17:07
  • @ErikEidt The original design is in Java (or C#). And even if it is "standard" in C++ doesn't mean you won't get advantages for making your design like that. It makes dependency tree shallower, allowing for better partial or parallel build. – Euphoric Jan 15 '16 at 18:38