Code Review Please

BitLost

Active member
Joined
Dec 10, 2016
Messages
35
Programming Experience
Beginner
I have been slowly creating my first stand alone application. I have leaned heavily on the Contosso University net core example adapting this to suit what I am setting out to achieve.

Whilst this has worked and got to the 'end result' I set out with, prior to moving it the next step up where i start to really depart from the example I would appreciate some feedback and advice on some things I think logical and I am unclear on the direction.

Site controller extract:

C#:
[COLOR=blue][FONT=Consolas]public[/FONT][/COLOR][COLOR=black][FONT=Consolas] [/FONT][/COLOR][COLOR=blue][FONT=Consolas]async[/FONT][/COLOR][COLOR=black][FONT=Consolas] [/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]Task[/FONT][/COLOR][COLOR=black][FONT=Consolas]<[/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]IActionResult[/FONT][/COLOR][COLOR=black][FONT=Consolas]> Index([/FONT][/COLOR][COLOR=blue][FONT=Consolas]string[/FONT][/COLOR][COLOR=black][FONT=Consolas] sortOrder, [/FONT][/COLOR][COLOR=blue][FONT=Consolas]string[/FONT][/COLOR][COLOR=black][FONT=Consolas] currentFilter, [/FONT][/COLOR][COLOR=blue][FONT=Consolas]string[/FONT][/COLOR][COLOR=black][FONT=Consolas] searchString, [/FONT][/COLOR][COLOR=blue][FONT=Consolas]int[/FONT][/COLOR][COLOR=black][FONT=Consolas]? page)[/FONT][/COLOR]        {
 
            ViewData[[COLOR=#a31515]"CurrentSort"[/COLOR]] = sortOrder;
 
            ViewData[[COLOR=#a31515]"SiteSortParm"[/COLOR]] = [COLOR=#2b91af]String[/COLOR].IsNullOrEmpty(sortOrder) ? [COLOR=#a31515]"site_desc"[/COLOR] : [COLOR=#a31515]""[/COLOR];
 
            ViewData[[COLOR=#a31515]"SubSortParm"[/COLOR]] = sortOrder == [COLOR=#a31515]"Sub"[/COLOR] ? [COLOR=#a31515]"sub_desc"[/COLOR] : [COLOR=#a31515]"Sub"[/COLOR];
 
            ViewData[[COLOR=#a31515]"FactSortParm"[/COLOR]] = sortOrder == [COLOR=#a31515]"Fact"[/COLOR] ? [COLOR=#a31515]"fact_desc"[/COLOR] : [COLOR=#a31515]"Fact"[/COLOR];
 
            [COLOR=blue]if[/COLOR] (searchString != [COLOR=blue]null[/COLOR])
            {
                page = 1;
            }
            [COLOR=blue]else[/COLOR]
            {
                searchString = currentFilter;
            }
 
            [COLOR=blue]var[/COLOR] site = [COLOR=blue]from[/COLOR] s [COLOR=blue]in[/COLOR] _context.Sites
                        .Include(s => s.SiteType)
                       [COLOR=blue]select[/COLOR] s;
 
            ViewData[[COLOR=#a31515]"CurrentFilter"[/COLOR]] = searchString;
 
            
 
            [COLOR=blue]if[/COLOR] (![COLOR=#2b91af]String[/COLOR].IsNullOrEmpty(searchString))
            {
                site = site.Where(s => s.SiteName.Contains(searchString));
            }
 
 
            [COLOR=blue]switch[/COLOR] (sortOrder)
            {
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"site_desc"[/COLOR]:
                   site = site.OrderByDescending(s => s.SiteName);
                    [COLOR=blue]break[/COLOR];
 
                
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"Sub"[/COLOR]:
                    site = site.OrderBy(s => s.SiteSuburb);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"sub_desc"[/COLOR]:
                    site =site.OrderByDescending(s => s.SiteSuburb);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"Fact"[/COLOR]:
                    site = site.OrderBy(s => s.SiteType.SType);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"fact_desc"[/COLOR]:
                    site = site.OrderByDescending(s => s.SiteType.SType);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]default[/COLOR]:
                    site = site.OrderBy(s => s.SiteName);
                    [COLOR=blue]break[/COLOR];
 
            }
 
            [COLOR=blue]int[/COLOR] pageSize = 7;
  [COLOR=black][FONT=Consolas]            [/FONT][/COLOR][COLOR=blue][FONT=Consolas]return[/FONT][/COLOR][COLOR=black][FONT=Consolas] View([/FONT][/COLOR][COLOR=blue][FONT=Consolas]await[/FONT][/COLOR][COLOR=black][FONT=Consolas] [/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]PaginatedList[/FONT][/COLOR][COLOR=black][FONT=Consolas]<[/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]Site[/FONT][/COLOR][COLOR=black][FONT=Consolas]>.CreateAsync(site.AsNoTracking(), page ?? 1, pageSize));[/FONT][/COLOR]

Water Bodies controller extract:

C#:
[COLOR=blue][FONT=Consolas]public[/FONT][/COLOR][COLOR=black][FONT=Consolas] [/FONT][/COLOR][COLOR=blue][FONT=Consolas]async[/FONT][/COLOR][COLOR=black][FONT=Consolas] [/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]Task[/FONT][/COLOR][COLOR=black][FONT=Consolas]<[/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]IActionResult[/FONT][/COLOR][COLOR=black][FONT=Consolas]> Index([/FONT][/COLOR][COLOR=blue][FONT=Consolas]string[/FONT][/COLOR][COLOR=black][FONT=Consolas] sortOrder, [/FONT][/COLOR][COLOR=blue][FONT=Consolas]string[/FONT][/COLOR][COLOR=black][FONT=Consolas] currentFilter,[/FONT][/COLOR][COLOR=blue][FONT=Consolas]string[/FONT][/COLOR][COLOR=black][FONT=Consolas] searchString, [/FONT][/COLOR][COLOR=blue][FONT=Consolas]int[/FONT][/COLOR][COLOR=black][FONT=Consolas]? page)[/FONT][/COLOR]        {
            ViewData[[COLOR=#a31515]"CurrentSort"[/COLOR]] = sortOrder;
 
            ViewData[[COLOR=#a31515]"SiteSortParm"[/COLOR]] = [COLOR=#2b91af]String[/COLOR].IsNullOrEmpty(sortOrder) ? [COLOR=#a31515]"site_desc"[/COLOR] : [COLOR=#a31515]""[/COLOR];
 
            ViewData[[COLOR=#a31515]"ConSortParm"[/COLOR]] = sortOrder==[COLOR=#a31515]"Const"[/COLOR] ? [COLOR=#a31515]"construction_desc"[/COLOR] : [COLOR=#a31515]"Const"[/COLOR];
 
            ViewData[[COLOR=#a31515]"PTSortParm"[/COLOR]] = sortOrder == [COLOR=#a31515]"PT"[/COLOR] ? [COLOR=#a31515]"PT_desc"[/COLOR] : [COLOR=#a31515]"PT"[/COLOR];
 
            ViewData[[COLOR=#a31515]"AreaSortParm"[/COLOR]] = sortOrder == [COLOR=#a31515]"Area"[/COLOR] ? [COLOR=#a31515]"Area_desc"[/COLOR] : [COLOR=#a31515]"Area"[/COLOR];
 
            ViewData[[COLOR=#a31515]"VolSortParm"[/COLOR]] = sortOrder == [COLOR=#a31515]"Vol"[/COLOR] ? [COLOR=#a31515]"Vol_desc"[/COLOR] : [COLOR=#a31515]"Vol"[/COLOR];
 
 
            [COLOR=blue]if[/COLOR] (searchString != [COLOR=blue]null[/COLOR])
            {
                page = 1;
            }
 
            [COLOR=blue]else[/COLOR]
            {
                searchString = currentFilter;
            }
 
            ViewData[[COLOR=#a31515]"CurrentFilter"[/COLOR]] = searchString;
 
            [COLOR=blue]var[/COLOR] wb = [COLOR=blue]from[/COLOR] w [COLOR=blue]in[/COLOR] _context.WaterBodys
                     .Include(w => w.Construction)
                     .Include(w => w.Location)
                     .Include(w => w.PoolType)
                     .Include(w => w.Site)
                     [COLOR=blue]select[/COLOR] w;
 
            [COLOR=green]//var sites = from s in _context.Sites[/COLOR]
                          [COLOR=green]//select s;[/COLOR]
 
            [COLOR=blue]if[/COLOR] (![COLOR=#2b91af]String[/COLOR].IsNullOrEmpty(searchString))
            {
                wb = wb.Where(s => s.Site.SiteName.Contains(searchString));
            }
 
 
            [COLOR=blue]switch[/COLOR] (sortOrder)
            {
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"site_desc"[/COLOR]:
                    wb = wb.OrderByDescending(s => s.Site.SiteName);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"construction_desc"[/COLOR]:
                    wb = wb.OrderByDescending(s => s.Construction.iConstruction);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"Const"[/COLOR]:
                    wb = wb.OrderBy(s => s.Construction.iConstruction);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"PT"[/COLOR]:
                    wb = wb.OrderBy(s => s.PoolType.iPoolType);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"PT_desc"[/COLOR]:
                    wb = wb.OrderByDescending(s => s.PoolType.iPoolType);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"Area"[/COLOR]:
                    wb = wb.OrderBy(s => s.PoolArea);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"Area_desc"[/COLOR]:
                    wb = wb.OrderByDescending(s => s.PoolArea);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"Vol"[/COLOR]:
                    wb = wb.OrderBy(s => s.PoolVolume);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]case[/COLOR] [COLOR=#a31515]"Vol_desc"[/COLOR]:
                    wb = wb.OrderByDescending(s => s.PoolVolume);
                    [COLOR=blue]break[/COLOR];
 
                [COLOR=blue]default[/COLOR]:
                    wb = wb.OrderBy(s => s.Site.SiteName);
                    [COLOR=blue]break[/COLOR];
 
            }
 
           
            [COLOR=blue]int[/COLOR] pageSize = 7;
  [COLOR=black][FONT=Consolas]            [/FONT][/COLOR][COLOR=blue][FONT=Consolas]return[/FONT][/COLOR][COLOR=black][FONT=Consolas] View([/FONT][/COLOR][COLOR=blue][FONT=Consolas]await[/FONT][/COLOR][COLOR=black][FONT=Consolas] [/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]PaginatedList[/FONT][/COLOR][COLOR=black][FONT=Consolas]<[/FONT][/COLOR][COLOR=#2B91AF][FONT=Consolas]WaterBody[/FONT][/COLOR][COLOR=black][FONT=Consolas]>.CreateAsync(wb.AsNoTracking(), page ?? 1, pageSize));[/FONT][/COLOR]

Both these are very similar in structure and to a large extent function.

First question is this code efficient and if not why not?

Second question. Both Indexes perform searches. They both have a similar structure in the way they search. Why do I do it this way? Perhaps I am overthinking however in pseudo code couldn't I do something like:

public void ThisIsSort (string sortString)

ViewData[sortString] = sortOrder==sortString ? sortString+"_desc" : sortString;

switch (sortOrder)
{
case "site_desc":
wb = wb.OrderByDescending(s => s.Site.SiteName);
break;

case "construction_desc":
wb = wb.OrderByDescending(s => s.Construction.iConstruction);
break;

etc etc...

My thinking being I have one sort system for the entire site and all sorts call this one 'class' saving me debugging and so on. I am not sure how to call out to another 'class' but feel it cant be that hard. And if I did it this way would it be in a class and if so where in views in viewmodels or models?
 
Back
Top Bottom