Która wersja kodu jest lepsza

Powiedzmy, że chciałbym napisać loggera. Wywołuję metodę i na moim historyListView pojawia się odpowiedni napis w kolorze zależnym od podanej typu wiadomości. Najprościej można to zaimplementować tak:

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }

    private void Log(string message, LogType type = LogType.Info)
    {
        var timestamp = DateTime.Now.ToString("dd-MM-yyyy hh:mm:ss");
        var msg = timestamp + " - " + message;
        var color = Color.Black;

        switch (type)
        {
            case LogType.Error:
                color = Color.Red;
                break;
            case LogType.Warn:
                color = Color.Orange;
                break;
            case LogType.Info:
                color = Color.Black;
                break; 
        }

        ListViewItem item = new ListViewItem()
        {
            Text = msg,
            ForeColor = color
        };

        historyListView.Items.Add(item);
        historyListView.ScroolToEnd();
    }

    private void performActionButton_Click(object sender, EventArgs e)
    {
        Log("error", LogType.Error);
        Log("warn", LogType.Warn);
        Log("info", LogType.Info);
    }
}

Tylko dlaczego Form ma odpowiadać za logikę powstawania wiadomości? Wyodrębniłem to więc do osobnej klasy LogMessageCreator:

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }

    private void Log(string message, LogType type = LogType.Info)
    {
        var logMessageCreator = new LogMessageCreator();
        var item = logMessageCreator.Create(message, type);

        historyListView.Items.Add(item);
        historyListView.ScroolToEnd();
    }

    private void performActionButton_Click(object sender, EventArgs e)
    {
        Log("error", LogType.Error);
        Log("warn", LogType.Warn);
        Log("info", LogType.Info);
    }
}

class LogMessageCreator
{
    public ListViewItem Create(string message, LogType type = LogType.Info)
    {
        var timestamp = DateTime.Now.ToString("dd-MM-yyyy hh:mm:ss");
        var msg = timestamp + " - " + message;
        var color = Color.Black;

        switch (type)
        {
            case LogType.Error:
                color = Color.Red;
                break;
            case LogType.Warn:
                color = Color.Orange;
                break;
            case LogType.Info:
                color = Color.Black;
                break;
        }

        ListViewItem item = new ListViewItem()
        {
            Text = msg,
            ForeColor = color
        };

        return item;
    }
}

Tylko znowu zakładając, że chciałbym w różnym miejscu inaczej wyświetlać wiadomości (raz np. z datą, a raz nie) to stworzyłem klasę, która odpowiada za skonfigurowanie sposobu wyświetlania wiadomości:

class LogMessageCreator
{
	public ListViewItem Create(string message, LogType type, LogMessageCreatorOptions options = null)
	{
		if (options == null)
		{
			options = new LogMessageCreatorOptions();
		}

		var messageBuilder = new StringBuilder();

		if (options.AddDateTime)
			messageBuilder.Append(DateTime.Now.ToString("dd-MM-yyyy hh:mm:ss")).Append(" - ");

		messageBuilder.Append(message);

		var color = Color.Black;

		switch (type)
		{
			case LogType.Error:
				options.MessageColor = Color.Red;
				break;
			case LogType.Warn:
				options.MessageColor = Color.Orange;
				break;
			case LogType.Info:
				options.MessageColor = Color.Black;
				break;
		}

		ListViewItem item = new ListViewItem()
		{
			Text = messageBuilder.ToString(),
			ForeColor = options.MessageColor
		};

		return item;
	}
}

class LogMessageCreatorOptions
{
    public bool AddDateTime { get; set; } = true;
    public Color MessageColor { get; set; } = Color.Black;
}

1. Czy coś takiego jest ok? Czy powinienem iść dalej i jeszcze bardziej to komplikować? Pewnie coś by się znalazło.
2. Czy ten zapis:

public bool AddDateTime { get; set; } = true;
public Color MessageColor { get; set; } = Color.Black;

jest ok? Czy może powinienem ustawić te właściwości w konstruktorze? A może dodać metodę CreateDefault? Czy nie powinny one być czasem prywatne? Wydaje mi się, że nie muszą być prywatne, bo służą jako modele DTO.

1. W ostatniej opcji do metody Create przesyłam obiekt LogMessageCreatorOptions, któremu potem wypełniam pole MessageColor. Czy takie rozwiązanie jest ok? Wydaje mi się, że powinienem tylko odczytywać dane z tego obiektu, a nie wypełniać go jakimiś danymi.

2. Czy czasem obiekt LogMessageCreatorOptions zamiast właściwości MessageColor powinien posiadać właściwość MessageType? Wtedy nie musiałbym mu ustawiać właściwości o kolorze, a tylko odczytywałbym typ wiadomości i na podstawie tego tworzył zmienną z kolorem. Tylko czy takie rozwiązanie nie przeczy idei tego obiektu? On miał służyć temu w jaki sposób mają być wyświetlone te dane (ich formatowanie), a nie typ tych danych. A może kolor właśnie jest sposobem sformatowania danych?

3. Mam wrażenie, że w programowaniu często problemy rozwiązuję "na czuja", tak jak mi się wydaję. Nie mam takiego określonego schematu - zadaniem tego obiektu jest XXX, więc nie może on robić YYY. Jak sobie z tym poradzić? Innym przykładem będzie to, że nie wiem, czy powinienem zrobić folder Extensions i tam umieszczać klasy StringExtensions, DateTimeExtensions.. czy może zrobić folder Helpers i tam umieścić klasę Extensions, w której miałbym wszystkie rozszerzenia..

Działa, ale ja bym tego tak nie napisał. Nie rozumiem...dobra nie będę tego rozwijał. Zastanów się jak to ma działać, sprawdź sobie po co są właściwości i pola jeśli nie możesz się zdecydować, wszystko ma swoje miejsce. Osobiście zacząłbym od jakiejś warstwy abstrakcji, później zastanowił się czy chcę tak bardzo uniezależniać logger od frameworka, najpierw coś ogólniejszego i czy na pewno potrzebujesz instancji, dla mnie bardziej pasowałaby klasa statyczna.

@Zimny Krawiec: To nie jest w żadnym wypadku zbędne, tutaj chodzi o to żeby uczył się dobrych wzorców zaczynając od właśnie takich pozornie prostych rzeczy. Nie ma pisać by "działało" ale wyrobił sobie myślne o wielokrotnym użytku, aby tak jak ten logger nie działał tylko w tym konkretnym przypadku, ale w przyszłości można było go wykorzystać w innej aplikacji, wypisywać logi na konsole, czy tworzyć pliki z logami. Jestem pewny, że autor tematu uczy się programować i po to piszę się małe projekty, aby później napisać coś znacznie większego i się w tym nie pogubić.

Każda wersja jest zła. Ale idziesz w dobrym kierunku zadając odpowiednie pytania.

Przede wszystkim musisz wyodrębnić klasę Logger - to już zrobiłeś.
Po drugie, zrób z niej klasę statyczną. Ona nie trzyma żadnych informacji, więc bez sensu jest jej tworzenie. Niech będzie statyczna. Wtedy dostęp do niej też będzie ułatwiony:
Logger.Log("Siema");

Po trzecie - niech ta klasa nie zwraca ListViewItem. To ma być klasa do logowania, a nie tworzenia ListViewItem.

Ja bym to zrobił tak, że:

• wyposażyłbym statyczną klasę Loggera w statyczne property, które przechowywałoby jakieś ustawienia. Ewentualnie zrobiłbym z tego single instance za pomocą najlepiej jakiegoś kontenera IoC i wstrzyknął do konstruktora opcje.

Klasa powinna mieć następujące możliwości:

• powinna móc wypisywać komunikaty w konsoli
• powinna móc wypisywać komunikaty w pliku
• powinna móc wypisywać komunikaty w inny dowolny sposób (bo widzę, że do tego dążysz, ja bym zatrzymał się na konsoli)

To wszystko powinno dziać się w zależności od opcji.

Punkt trzeci możesz rozwiązać dwojako.
Przede wszystkim możesz stworzyć sobie specjalnego itema, w stylu:

class LogItem
{
  public Color Color {get;set;}
  public string Message {get;set;}
}

I zwracać tego itema.

Możesz też wyposażyć Loggera w jakieś zdarzenie OnLog i wywoływać to zdarzenie zamiast zwracać item.

A dlaczego nie powinieneś zwracać ListViewItem? Bo wtedy ściśle wiążesz sobie tą klasę z jedną konkretną technologią i nie wykorzystasz jej nigdzie indziej.

Juhas napisał(a):

Po drugie, zrób z niej klasę statyczną. Ona nie trzyma żadnych informacji, więc bez sensu jest jej tworzenie. Niech będzie statyczna. Wtedy dostęp do niej też będzie ułatwiony:

Jak masz komus tak radzic to lepiej nic nie pisz... bo tylko krzywde robisz mu

Hej,
tak trochę ogólnie, ze swojego niewielkiego doświadczenia... Jak dobrze tworzyć obiekty i metody... tego Ci nikt nie powie... są pewne podstawy... ale jak w praktyce to robić, to zależy od wielu czynników... natomiast jedno jest pewne... zanim zaczniesz projektować klasy, objekty, metody... zastanów się jak ma wyglądać całość... jak sobie zaprojektujesz cały schemat (tak mniej więcej, ale można też szczegółowo)... to wtedy będzie Ci łatwiej podjąć decyzję, gdzie wcisnąć jaką metodę... jakie stworzyć objekty, itd... Nikt sensowny, nie odpowie Ci czy dobrze tworzysz objekty nie znając dobrze Twojego problemu programistycznego, o ile nie popełniasz elementarnych błędów... Ufff, to tyle... :) PS. dobrze jest projektować też tak, aby przestrzegać pewnych standardów i umieć przewidywać, co się stanie, jak będziesz próbował rozszerzyć projekt o kolejne objekty, ale to nie takie proste...

west napisał(a):

1. Mam wrażenie, że w programowaniu często problemy rozwiązuję "na czuja", tak jak mi się wydaję. Nie mam takiego określonego schematu - zadaniem tego obiektu jest XXX, więc nie może on robić YYY. Jak sobie z tym poradzić?

Dobrze Ci się wydaje, programowanie to działalność empiryczna, bazująca na próbowaniu i zbieraniu doświadczeń.

Innym przykładem będzie to, że nie wiem, czy powinienem zrobić folder Extensions i tam umieszczać klasy StringExtensions, DateTimeExtensions.. czy może zrobić folder Helpers i tam umieścić klasę Extensions, w której miałbym wszystkie rozszerzenia..

No i jak wrzucisz wszystkie rozszerzenia do jednej klasy, to ile będzie ona miała linijek? Łatwo będzie się nawigowało i edytowało taką klasę?
Taka klasa będzie wielka i uciązliwa w edycji, więc lepiej mieć rozszerzenia dla różnych typów (a nawet różnych celów dla jednego typu) w oddzielnych klasach.

Co do głównego problemu, to to co masz już jest zbyt skomplikowane. Ty potrzebujesz po pierwsze pojemnika opisującego dane do zalogowania:

class LogItem
{
  public string Message { get; }
  public LogType Type { get; }
  public bool WithTimestamp { get; }
  // tutaj konstruktor
}

Po drugie możliwości zapisu do miejsca docelowego:

interface ILogTarget
{
  void Write(LogItem logItem);
}

Zaimplementuj ten interfejs w klasie ListViewLogTarget i w najprostszym rozwiązaniu to wszystko.
Żadne LogMessageCreatorOptions nie są potrzebne, bo możesz po prostu utworzyć obiekt LogItem, który będzie zawierał już wszystkie informacje. A jak będziesz chciał dodać zapis do pliku, bazy, maila, textboxa, czy czegokolwiek to będziesz musiał tylko napisać nową implementację ILogTarget.

Wydaje mi się że lepiej skupić się na tym co chcesz zrobić, a nie na samym kodzie - jeśli chcesz na szybko zrobić logger to na szybko wyklep bez myślenia wersje 1 - logger najczęściej służy tylko do debuggowania gdy nie wiadomo co w jakiej kolejności się wykonało i często jest niepotrzebny jak skończysz projekt. Jak stwierdzisz że potrzebujesz lepszą abstrakcję to zastanów się co chcesz zrobić:
np.: możliwość logowania bez znaczenia czy logujesz na UI, w konsoli czy do pliku

jak zdefiniujesz problem w ten sposób to robisz API do loggera jak najprościej się da i gdzieś jako argument definiujesz odbiorce loga. Odbiorce loga zaimplementuj na co najmniej 2 sposoby - nie rób nigdy abstrakcji jeśli jej nie potrzebujesz w danym momencie - skup się na funkcjach a nie na klasach.

I oczywiście prawie wszystko co wymyślisz ktoś już zrobił - poszukaj gotowca - na dłuższą metę zaoszczędzisz czasu