patterncsharpMinor
Calculating German common holidays
Viewed 0 times
holidayscalculatingcommongerman
Problem
I wrote a simple dll which is able to calculate all common holidays, but, I would like to optimize the code in readability and flexibility.
Federal state
holiday
```
///
/// Hält eine Liste von Feiertagen für die Jahr Monat Kombination
///
public class MyFeiertagLogic
{
private static MyFeiertagLogic Instance;
private List feiertage;
private int year;
///
/// Das Jahr für welches die Feiertage berechnet werden
///
public int CurrentYear
{
get { return year; }
set { year = value; }
}
///
/// Erzeugt eine neue Instanz der Feiertage
Federal state
public enum Bundesland
{
BadenWürttemberg,
Bayern,
Berlin,
Brandenburg,
Bremen,
Hamburg,
Hessen,
MecklenburgVorpommern,
Niedersachsen,
NordrheinWestfalen,
RheinlandPfalz,
Saarland,
Sachsen,
SachsenAnhalt,
SchleswigHolstein,
Thüringen
}holiday
///
/// Representiert den Feiertag mit
/// allen wichtigen informationen
///
public class Feiertag : IComparable
{
private bool isFix;
private DateTime datum;
private string name;
public Feiertag(bool isFix, DateTime datum, string name)
{
this.IsFix = isFix;
this.Datum = datum;
this.Name = name;
}
///
/// Der Name des Feiertages
///
public string Name
{
get { return name; }
set { name = value; }
}
///
/// Das Datum an dem dieser Feiertag stattfindet
///
public DateTime Datum
{
get { return datum; }
set { datum = value; }
}
///
/// Zeigt an ob es sich um einen Datums spezifischer
/// oder zyklisch Feiertag handelt
///
public bool IsFix
{
get { return isFix; }
set { isFix = value; }
}
#region IComparable Member
public int CompareTo(Feiertag other)
{
return this.datum.Date.CompareTo(other.datum.Date);
}
#endregion IComparable Member
}holidaylogic:```
///
/// Hält eine Liste von Feiertagen für die Jahr Monat Kombination
///
public class MyFeiertagLogic
{
private static MyFeiertagLogic Instance;
private List feiertage;
private int year;
///
/// Das Jahr für welches die Feiertage berechnet werden
///
public int CurrentYear
{
get { return year; }
set { year = value; }
}
///
/// Erzeugt eine neue Instanz der Feiertage
Solution
Singleton bug
Your singleton implementation is wrong:
If I try to do…
… I'll get the instance for Saxony instead of a second instance for Brandenburg as expected.
Library design
I think that requiring the year to be specified in the
Enum
The Bundesländer all have standard two-letter abbreviations. I think the code would be less awkward if you used those abbreviations as enums instead of the full names.
Your singleton implementation is wrong:
public static MyFeiertagLogic GetInstance(int year)
{
if (Instance == null || year != Instance.CurrentYear)
{
Instance = new MyFeiertagLogic(year, Bundesland.Sachsen);
return Instance;
}
return Instance;
}
public static MyFeiertagLogic GetInstance(int year, Bundesland bl)
{
if (Instance == null || year != Instance.CurrentYear)
{
Instance = new MyFeiertagLogic(year, bl);
return Instance;
}
return Instance;
}If I try to do…
MyFeiertagLogic feiertageInSachsenDiesesJahr = MyFeiertagLogic.GetInstance(2015);
MyFeiertagLogic feiertageInBrandenburgDiesesJahr = MyFeiertagLogic.GetInstance(2015, Bundesland.Brandenburg);… I'll get the instance for Saxony instead of a second instance for Brandenburg as expected.
Library design
I think that requiring the year to be specified in the
GetInstance() call is a poor design. The year is part of the DateTime arguments passed to isFeierTag(DateTime) (which should be called IsFeierTag(DateTime), by the way) and GetFeiertagName(DateTime). It should be your library's job to figure out how to satisfy those queries by loading the data for the appropriate year.Enum
The Bundesländer all have standard two-letter abbreviations. I think the code would be less awkward if you used those abbreviations as enums instead of the full names.
Code Snippets
public static MyFeiertagLogic GetInstance(int year)
{
if (Instance == null || year != Instance.CurrentYear)
{
Instance = new MyFeiertagLogic(year, Bundesland.Sachsen);
return Instance;
}
return Instance;
}
public static MyFeiertagLogic GetInstance(int year, Bundesland bl)
{
if (Instance == null || year != Instance.CurrentYear)
{
Instance = new MyFeiertagLogic(year, bl);
return Instance;
}
return Instance;
}MyFeiertagLogic feiertageInSachsenDiesesJahr = MyFeiertagLogic.GetInstance(2015);
MyFeiertagLogic feiertageInBrandenburgDiesesJahr = MyFeiertagLogic.GetInstance(2015, Bundesland.Brandenburg);Context
StackExchange Code Review Q#99003, answer score: 5
Revisions (0)
No revisions yet.