From Template Method to Delegation
Here is a mildly interesting refactoring from a use of the GoF Template Method to a use of delegation injected into a parent class.
In determining the Lauds and Vespers hymns on a seasonal basis, most are straightforward and do not vary by Use: Roman and Sarum uses agree, and the hymns are the same throughout the season. But for days during Ordinary Time these vary by day of week and in one of the two periods of Ordinary time the Roman and Sarum uses do not agree: Roman use uses the same pattern as during the other period, but Sarum use uses one hymn per office for the whole season (which is the same as the Roman Sunday hymn).
The original solution to this was to use a form of the GoF Template Method pattern.
A switch statement which generally returns fixed values has four functions it calls for the relevant cases:
switch (inOffice)
{
using enum OfficeNames;
case COMPLINE:
return ComplineChapter::GetHymnName(grade(), getSeason(),
followRomanUse());
case TERCE:
return Terce::GetHymnName(getSeason());
case PRIME:
return "Iam lucis ordo sidere";
case SEXT:
return "Rector potens, verax Deus";
case NONE:
return "Rerum Deus tenax vigor";
case VESPERS:
switch (getSeason())
{
using enum HymnSeasons;
case SEPTUAGESIMA:
return getPreLentenVespersHymn();
break;
case NORMAL:
return getPostLentenVespersHymn();
break;
case ADVENT:
return "Conditor alme siderum";
case INCARNATION:
return "Veni, Redemptor gentium";
case LENT:
return "Ex more docti mystico";
case MID_LENT:
return "Ecce tempus idoneum";
case PASSIONTIDE:
return "Vexilla Regis prodeunt";
case EASTER:
return "Ad coenam agni providi";
case ASCENSION:
return "Aeternae Rex altissime";
case PENTECOST:
return "Beata nobis gaudia";
}
break;
case LAUDS:
switch (getSeason())
{
using enum HymnSeasons;
case SEPTUAGESIMA:
return getPreLentenLaudsHymn();
break;
case NORMAL:
return getPostLentenLaudsHymn();
break;
case ADVENT:
return "Vox clara ecce intonat";
case INCARNATION:
return "A solis ortus cardine";
case LENT:
return "Audi benigne Conditor";
case MID_LENT:
return "Iesu, quadragesinarie";
case PASSIONTIDE:
return "Lustra sex qui iam peracta";
case EASTER:
return "Sermone blando angelus";
case ASCENSION:
return "Te, Christe, nostrum gaudium";
case PENTECOST:
return "Impleta gaudent viscera";
}
}
return {};
}
and the functions are declared pure virtual in the parent class, in addition to a getDay() call used in a different function (still Template Method pattern).
class FerialHoursInfo : public IHoursInfo
{
...
protected:
virtual Days getDay() const = 0;
virtual std::string getPreLentenVespersHymn() const = 0;
virtual std::string getPostLentenVespersHymn() const = 0;
virtual std::string getPreLentenLaudsHymn() const = 0;
virtual std::string getPostLentenLaudsHymn() const = 0;
...
There are classes for each day of the week, and in, e.g., the Monday class we have:
std::string getPreLentenVespersHymn() const override
{
return "Immense caeli Conditor";
}
std::string getPostLentenVespersHymn() const override
{
if (!followRomanUse())
{
return "Lucis creator optime";
}
else
{
return "Immense caeli Conditor";
}
}
std::string getPreLentenLaudsHymn() const override
{
return "Splendor Paternae gloriae";
}
std::string getPostLentenLaudsHymn() const override
{
if (!followRomanUse())
{
return "Ecce iam noctis";
}
else
{
return "Splendor Paternae gloriae";
}
}
This is, on its face, a reasonable solution, following a known pattern. But there's a small niggling issue. Consider the Tuesday version:
std::string getPreLentenVespersHymn() const override
{
return "Telluris alme Conditor";
}
std::string getPostLentenVespersHymn() const override
{
if (!followRomanUse())
{
return "Lucis creator optime";
}
else
{
return "Telluris alme Conditor";
}
}
std::string getPreLentenLaudsHymn() const override
{
return "Ales diei nuntius";
}
std::string getPostLentenLaudsHymn() const override
{
if (!followRomanUse())
{
return "Ecce iam noctis";
}
else
{
return "Ales diei nuntius";
}
}
There's a repeated structural similarity between the two cases: the default Sarum hymn for the post-Lenten ordinary time block is identical, and the logic is structurally isomorphic. This is repeated seven times through seven child classes.
We can avoid duplicated code by using strategies. The first aim, then is to replace the wired-in logic with delegation to one of a pair of strategies:
class IVariableHymnGenerator
{
public:
virtual ~IVariableHymnGenerator();
virtual std::string getPreLentenVespersHymn() const = 0;
virtual std::string getPostLentenVespersHymn() const = 0;
virtual std::string getPreLentenLaudsHymn() const = 0;
virtual std::string getPostLentenLaudsHymn() const = 0;
virtual std::unique_ptr<IVariableHymnGenerator> clone() const = 0;
virtual bool isRoman() const = 0;
};
The Roman version is straightforward:
class RomanVariableHymnGenerator : public IVariableHymnGenerator
{
public:
RomanVariableHymnGenerator(const std::string &inVespersHymn,
const std::string &inLaudsHymn):
m_vespersHymn(inVespersHymn),
m_laudsHymn(inLaudsHymn)
{ }
~RomanVariableHymnGenerator() override;
// Functions from IVariableHymnGenerator
std::string getPreLentenVespersHymn() const override
{
return m_vespersHymn;
}
std::string getPostLentenVespersHymn() const override
{
return m_vespersHymn;
}
std::string getPreLentenLaudsHymn() const override { return m_laudsHymn; }
std::string getPostLentenLaudsHymn() const override { return m_laudsHymn; }
std::unique_ptr<IVariableHymnGenerator> clone() const override
{
return std::make_unique<RomanVariableHymnGenerator>(*this);
}
bool isRoman() const override { return true; }
private:
std::string m_vespersHymn;
std::string m_laudsHymn;
};
The Sarum version is almost as simple:
class SarumVariableHymnGenerator : public IVariableHymnGenerator
{
public:
SarumVariableHymnGenerator(const std::string &inVespersHymn,
const std::string &inLaudsHymn):
m_vespersHymn(inVespersHymn),
m_laudsHymn(inLaudsHymn)
{ }
~SarumVariableHymnGenerator() override;
std::string getPreLentenVespersHymn() const override
{
return m_vespersHymn;
}
std::string getPostLentenVespersHymn() const override
{
return "Lucis creator optime";
}
std::string getPreLentenLaudsHymn() const override { return m_laudsHymn; }
std::string getPostLentenLaudsHymn() const override
{
return "Ecce iam noctis";
}
std::unique_ptr<IVariableHymnGenerator> clone() const override
{
return std::make_unique<SarumVariableHymnGenerator>(*this);
}
bool isRoman() const override { return false; }
private:
std::string m_vespersHymn;
std::string m_laudsHymn;
};
We have eliminated all the if tests and captured the variation as general logic. But now all those template functions have identical bodies in every class, e.g.
std::string getPreLentenVespersHymn() const override
{
return m_generator->getPreLentenVespersHymn();
}
so we have exact code duplication across seven classes. We can push the logic up into the parent by passing the variation in via the constructor (which is protected: it can be called only in instantiating a child class):
FerialHoursInfo(const HymnSeasons inSeason, const Grades inGrade,
std::unique_ptr<IVariableHymnGenerator> &&inGenerator):
m_generator(std::move(inGenerator)),
m_season(inSeason), m_grade(inGrade)
{ }
Now we don't even have to have the four template methods defined: we can call the strategy directly. In the child classes, each day does appropriate initialization of its parent:
namespace
{
const std::string VespersHymn{ "Immense caeli Conditor" };
const std::string LaudsHymn{ "Splendor Paternae gloriae" };
}
namespace BreviaryLib
{
MondayHoursInfo::MondayHoursInfo(const HymnSeasons inSeason,
const Grades inGrade, const bool inRomanUse):
FerialHoursInfo(
inSeason, inGrade,
[](const bool romanUse) -> std::unique_ptr<IVariableHymnGenerator> {
if (romanUse)
{
return std::make_unique<RomanVariableHymnGenerator>(VespersHymn,
LaudsHymn);
}
else
{
return std::make_unique<SarumVariableHymnGenerator>(VespersHymn,
LaudsHymn);
}
}(inRomanUse))
{ }
...
These are generated in a factory which hides the concrete types from the callers:
std::unique_ptr<IHoursInfo> DailyHoursInfoFactory::CreateWeekdayHoursInfo(
const Days inDay, const HymnSeasons inSeason, const Grades inGrade,
bool inRomanUse)
{
switch (inDay)
{
using enum Days;
case SUNDAY:
return std::make_unique<SundayHoursInfo>(inSeason, inGrade, inRomanUse);
case MONDAY:
return std::make_unique<MondayHoursInfo>(inSeason, inGrade, inRomanUse);
case TUESDAY:
return std::make_unique<TuesdayHoursInfo>(inSeason, inGrade, inRomanUse);
case WEDNESDAY:
return std::make_unique<WednesdayHoursInfo>(inSeason, inGrade,
inRomanUse);
case THURSDAY:
return std::make_unique<ThursdayHoursInfo>(inSeason, inGrade,
inRomanUse);
case FRIDAY:
return std::make_unique<FridayHoursInfo>(inSeason, inGrade, inRomanUse);
case SATURDAY:
return std::make_unique<SaturdayHoursInfo>(inSeason, inGrade,
inRomanUse);
default:
throw OfficeParseException("Illegal forced value for daily hours info");
}
}
(You can use a template function to do the creation, but there is so little to cover that it essentially just replaces std::make_unique with the function name.)
Now, you may note that we are halfway to being able to eliminate the concrete classes entirely if we wanted. If we changed the parent class structure a little, made the constructor public, and used the factory to determine what went in we could get rid of the concrete weekday classes altogether. All variation would be handled either by simple data variation or by the Strategy pattern. However, I'm not sure that this gets us any benefit: all we are doing is swapping out one location for having the initialization knowledge for seven separate places. And that, if you think about it, goes against the grain of having each class do as little as possible[1]. There is tight cohesion between a class which represents Monday variation and the knowledge of which hymns it needs. There is less between a general factory which, as it currently stands, needs to know only the day of the week and some characteristics of the day being processed, and the same hymn knowledge. (That small amount of knowledge is what make for the repeated call signatures in the switch branches. Variation takes place out of sight of the factory.) Moving the details out of small child classes -- and they are now very small -- into the factory produces a single class with more responsibility and weaker cohesion.
[1] "The first rule of classes is that they should be small. The second rule of classes is that they should be smaller than that." (Robert C. Martin, Clean Code: A Handbook of Agile Software Craftsmanship (Prentice Hall 2008))
So we will leave this where it is. In place of eight classes with longer interfaces (the original template methods) we now have eleven classes with shorter interfaces. The parent class is slightly more complex, as it now has an extra member, though it trades that off against the simplification of its interface. (Actually, the strategy replaces a boolean flag indicating the use, so it's not even an extra member, just a more complex one.). The seven child classes are all now considerably simpler and, in another plus, complexity has been moved from runtime calls into the constructor. The three new classes are all small and simple -- and have allowed a couple of if/else tests to be dispensed with.
Comments
Post a Comment