LINQ refactoring query

LINQ refactoring query
Aenyatia
Aenyatia
Aenyatia
Aenyatia
  • Rejestracja:prawie 7 lat
  • Ostatnio:ponad 3 lata
  • Postów:35
0

Na głównej stronie projektu chcę wyświetlić podsumowanie użytkownikowi związane z jego zadaniami. Mam taki kod, spełnia on swoje zadanie ale według mnie nie wygląda on dobrze. Jak można go poprawić? Będę wdzięczny za jakieś wzorce, techniki itp. Wydaje mi się, że coś w Where i Select trzeba zmienić bo za dużo jest duplikacji. Nie wiem czy ToList() dobrze użyłem, teraz będą 4 zapytania do bazy danych, można to jakoś zoptymalizować?

Kopiuj
var summaryViewModel = new SummaryViewModel
{
	TodayTasks = _context.Tasks
		.Where(t => t.DueDate.Date == DateTime.Today.Date &&
					t.IsCompleted == false &&
					t.UserId == userId)
		.OrderByDescending(t => t.Priority)
		.Take(5)
		.Select(t => new TaskViewModel
		{
			Id = t.Id,
			Name = t.Name,
			DueDate = t.DueDate.ToLongDateString(),
			Priority = t.Priority.ToString(),
			Category = t.Category.Name
		})
		.ToList(),

	TomorrowTasks = _context.Tasks
		.Where(t => t.DueDate.Date == DateTime.Today.AddDays(1).Date &&
					t.IsCompleted == false &&
					t.UserId == userId)
		.OrderByDescending(t => t.Priority)
		.Take(5)
		.Select(t => new TaskViewModel
		{
			Id = t.Id,
			Name = t.Name,
			DueDate = t.DueDate.ToLongDateString(),
			Priority = t.Priority.ToString(),
			Category = t.Category.Name
		})
		.ToList(),

	LaterTasks = _context.Tasks
		.Where(t => t.DueDate.Date > DateTime.Today.AddDays(1).Date &&
					t.IsCompleted == false &&
					t.UserId == userId)
		.OrderBy(t => t.DueDate.Date)
		.ThenByDescending(t => t.Priority)
		.Take(5)
		.Select(t => new TaskViewModel
		{
			Id = t.Id,
			Name = t.Name,
			DueDate = t.DueDate.ToLongDateString(),
			Priority = t.Priority.ToString(),
			Category = t.Category.Name
		})
		.ToList(),

	RecentlyCompletedTasks = _context.Tasks
		.Where(t => t.IsCompleted &&
					t.UserId == userId)
		.OrderBy(t => t.CompletedAt.Value)
		.Take(5)
		.Select(t => new TaskViewModel
		{
			Id = t.Id,
			Name = t.Name,
			DueDate = t.DueDate.ToLongDateString(),
			Priority = t.Priority.ToString(),
			Category = t.Category.Name
		})
		.ToList()
};
"Simplicity is the Ultimate Sophistication"
Udostępnij
Kopiuj link do postu Kopiuj link do postu jako Markdown
Komentuj
WeiXiao
WeiXiao
WeiXiao
WeiXiao
  • Rejestracja:około 9 lat
  • Ostatnio:3 minuty
  • Postów:5138
0

Generalnie tak:

Rozbiłbym to na metody,

Wszystkie te selecty bym wrzucił do jednej metody

A całą resztę dałoby się bardzo ładnie skrócić gdyby nie te Ordery, bo warunki w Where spokojnie możesz dynamicznie podawać.

Jak wymyślisz jak podawać te Ordery jako parametry bez zabawy z czymś, co pogorszy przejrzystość tego kodu (np. refleksję lub dodatkowa metoda) to mamy to :P

edytowany 5x, ostatnio: WeiXiao
Udostępnij
Kopiuj link do postu Kopiuj link do postu jako Markdown
Komentuj
szydlak
SZ
szydlak
SZ
  • Rejestracja:prawie 11 lat
  • Ostatnio:około godziny
  • Postów:1494
0

A ja bym raz pobrał wszystkie Taski w jednym zapytaniu (warunek z userem oraz datą) a potem filtrował w pamięci już.

WeiXiao
pójście na łatwiznę, noob!!! :D
Aenyatia
też myślałem o tym i w moim przypadku to nawet by mogło się sprawdzić bo nie powinno być dużo danych do pobrania ale to nie jest chyba dobra praktyka pobierać wszystko z bazy danych a potem filtrować
WeiXiao
@Aenyatia: Jeżeli masz relatywnie mało danych, to szkoda tracić czas na pierdoły, bo ten poświęcony czas pewnie nie zwróci się przez najbliższe X lat.
SZ
Nie wszystkie które są w bazie tylko w jednym zapytaniu wszystkie które są potrzebne
WeiXiao
@szydlak: No czyli wszystko z dzisiaj i później.
Udostępnij
Kopiuj link do postu Kopiuj link do postu jako Markdown
Komentuj
szalonyfacet
szalonyfacet
szalonyfacet
szalonyfacet
  • Rejestracja:ponad 12 lat
  • Ostatnio:10 miesięcy
  • Lokalizacja:Dąbrowa Górnicza
1

Czy pobierzesz sobie od strzala wszystkie Taski powyzej dzisiejszego czy dasz metody do budowy Iqueryable to juz kwestia dyskusyjna czy warto czy nie, ale generowanie modelu to sobie opakujw metode a unikniesz powtorzen i lamania zasady DRY cos takiugo np:

Kopiuj
public IList<Task> GetTaskViewModel(IQueryable<Task>tasks, int noOfRecords = 5)
    {
        return tasks
        .Take(noOfRecords)
        .Select(t => new TaskViewModel
        {
            Id = t.Id,
            Name = t.Name,
            DueDate = t.DueDate.ToLongDateString(), 
            Priority = t.Priority.ToString(),
            Category = t.Category.Name
        })
        .ToList()
    }

I moje pytanie po co konwertujesz na stringi?? trzymnaj w modelu oryginalny typ a konwertuj na sam koniec ew pozwol na automatyczny casting.

edytowany 1x, ostatnio: szalonyfacet
Aenyatia
Priority jest enumem ale będę go zamieniał na klasę prawdopodobnie i nie chcę, żeby szczegóły modelu przedostały się do widoku.
szalonyfacet
to w takim arzie bedzie to niewydajne. Enum.ToString(0 nie zadziala w SQL. bedziesz musial wymusic materializacje wczesniej, a jesli nie wywala bledu teraz to wymuszasz ja juz, wiec cala idea wydajnosci idzie sie walic]
PI
to jakaś dupa, a nie refactoring.
Udostępnij
Kopiuj link do postu Kopiuj link do postu jako Markdown
Komentuj
Aenyatia
Aenyatia
Aenyatia
Aenyatia
  • Rejestracja:prawie 7 lat
  • Ostatnio:ponad 3 lata
  • Postów:35
0

Przeglądam kod i znalazłem w innym miejscu takie coś i znowu mam powtórzenie warunków @WeiXiao czyli tu proponujesz coś a la specification pattern i zwracać expression

Kopiuj
return View("TasksViewComponent", new NumberOfTasksViewModel
{
	Today = _context.Tasks.Count(t => t.DueDate.Date == DateTime.Today.Date &&
									  t.IsCompleted == false &&
									  t.UserId == userId),

	Tomorrow = _context.Tasks.Count(t => t.DueDate.Date == DateTime.Today.AddDays(1).Date &&
										 t.IsCompleted == false &&
										 t.UserId == userId),

	Later = _context.Tasks.Count(t => t.DueDate.Date > DateTime.Today.AddDays(1).Date && 
	                                  t.IsCompleted == false &&
	                                  t.UserId == userId),

	NotDone = _context.Tasks.Count(t => t.DueDate.Date < DateTime.Today.Date && 
	                                    t.IsCompleted == false &&
	                                    t.UserId == userId),

	History = _context.Tasks.Count(t => t.IsCompleted &&
	                                    t.UserId == userId)
});
"Simplicity is the Ultimate Sophistication"
Udostępnij
Kopiuj link do postu Kopiuj link do postu jako Markdown
Komentuj
WeiXiao
WeiXiao
WeiXiao
WeiXiao
  • Rejestracja:około 9 lat
  • Ostatnio:3 minuty
  • Postów:5138
1

@Aenyatia:

Chociaż teraz to aż tak wiele nie skróci :P

Kopiuj
public List<TaskViewModel> GetSpecificTasksCount(Expression<Func<Task, bool>> func)
{
	return _context.Tasks.Where(func).Count();
}

Gdyby jeszcze trzeba było tego Selecta walnąć do zmiany, to byłoby fajniej :P

Kopiuj
public static List<TaskViewModel> Simplify(List<Task> tasks)
{
	return tasks.Select(t => new TaskViewModel
	{
		Id = t.Id,
		Name = t.Name,
		DueDate = t.DueDate.ToLongDateString(),
		Priority = t.Priority.ToString(),
		Category = t.Category.Name
	}).ToList();
}
edytowany 11x, ostatnio: WeiXiao
mad_penguin
mad_penguin
jak już to nie Func<Task, bool> tylko Expression<Func<Task, bool>>! bo w obecnej wersji będzie filtrować całą tabelę w pamięci ;)
WeiXiao
@mad_penguin: oo? z tego co czytałem, to expression tylko do debugowania podobno. poka bencha :)
WeiXiao
@mad_penguin: Masz rację :) @Aenyatia Popraw sobie :D
Udostępnij
Kopiuj link do postu Kopiuj link do postu jako Markdown
Komentuj
szalonyfacet
szalonyfacet
szalonyfacet
szalonyfacet
  • Rejestracja:ponad 12 lat
  • Ostatnio:10 miesięcy
  • Lokalizacja:Dąbrowa Górnicza
1

ojej, tu jest troche mieszanie warstw. Stworz sobie jakis serwis ktory bedzie zaciagal z repozytorium te dane. Bo na chwile obecna w kontrolerze trzymasz referencje do kontekstu bazodanowego. A co jesli na pewnej stronie zaraz bedziesz potrzebowal liczbe samych taskow ktore sa NotDone. odwolasz sie do tej samej metody kontrolera ktora pobiera wszytsko?? czy bedziesz duplikowal kod piszac jesczze raz w kontrolerze zaciaganie taskow do modelu?? Wprowadz wiecej wartsw. przynajmniej serwis dla samych metod do wyciagania danych z bazy i jakies repo do kontaktu z baza bo za chwile bedziesz mial kilkanascie miejsc gdzie kod bedzie podobny bo wszystko bedzie upcahne w kontrolerach.

Aenyatia
zgadzam się z Tobą, mam zamiar dodać warstwę serwisów w których będę korzystał z contextu po porostu na początku nie chciałem sobie "komplikować" bo nie miałem dokładnej wizji co chcę stworzyć i prościej mi było logikę w kontrolerze robić i od razu testować a dodatkowa warstwa to trochę więcej pracy
Udostępnij
Kopiuj link do postu Kopiuj link do postu jako Markdown
Komentuj

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.

Utwórz konto