Proszę o ocenę kodu napisanego w C# + Entity + WPF
https://github.com/OrientMantis24/BloodBankManagementSoftware

Moje pytania:

1. w DonatorsDatabase.GetDonator(string, string, string) mam aż 7 ifów. Jak zrobić to przejrzyściej i prościej?
2. Czy w FoundDonatorEntries powinno się użyć różnych konstruktorów czy jakoś inaczej?

Dzięki!

0. Źle używasz EF. Powinieneś użyć LINQ, żeby EF wygenerował wydajne zapytanie, a nie odczytywać za każdym razem wszystko i ręcznie sprawdzać.
1. W tej drabinie ifów przynajmniej trochę lepiej byłoby, gdyby do pustych argumentów przekazywać null zamiast jakichś dziwnych stringów typu "-" i sprawdzać np tak

var donors = context.Donors.All();
if (Name != null)
    donors = donors.Where(x => x.Name == Name);
if (Town != null)
    donors = donors.Where(x => x.Town == Town);
if (Phone != null)
    donors = donors.Where(x => x.Phone == Phone);
return donors.ToList();

W ten sposób przy wywołaniu ToList() zostanie wygenerowane zapytanie i sprawdzaniem warunków zajmie się baza danych. Wydaje mi się w ogóle, że ta funkcja powinna wyglądać inaczej, być bardziej podzielona czy coś, ale mam za małe doświadczenie żeby coś poradzić.
2. W ogóle to powinieneś mieć jakieś klasy na wyniki wyszukiwań albo używać choćby List, a do ich wyświetlenia użyć bindingu zamiast ręcznie przypisywać ItemSource.

Dodatkowo:
Funkcje zwracające listy powinny mieć nazwy w liczbie mnogiej.
Zamiast FindDonator OP = new FindDonator(); można użyć var OP = new FindDonator();, mniej tekstu a czytelność nie na tym nie cierpi.

Tyle ode mnie, ktoś bardziej doświadczony na pewno jeszcze coś uzupełni ;)

Po co takie cuda:

List<Donator> Donators = new List<Donator>();
foreach (var Donator in DonatorsDatabase.GetDonator(SearchedName, SearchedTown, SearchedPhoneNumber))
{
      Donators.Add(Donator);
}
LVDonators.ItemsSource = Donators;

nie można po prostu:

LVDonators.ItemsSource = DonatorsDatabase.GetDonator(SearchedName, SearchedTown, SearchedPhoneNumber);

Jeśli chodzi o Twoje pierwsze pytanie, to zauważyłem że masz dużo razy

DonatorList.Add(Donator);

więc całą tą metodę rozbiłem na dwie:

• Jedna która iteruje po liście i dodaje do innej
• Druga która sprawdza czy element powinien być dodany do listy.

Kroki które przedsięwziąłem, są co następuje:

• Przeniosłem te ify do innej metody która tylko mówi "Dodaj albo nie dodaj" (czyli return true/false), a już ta pierwsza zajmuje się dodawaniem
• Skoro korzystam z returna, to i tak wyjdę z funkcji czyli else'y są nie potrzebne
• Wyciągnąłem te porównania do zmiennych żeby kod był chociaż trochę bardziej czytelny

public static List<Donator> GetDonator(string Name, string Town, string PhoneNumber)
{
	List<Donator> DonatorList = new List<Donator>();

	using (var context = new DonatorContext())
	{
		foreach (var Donator in context.Donators)
		{
			if (shouldDonatorBeAdded(Name,Town,PhoneNumber,Donator)) 
			{
				DonatorList.Add(Donator);
			}
		}
	}
	return DonatorList;
}

private static shouldDonatorBeAdded(string Name, string Town, string PhoneNumber, Donator Donator)
{
	bool TownIsNotEmpty = Town != "";
	bool PhoneNumberIsNotEmpty = PhoneNumber != "___-___-___";
	bool NameIsNotEmpty = Name != "";
	
	bool TownIsEmpty = Town == "";
	bool PhoneNumberIsEmpty = PhoneNumber == "___-___-___";
	bool NameIsEmpty = Name == "";
	
	
	if (NameIsNotEmpty && TownIsNotEmpty && PhoneNumberIsNotEmpty)
	{
		return (Donator.Name == Name && Donator.Town == Town && Donator.PhoneNumber == PhoneNumber); 
	}
	
	if (NameIsNotEmpty && TownIsNotEmpty && PhoneNumberIsEmpty)
	{
		return (Donator.Name == Name && Donator.Town == Town)
	}
	
	if (NameIsNotEmpty && TownIsEmpty && PhoneNumberIsNotEmpty)
	{
		return (Donator.Name == Name && Donator.PhoneNumber == PhoneNumber);
	}
	
	
	if (NameIsEmpty && TownIsNotEmpty && PhoneNumberIsEmpty)
	{
		return (Donator.PhoneNumber == PhoneNumber && Donator.Town == Town);
	}
	
	if (NameIsEmpty && TownIsEmpty && PhoneNumberIsNotEmpty)
	{
		return (Donator.PhoneNumber == PhoneNumber);
	}
	
	
	if (NameIsNotEmpty && TownIsEmpty && PhoneNumberIsEmpty)
	{
		return (Donator.Name == Name);
	}
	
	if (NameIsEmpty && TownIsEmpty && PhoneNumberIsEmpty)
	{
		return (Donator.Town == Town);
	}
	
	return false;
}

Żeby kod był bardziej przejżysty możesz zrobić to:

Rozbijasz środek każdego ifa do innej metody i nazywasz je np (kolejno)

1. AllFieldsFilled()
2. OnlyPhoneEmpty()
3. OnlyTownEmpty()
4. OnlyNameEmpty()
5. NameAndTownEmpty()
6. NameAndPhoneEmpty()
   i tak dalej

te porównania Donator.Name==Name && Donator.Town == Town zamieniłbym na

1. TownMatch() && NameMatch() albo coś takiego
   metody te można trzymać w tej klasie bo jest mała albo dodać do innej dla przejżystości.

Teraz jeżeli koniecznie chcesz się pozbyć ifów to masz dwie opcje (łatwiejsza i trudniejsza)

Pierwsza:

1. Robisz sobie zmienną (Twoje metody są statyczne więc zmienna niestety też będzie musiała być statyczna (źle! nie rób tak nigdy :D)) która nazywa się bool shouldAddDonator = false.

2. Każdego z Twoich 7dmiu ifów zamieniasz w taki sposób

if (NameIsNotEmpty && TownIsNotEmpty && PhoneNumberIsNotEmpty)
{
	return (Donator.Name == Name && Donator.Town == Town && Donator.PhoneNumber == PhoneNumber); 
}

na

shouldAddIfAllFieldsAreNotEmpty();
// ...
private void shouldAddIfAllFieldsAreNotEmpty() 
{
    // tutaj Twój if
    if (NameIsNotEmpty && TownIsNotEmpty && PhoneNumberIsNotEmpty)
    {
         shouldAddDonator = true;
    }
}

i tak z każdym ifem, teraz Twoich 7 brzydkich ifów jest 7dmioma wywołaniami metody.
Kiedy już to zrobisz możesz to zrobić ładniej i tych 7 metod oraz 7 ifów oraz zmienna 'shouldAddDonator' przenieść do innej klasy i wtedy już w ogóle będzie pięknie (nie ma statycznej zmiennej). Twój DonatorDatabase nagle skraca się do jednej pętli.

Druga:

Jeżeli jednak chciałbyś zrobić żeby ta klasa była w miarę elastyczna i żeby dodanie nowego pola lub warunku było w miarę proste to mógłbyś JESZCZE DO TEGO użyć polimorfizmu (jeżeli się podejmiesz i masz wrażenie że dasz radę i nie będzie to dla Ciebie skomplikowane). Jeżeli Cię to interesuje to skomentuj i wtedy opiszę dokładniej

Wiem że te porównania można by zamienić na czadowe lambdy i linq albo przeniesienie sprawdzania do bazy danych, ale zrobiłem to tak jakbym ja to zrobił bez korzystania ze specyfiki języka. Dziękuję.