HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Calculating German common holidays

Submitted by: @import:stackexchange-codereview··
0
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

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:

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.