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

Separation of concern on micro lvl

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
microconcernlvlseparation

Problem

The task is to create a DataTable with 1 column + header and 7 rows with content. The content will never change

Implementation:

private DataTable createTable()
    {
        var table = new DataTable();
        table.Columns.Add(new DataColumn("Wochentage", typeof(string)));// Wochentage=> WeekDays

        createRowAndAdd(table, "Montag");
        createRowAndAdd(table, "Dienstag");
        createRowAndAdd(table, "Mittwoch");
        createRowAndAdd(table, "Donnerstag");
        createRowAndAdd(table, "Freitag");
        createRowAndAdd(table, "Samstag");
        createRowAndAdd(table, "Sonntag");

        return table;
    }

    private void createRowAndAdd(DataTable table,string text)
    {
        DataRow newRow = table.NewRow();
        newRow[0] = text;
        table.Rows.Add(newRow);
    }


createRowAndAdd does 2 things:

  • create a row (and add the text)



  • add the row to the Table



Also, my createTable does more than just creating the table.

A method should only do one thing, right? How would you improve this and do you think there is a line between clean coding and adding overhead with no need?

Wochenuebersicht

```
public class Wochenuebersicht
{
public int KW { get; set; }
public DateTime BeginDate { get; set; }
public DateTime EndDate { get; set; }
public List WochenMenuListe { get; set; }
public DataTable Gesamt { get; set; }
public string Wochenangebot { get; set; }

public Wochenuebersicht()
{
DisplayTitle ="Wochenübersicht";
WochenMenuListe = new List();
WochenMenuListe.Add(createTage());
}

private WochenMenu createTage()
{
var table = new DataTable();
table.Columns.Add(new DataColumn("Wochentage", typeof(string)));

createRowAndAdd(table, "Montag");
createRowAndAdd(table, "Dienstag");
createRowAndAdd(table, "Mittwoch");
createRowAnd

Solution

You're correct, in general methods should try to do just one thing.

It doesn't seem difficult to alter your code to make that so.

private List days = new List()
{
    "Montag",
    "Dienstag",
    "Mittwoch",
    "Donnerstag",
    "Freitag",
    "Samstag",
    "Sonntag"
};

private const string ColumnHeader = "Wochentage";

private DataTable CreateTable()
{
    var table = new DataTable();
    table.Columns.Add(new DataColumn(ColumnHeader, typeof(string)));

    foreach(var day in days)
    {
        table.Rows.Add(CreateRow(table, day));
    }

    return table;
}

private DataRow CreateRow(DataTable table,string text)
{
    var row table.NewRow();
    row[0] = text;
    return row
}


You'll note I've made some other changes too.

  • Refactored your list of days into a, well, list. This lets us iterate over it and reduces duplicate code.



  • Removed your magic string "Wochentage" into a const.



  • Used var when declaring local variables when the right-hand side of declaration makes the type obvious



  • Used PascalCase when defining methods (as you should)



An additional change I'd make? I'd make the column header and the days parameters of CreateTable. Then you have a more generic function that could be usable elsewhere. I know the data is apparently not going to change, ever, but in the real world you can basically never guarantee that.

Code Snippets

private List<string> days = new List<string>()
{
    "Montag",
    "Dienstag",
    "Mittwoch",
    "Donnerstag",
    "Freitag",
    "Samstag",
    "Sonntag"
};

private const string ColumnHeader = "Wochentage";

private DataTable CreateTable()
{
    var table = new DataTable();
    table.Columns.Add(new DataColumn(ColumnHeader, typeof(string)));

    foreach(var day in days)
    {
        table.Rows.Add(CreateRow(table, day));
    }

    return table;
}

private DataRow CreateRow(DataTable table,string text)
{
    var row table.NewRow();
    row[0] = text;
    return row
}

Context

StackExchange Code Review Q#80340, answer score: 3

Revisions (0)

No revisions yet.