LINQ refactoring query

Odpowiedz Nowy wątek
2018-11-05 16:10
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ć?

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()
};

Pozostało 580 znaków

2018-11-05 18:37
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, 2018-11-05 18:41

Pozostało 580 znaków

2018-11-05 18:42
0

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

pójście na łatwiznę, noob!!! :D - WeiXiao 2018-11-05 18:43
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ć - Aenyatia 2018-11-05 18:47
@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. - WeiXiao 2018-11-05 18:49
Nie wszystkie które są w bazie tylko w jednym zapytaniu wszystkie które są potrzebne - szydlak 2018-11-05 19:00
@szydlak: No czyli wszystko z dzisiaj i później. - WeiXiao 2018-11-05 19:01

Pozostało 580 znaków

2018-11-05 18:54
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:

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, 2018-11-05 18:55
Priority jest enumem ale będę go zamieniał na klasę prawdopodobnie i nie chcę, żeby szczegóły modelu przedostały się do widoku. - Aenyatia 2018-11-05 19:11
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] - szalonyfacet 2018-11-05 19:13
to jakaś dupa, a nie refactoring. - Pietass 2018-11-06 00:25

Pozostało 580 znaków

2018-11-05 19:32
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

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)
});

Pozostało 580 znaków

2018-11-05 19:37

@Aenyatia:

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

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

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, 2018-11-06 00:05
jak już to nie Func<Task, bool> tylko Expression<Func<Task, bool>>! bo w obecnej wersji będzie filtrować całą tabelę w pamięci ;) - mad_penguin 2018-11-05 22:30
@mad_penguin: oo? z tego co czytałem, to expression tylko do debugowania podobno. poka bencha :) - WeiXiao 2018-11-05 22:30
@mad_penguin: Masz rację :) @Aenyatia Popraw sobie :D - WeiXiao 2018-11-06 00:05

Pozostało 580 znaków

2018-11-05 19:43
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.

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 - Aenyatia 2018-11-05 20:02

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0