Lite feedback på min kod, om någon orkar.

Permalänk

Lite feedback på min kod, om någon orkar.

Hej, jag är ny inom programmering och håller på med kursen Programmering 1 på gymnasienivå. Jag har fått feedback från min lärare att min kod innehåller mycket kod och är den blir "knölig". Jag försökte göra den mindre i en tidigare uppgift men även den tyckte läraren var "knölig". Han vill att jag kommer på handledning men pga. att jag jobbar nästan 100% i butik samtidigt så hinner jag inte på de tider han har.

Hur som haver så ska sista inlämningsuppgiften in snart och jag har försökt så gott jag kan att göra den så lätt att läsa som möjligt. Är det någon som orkar kolla igenom och ge mig lite konstruktiv kritik och feedback på vad jag kan förbättra i koden? Skulle uppskattas massor. Är inte klar med allt än men det är åt det här hållet jag tänkt mig. (Jag har inte lagt in några kommentarer än.)

Ber om ursäkt på förhand ifall jag inte får till

markeringarna direkt. Det är flera filer kod och jag är även ny på denna hemsida.

//Fil: main.cs (Detta står ej med i koden.) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class Program { public static void Main (string[] args) { //Console.Clear(); Console.WriteLine("Hi, welcome to the Buss-Simulator! \nPress enter to start."); Console.ReadKey(); var mybus = new Bus(); mybus.Run(); Console.ReadKey(); } }

//Fil: MyBus (detta står ej med i koden) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class Bus { public static int total_passengers = 0; public static Passenger[] passengers_info = new Passenger[25]; public static int totalSeats = 25; public void Run() { passengers_info = new Passenger[25]; string [] menu = new string[]{"1. Pick up passenger.", "2. Printing options.", "3. Calculation options.", "4. Searching options.", "0. Quit program."}; int MenuSelect = 0; while (true) { Console.Clear(); Console.WriteLine("What do you want to do?"); Console.WriteLine(); Console.CursorVisible = false; if (MenuSelect == 0) { Console.WriteLine(menu[0] + " ⭅"); Console.WriteLine(menu[1]); Console.WriteLine(menu[2]); Console.WriteLine(menu[3]); Console.WriteLine(menu[4]); } else if(MenuSelect == 1) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1] + " ⭅"); Console.WriteLine(menu[2]); Console.WriteLine(menu[3]); Console.WriteLine(menu[4]); } else if(MenuSelect == 2) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1]); Console.WriteLine(menu[2] + " ⭅"); Console.WriteLine(menu[3]); Console.WriteLine(menu[4]); } else if(MenuSelect == 3) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1]); Console.WriteLine(menu[2]); Console.WriteLine(menu[3] + " ⭅"); Console.WriteLine(menu[4]); } else if(MenuSelect == 4) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1]); Console.WriteLine(menu[2]); Console.WriteLine(menu[3]); Console.WriteLine(menu[4] + " ⭅"); } var keyPressed = Console.ReadKey(); if(keyPressed.Key == ConsoleKey.DownArrow && MenuSelect != menu.Length -1) { MenuSelect++; } else if (keyPressed.Key == ConsoleKey.UpArrow && MenuSelect >= 1) { MenuSelect--; } else if (keyPressed.Key == ConsoleKey.Enter) { switch (MenuSelect) { case 0: add_passengers(); break; case 1: PrintMenu.PrintingMenu(); break; case 2: CalculationMenu.CalculatingMenu(); break; case 3: SearchMenu.SearchingMenu(); break; case 4: System.Environment.Exit(1); break; } } } } public static void add_passengers() { Console.Clear(); if (total_passengers == 25) { Console.WriteLine("\nBus is full!"); System.Threading.Thread.Sleep(1500); return; } try { Console.WriteLine("\nType the name, age & gender of your passenger."); Console.Write("\nName: "); string name = Console.ReadLine(); Console.Write("\nAge: "); int age = Convert.ToInt32(Console.ReadLine()); Console.Write("\nGender (Male/Female): "); string gender = Console.ReadLine(); Passenger passenger = new Passenger(aName: name, aAge: age, aGender: gender); Array.Resize(ref passengers_info, passengers_info.Length + 1); passengers_info[passengers_info.Length - 1] = passenger; } catch { Console.WriteLine("\nFollow instructions."); System.Threading.Thread.Sleep(1500); return; } total_passengers++; Console.WriteLine("You boarded 1 Passenger." + "\nThere are " + (totalSeats - total_passengers) + " seats left."); System.Threading.Thread.Sleep(1500); return; } }

//Fil: PassengersObject (detta står ej med i koden) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class Passenger{ public string name { get; set; } public int age { get; set; } public string gender { get; set; } public Passenger(string aName, int aAge, string aGender) { name = aName; age = aAge; gender = aGender; } public override string ToString() { return string.Format($"{name}, {gender}, {age} years old."); } }

//Fil: PrintMenu (detta står ej med i koden) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class PrintMenu { public static void PrintingMenu() { string [] menu = new string[]{"1. See who's on the buss.", "2. See what genders are on the buss.", "3. Back out."}; int MenuSelect = 0; while (true) { Console.Clear(); Console.WriteLine("What do you want to do?"); Console.WriteLine(); Console.CursorVisible = false; if (MenuSelect == 0) { Console.WriteLine(menu[0] + " ⭅"); Console.WriteLine(menu[1]); Console.WriteLine(menu[2]); } else if(MenuSelect == 1) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1] + " ⭅"); Console.WriteLine(menu[2]); } else if(MenuSelect == 2) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1]); Console.WriteLine(menu[2] + " ⭅"); } var keyPressed = Console.ReadKey(); if(keyPressed.Key == ConsoleKey.DownArrow && MenuSelect != menu.Length -1) { MenuSelect++; } else if (keyPressed.Key == ConsoleKey.UpArrow && MenuSelect >= 1) { MenuSelect--; } else if (keyPressed.Key == ConsoleKey.Enter) { switch (MenuSelect) { case 0: Print.print_passengers(); break; case 1: Print.print_gender(); break; case 2: return; } } } } }

//Fil: PrintingMethods (detta står ej med i koden) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class Print { public static void print_passengers() { Console.Clear(); if (Bus.total_passengers == 0) { Console.WriteLine("\nThere are no passengers on the bus."); System.Threading.Thread.Sleep(1500); return; } Console.WriteLine("\nThese are your passengers."); Bus.passengers_info = Bus.passengers_info.Where(c => c != null).ToArray(); foreach (var i in Bus.passengers_info) { Console.WriteLine("\n" + i); } Console.WriteLine("\nPress enter to return."); Console.ReadKey(); } public static void print_gender() { Console.Clear(); if (Bus.total_passengers == 0) { Console.WriteLine("\nThere are no passengers on the bus."); System.Threading.Thread.Sleep(1500); return; } for (int i = Bus.passengers_info.GetLowerBound(0); i <= Bus.passengers_info.GetUpperBound(0); i++) { Console.WriteLine("\n [{0,2}]: {1}", i, Bus.passengers_info[i].gender); } Console.ReadKey(); } }

//Fil: SearchMenu (detta står ej med i koden) class SearchMenu { public static void SearchingMenu() { string [] menu = new string[]{"1. Search for oldest passenger.", "2. Search for passengers in certain age group.", "3. Back out."}; int MenuSelect = 0; while (true) { Console.Clear(); Console.WriteLine("What do you want to do?"); Console.WriteLine(); Console.CursorVisible = false; if (MenuSelect == 0) { Console.WriteLine(menu[0] + " ⭅"); Console.WriteLine(menu[1]); Console.WriteLine(menu[2]); } else if(MenuSelect == 1) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1] + " ⭅"); Console.WriteLine(menu[2]); } else if(MenuSelect == 2) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1]); Console.WriteLine(menu[2] + " ⭅"); } var keyPressed = Console.ReadKey(); if(keyPressed.Key == ConsoleKey.DownArrow && MenuSelect != menu.Length -1) { MenuSelect++; } else if (keyPressed.Key == ConsoleKey.UpArrow && MenuSelect >= 1) { MenuSelect--; } else if (keyPressed.Key == ConsoleKey.Enter) { switch (MenuSelect) { case 0: Search.find_maxAge(); break; case 1: Search.find_ageGroup(); break; case 2: return; } } } } }

//Fil: SearchingMethods (detta står ej med i koden) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class Search { public static void find_maxAge() { Console.Clear(); if (Bus.total_passengers == 0) { Console.WriteLine("\nThere are no passengers on the bus."); System.Threading.Thread.Sleep(1500); return; } var oldest = Bus.passengers_info.Max(p => p.age); var passenger = Bus.passengers_info.First(p => p.age == oldest); Console.WriteLine("\nThis is the oldest passenger on the bus: " + passenger); Console.ReadKey(); } public static void find_ageGroup() { Console.Clear(); if (Bus.total_passengers == 0) { Console.WriteLine("\nThere are no passengers on the bus."); System.Threading.Thread.Sleep(1500); return; } int lowest; int highest; try { do { Console.WriteLine("\nType in the lowest and the highest age you wanna search between."); Console.Write("\nLowest: "); lowest = Convert.ToInt32(Console.ReadLine()); Console.Write("\nHighest: "); highest = Convert.ToInt32(Console.ReadLine()); if (lowest > highest) { Console.WriteLine("\nStart with the lowest age."); } }while (lowest > highest); } catch { Console.WriteLine("Error! Try using numbers."); return; } Console.WriteLine("\nThese are the passengers between the chosen age groups."); Console.ReadKey(); } }

//Fil: CalculationMenu (detta står ej med i koden) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class CalculationMenu { public static void CalculatingMenu() { string [] menu = new string[]{"1. Calculate the combined age of the passengers.", "2. Calculate the average age of the passengers.", "3. Back out."}; int MenuSelect = 0; while (true) { Console.Clear(); Console.WriteLine("What do you want to do?"); Console.WriteLine(); Console.CursorVisible = false; if (MenuSelect == 0) { Console.WriteLine(menu[0] + " ⭅"); Console.WriteLine(menu[1]); Console.WriteLine(menu[2]); } else if(MenuSelect == 1) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1] + " ⭅"); Console.WriteLine(menu[2]); } else if(MenuSelect == 2) { Console.WriteLine(menu[0]); Console.WriteLine(menu[1]); Console.WriteLine(menu[2] + " ⭅"); } var keyPressed = Console.ReadKey(); if(keyPressed.Key == ConsoleKey.DownArrow && MenuSelect != menu.Length -1) { MenuSelect++; } else if (keyPressed.Key == ConsoleKey.UpArrow && MenuSelect >= 1) { MenuSelect--; } else if (keyPressed.Key == ConsoleKey.Enter) { switch (MenuSelect) { case 0: int totalAge = Calculations.calc_total_age(); Console.WriteLine("\nThe combined age of the passengers is: " + totalAge + " years.\nPress enter to return."); Console.ReadKey(); break; case 1: double avrgAge = Calculations.calc_average_age(); Console.WriteLine("\nThe average age of the passengers is: " + avrgAge + " years.\nPress enter to return."); Console.ReadKey(); break; case 2: return; } } } } }

//Fil: CalculationsMethods (detta står ej med i koden) using System; using System.Linq; using System.Text; using System.Collections.Generic; using System.Threading.Tasks; class Calculations { public static int calc_total_age() { int totalAge = 0; Bus.passengers_info = Bus.passengers_info.Where(c => c != null).ToArray(); for (int i = 0; i < Bus.total_passengers; i++) { totalAge += Bus.passengers_info[i].age; } return totalAge; } public static double calc_average_age() { double avrgAge = 0.0; Bus.passengers_info = Bus.passengers_info.Where(c => c != null).ToArray(); for (int i = 0; i < Bus.total_passengers; i++) { avrgAge += (Bus.passengers_info[i].age / Convert.ToDouble(Bus.total_passengers)); } return avrgAge; } }

Permalänk

Alla dina menuselects skulle bli mer läsbara med en switch.
Du skriver ut ungefär samma sak hela tiden i menyn, så att bygga en metod printMenu(selectedMenuOption int) skulle spara mycket rader. Man vill få bort all utfyllnad så man kan få en översikt av hur logiken fungerar. Detaljerna ser man in de olika funktionerna istället.

Permalänk
Medlem

Du har flera stora if-satser som skriver ut menyer, där enda skillnaden är var ⭅ läggs till någonstans. Du kan byta ut dessa mot for-loopar istället, som skriver ut varje menyrad och om loop-indexet matchar MenySelect så lägger du till ⭅ på den raden.

Koden blir då inte heller beroende av hur många menyalternativ det finns, så du kan med fördel flytta koden till en metod som du kan anropa överallt istället. D.v.s. istället för en stor if-sats på varje ställe du vill skriva ut en meny på så blir det bara t.ex. printMenu(menu, MenuSelect).

Du blandar även olika namnsystem för variabler och metoder: snake_case, lowerCamelCase, samt UpperCamelCase (a.k.a. PascalCase). Det gör att koden blir mer rörig att läsa eftersom man inte direkt ser vad som är vad, så försök att vara lite mer konsekvent (se t.ex. Microsofts rekommendationer för namngivning).

Permalänk
Skrivet av blegnitur:

Alla dina menuselects skulle bli mer läsbara med en switch.
Du skriver ut ungefär samma sak hela tiden i menyn, så att bygga en metod printMenu(selectedMenuOption int) skulle spara mycket rader. Man vill få bort all utfyllnad så man kan få en översikt av hur logiken fungerar. Detaljerna ser man in de olika funktionerna istället.

Okej, tack för din feedback!
Såhär visade dom i läromaterialet att man skulle bygga en switch-case för att få pilen att följa med, sen delade jag upp den för att den skulle bli så otroligt lång annars. Hur gör jag för att komprimera menyn till en mindre meny? Mitt försök var att ha uppdelad meny istället för att ha ett case / metod.

Permalänk
Skrivet av perost:

Du har flera stora if-satser som skriver ut menyer, där enda skillnaden är var ⭅ läggs till någonstans. Du kan byta ut dessa mot for-loopar istället, som skriver ut varje menyrad och om loop-indexet matchar MenySelect så lägger du till ⭅ på den raden.

Koden blir då inte heller beroende av hur många menyalternativ det finns, så du kan med fördel flytta koden till en metod som du kan anropa överallt istället. D.v.s. istället för en stor if-sats på varje ställe du vill skriva ut en meny på så blir det bara t.ex. printMenu(menu, MenuSelect).

Du blandar även olika namnsystem för variabler och metoder: snake_case, lowerCamelCase, samt UpperCamelCase (a.k.a. PascalCase). Det gör att koden blir mer rörig att läsa eftersom man inte direkt ser vad som är vad, så försök att vara lite mer konsekvent (se t.ex. Microsofts rekommendationer för namngivning).

Hej. Tack för din feedback!
Jag har sett andra alternativ av switch-case där man istället trycker på t.ex. tanget 4 för att välja val 4, men såhär med pilen så har jag bara fått visat för mig detta sättet att bygga den. Men om jag förstår rätt så kan man ta en for-loop och ändå få pilen att flytta sig med piltangenterna istället för att ha en if / else if för att uppdatera menyn?

Okej, så jag borde hålla en viss typ av skrift till en viss typ av del i koden? Typ en klass skrivs på ett sätt, metod på ett sätt etc. eller ska alla skrivas likadant, förstod inte riktigt i länken?

Hittade ett youtube-klipp på naming convetions som jag ska kika på

EDIT!! Förstår nu vad du menar. Löser detta, tack igen för feedback!

Permalänk
Medlem
Skrivet av NotSoGoodProgrammer:

Hej. Tack för din feedback!
Jag har sett andra alternativ av switch-case där man istället trycker på t.ex. tanget 4 för att välja val 4, men såhär med pilen så har jag bara fått visat för mig detta sättet att bygga den. Men om jag förstår rätt så kan man ta en for-loop och ändå få pilen att flytta sig med piltangenterna istället för att ha en if / else if för att uppdatera menyn?

Ja, för att skriva ut en meny utan pil så kan man ju bara göra:

for (int i = 0; i < menu.Length; i++) { Console.WriteLine(menu[i]); }

Hur kan du modifiera detta så att du istället skriver ut menu[i] + " ⭅" när det behövs?

Skrivet av NotSoGoodProgrammer:

Okej, så jag borde hålla en viss typ av skrift till en viss typ av del i koden? Typ en klass skrivs på ett sätt, metod på ett sätt etc. eller ska alla skrivas likadant, förstod inte riktigt i länken?

Hittade ett youtube-klipp på naming convetions som jag ska kika på

Ja, det vanliga i C# är att lokala variabler börjar med liten bokstav och t.ex. klasser/metoder med stor bokstav, så när jag ser t.ex. MenuSelect så tar det lite extra tid att fatta att det faktiskt är en lokal variabel. Exakt vilken stil som är bäst finns det delade meningar om, men det viktiga är att man är konsekvent och inte blandar stilar eftersom det gör det svårare för den som läser koden (vilket inkluderar dig själv).

Permalänk
Skrivet av perost:

Ja, för att skriva ut en meny utan pil så kan man ju bara göra:

for (int i = 0; i < menu.Length; i++) { Console.WriteLine(menu[i]); }

Hur kan du modifiera detta så att du istället skriver ut menu[i] + " ⭅" när det behövs?
Ja, det vanliga i C# är att lokala variabler börjar med liten bokstav och t.ex. klasser/metoder med stor bokstav, så när jag ser t.ex. MenuSelect så tar det lite extra tid att fatta att det faktiskt är en lokal variabel. Exakt vilken stil som är bäst finns det delade meningar om, men det viktiga är att man är konsekvent och inte blandar stilar eftersom det gör det svårare för den som läser koden (vilket inkluderar dig själv).

Okej, tack så jättemycket! Ska fixa dom grejerna! Får klura på den for() switch-case menyn. Jättebra att du talar om att det finns haha

Permalänk
Skrivet av NotSoGoodProgrammer:

Okej, tack för din feedback!
Såhär visade dom i läromaterialet att man skulle bygga en switch-case för att få pilen att följa med, sen delade jag upp den för att den skulle bli så otroligt lång annars. Hur gör jag för att komprimera menyn till en mindre meny? Mitt försök var att ha uppdelad meny istället för att ha ett case / metod.

Det perost skriver är det jag menar men mer utförligt beskrivet. Kör på det och sen när du snyggat till det lite så fortsätter du ett varv till. Lättast att ändra lite i taget.

Permalänk
Medlem
Skrivet av NotSoGoodProgrammer:

Okej, tack så jättemycket! Ska fixa dom grejerna! Får klura på den for() switch-case menyn. Jättebra att du talar om att det finns haha

Bara for (och if), du behöver ingen switch.

Permalänk
Skrivet av perost:

Bara for (och if), du behöver ingen switch.

Aha okej, läraren har rekommenderat användning av switch-case under Run(), men det är ju i och för sig bara en rekommendering. Känns inte som att dom kan få städad kod och switch-case på samma haha

Permalänk
Medlem
Skrivet av NotSoGoodProgrammer:

Aha okej, läraren har rekommenderat användning av switch-case under Run(), men det är ju i och för sig bara en rekommendering. Känns inte som att dom kan få städad kod och switch-case på samma haha

Din nuvarande switch-sats för att välja vad som ska göras efter att användaren valt ett menyalternativ är en bra användning av switch. Men du behöver ingen switch för at skriva ut menyn.

Permalänk
Skrivet av blegnitur:

Det perost skriver är det jag menar men mer utförligt beskrivet. Kör på det och sen när du snyggat till det lite så fortsätter du ett varv till. Lättast att ändra lite i taget.

Okej! Tack. Men om jag inte misstolkar dig så borde jag kanske ha en fil för bara en meny alltså? Och anropa den från Run()?

Permalänk
Skrivet av perost:

Din nuvarande switch-sats för att välja vad som ska göras efter att användaren valt ett menyalternativ är en bra användning av switch. Men du behöver ingen switch för at skriva ut menyn.

Aha.. Okej, men då ska jag klura lite! Tack igen! Uppskattat

Permalänk
Skrivet av NotSoGoodProgrammer:

Okej! Tack. Men om jag inte misstolkar dig så borde jag kanske ha en fil för bara en meny alltså? Och anropa den från Run()?

Du vill ha en egen metod för det. Grunden är att en metod gör en sak, och om du skriver samma sak på fler än ett ställe så borde det vara en metod för att dela kodne istället. Börja refaktorisera så, en sak i taget annars blir det för rörigt för dig.

Permalänk
Skrivet av blegnitur:

Du vill ha en egen metod för det. Grunden är att en metod gör en sak, och om du skriver samma sak på fler än ett ställe så borde det vara en metod för att dela kodne istället. Börja refaktorisera så, en sak i taget annars blir det för rörigt för dig.

Alright, då förstår jag! Tack igen! Uppskattad feedback!

Permalänk
Skrivet av perost:

Bara for (och if), du behöver ingen switch.

Lyckades inte lösa det. Kollade runt på Youtube och försökte hitta information om det. Men lyckades inte få ihop det med koden som den är skriven just nu, så jag behöll den med den stora switch-case menyn.

Har rättat till variabelnamnen genom att läsa på Microsoft och tror jag har fått till det på den fronten.

Men utöver att den är onödigt stor och upprepande är den okej? Är den svårläst eller något sånt?

Permalänk
Medlem
Skrivet av NotSoGoodProgrammer:

Lyckades inte lösa det. Kollade runt på Youtube och försökte hitta information om det. Men lyckades inte få ihop det med koden som den är skriven just nu, så jag behöll den med den stora switch-case menyn.

Ok, lösningen jag tänkte var bara så här:

for (int i = 0; i < menu.Length; i++) { if (i == menuSelect) { Console.WriteLine(menu[i] + " ⭅"); } else { Console.WriteLine(menu[i]); } }