Jeg sidder og laver review på noget kode jeg startede på for over et år siden. Pludselig synes jeg at der er flere steder, hvor koden har fået en dårlig lugt – den virker ikke hel optimal. Jeg vil med denne post , og måske flere, forsøge at forbedre eksisterende kode. Input er selvfølgelig meget velkommen.
Det jeg netop er faldet over, og som jeg synes lugter lidt af gammel klædeskab, er følgende:
protected virtual void SetCurrent(StepBase step)
{
if (_current != step) // Kun hvis der er tale om nyt step
{
try
{
if (_current != null)
_current.Cleanup(); // Ryd op i nuværende Step
step.Initialize(); // Initialicer kommende Step
_current = step; // Sæt property-storen til nyt Step
}
catch (Exception ex)
{
_current = new ErrorStep(ex);
OnStepError(EventArgs.Empty);
}
OnStepChange(EventArgs.Empty); // Fortæl omverdnen at sekvensen har ny tilstand
}
}
Ovenstående metode, er en del af en klasse, der håndterer en sekvens. Sekvensen kan altid kun være på eet Step, og diverse transaktioner styrer sekvensen rundt mellem de mulige Step’s. Metoden er oprettet, fordi jeg mener at koden er lige en tand for kompleks, til at være i en almindelig set-accessor på en property.
Den del, jeg måske har det lidt dårligt med, er: _current.Cleanup() og _current.Initialize(). Disse to metoder bruges på de enkelte Step’s til henholdsvis at gøre klar når sekvensen indtræder i Steppet og rydde op når Steppet forlades.
Min dårlige fornemmelse går på at Sequenceklassen har ansvar for at Stepklassen initialiceres og ryddes op – det virker ikke som god stil, da man er som minimum nød til at markerer metoderne som internal, for at undgå at klientkode får adgang til dem – kun en Sequence skal have den mulighed, når nu ikke Step kan selv.
Stepklasserne kan være yderst komplekse, og have adgang til web-services og andre latente komponenter – derfor vil det ikke være en løsning at klare sagerne i konstruktør og Finalizemetoder – det vil give for dårlig performance.
Lugter denne konstruktion, og er der en smartere måde?