28. januar 2008 - 12:39Der er
17 kommentarer og 1 løsning
Kode optimering
Jeg er i øjeblikket i færd med 4. semester på datamatiker uddannelsen, men synes at undervisning i kvalitetsprogrammering/optimering osv. har været meget sparsom, så det er noget jeg må finde ud af ved selvstudie.
Men nu har jeg prøvet at lave en meget lille klasse, et nedtællingsur i consol, der som standard tæller i 1 min, og hvis man så trykker på en hvilken som helst tast afbryder den nedtællingen og starter forfra.
Er der nogen der kan fortælle mig om jeg har nogen huller i min kode rent optimeringsmæssigt? Med andre ord, om der er noget jeg kan gøre for at forbedre koden som den ser ud nu?
Her er mit namespace som det ser ud nu:
namespace Countdown { public class Program { static void Main(string[] args) { if(args.Length != 0) { //allows for changing the standard 60 sec duration int arg = 0; try { arg = Convert.ToInt32(args[0]); } catch(Exception) { Console.WriteLine("Invalid input. Program started with standard settings."); goto postArgs; } Counter.ms = arg; Counter.divisions = arg / 10000; //sets the amount of whole 10 sec periods Counter.remainder = arg % 10000; //sets the remainder } postArgs: new Counter(); } }
public class Counter { private static SoundPlayer sp = new SoundPlayer(Countdown.Properties.Resources.alarm); //static player to play a custom wav file included in the resources (embedded) public static int ms = 60000; public static int divisions = 10; public static int remainder = 0;
public Counter() { run(); //I prefere not having loop code in constructors, else the content of run could be placed here instead }
private void run() { Thread t = null; while(true) { t = new Thread(timer); t.Start(); Console.ReadKey(true); //interrupts current thread and starts a new / starts a new if thread already done if(t.IsAlive) { t.Abort(); Console.WriteLine("Countdown aborted"); } } }
private void timer() {
Console.WriteLine("Countdown initiated"); for(int i = 1;i <= divisions;i++) { Thread.Sleep(10000); Console.WriteLine(i + "0 sec has passed"); SystemSounds.Exclamation.Play(); //plays a small windows sound to indicate the passing of 10 sec audible too. } Thread.Sleep(remainder); //runs the remainder sp.Play(); //plays the sound in a new thread Console.WriteLine("TIME IS UP!"); } } }
Der er flere ting som jeg i hvert fald personligt ikke synes er så fornemme.
bl.a er brugen af 'goto' statements en klassisk fy-fy, som i mere komplekse scenarier kan generere noget forfærdelig spaghetti kode...
try/catch conversion er i min optik også lidt grimt, da selve det at hive exception handling i luften er tungt. Der ville jeg nok snarere bruge int.TryParse()
Counter bør også laves som en statisk klasse (statiske kald er marginalt hurtigere end instance kald)
Jeg foreslår følgende:
namespace Countdown { public class Program { static void Main(string[] args) { int arg = 0; if(args.Length != 0 && !int.TryParse(args[0], arg)) { Console.WriteLine("Invalid input. Program started with standard settings."); }
Counter.ms = arg; Counter.divisions = arg / 10000; //sets the amount of whole 10 sec periods Counter.remainder = arg % 10000; //sets the remainder
Counter.Run; } }
public static class Counter { private static SoundPlayer sp = new SoundPlayer(Countdown.Properties.Resources.alarm); //static player to play a custom wav file included in the resources (embedded) internal static int ms = 60000; internal static int divisions = 10; internal static int remainder = 0;
public static void run() { Thread t = null; while (true) { t = new Thread(timer); t.Start(); Console.ReadKey(true); //interrupts current thread and starts a new / starts a new if thread already done if (t.IsAlive) { t.Abort(); Console.WriteLine("Countdown aborted"); } } }
private static void timer() { Console.WriteLine("Countdown initiated"); for (int i = 1; i <= divisions; i++) { Thread.Sleep(10000); Console.WriteLine(i + "0 sec has passed"); SystemSounds.Exclamation.Play(); //plays a small windows sound to indicate the passing of 10 sec audible too. } Thread.Sleep(remainder); //runs the remainder sp.Play(); //plays the sound in a new thread Console.WriteLine("TIME IS UP!"); } }
Det er vel langt hen ad vejen det samme jeg har foreslået.
1. Løst ved statisk klasse/run 2. Jeg har valgt at bibeholde de tre felter, ikke som public men internal. Hovedpointen i at pakke private fields i public properties er kompatibilitet. Hvis vi udelukkende interesserer os for performance er det marginalt hurtigere at tilgå fields end properties. Normalt vil jeg give arne_v ret, men hvis klassen kun skal bruges internt i assembly'en mener jeg sagtens det kan forsvares ikke at wrappe dem i properties. 3. I mit eksempel med en statisk klasse er de nødt til at være statiske. Et alternativ kunne være at pakke dem pænt ind i en struct, som kunne passes med rundt... 4./5. goto samt try/catch er omgået, jvf mit eksempel... ellers enig
namespace Countdown { public class Program { static void Main(string[] args) { int arg = 0; if (args.Length != 0 && !int.TryParse(args[0], out arg)) Console.WriteLine("Invalid input. Program started with standard settings.");
Counter.Run(new Parameters(arg)); } }
public abstract class Counter { public static void Run(Parameters p) { Thread t = null; while (true) { t = new Thread(Timer); t.Start(p); Console.ReadKey(true); //interrupts current thread and starts a new / starts a new if thread already done
if (t.IsAlive) { t.Abort(); Console.WriteLine("Countdown aborted"); } } }
private static void Timer(object parameters) { Parameters p = parameters as Parameters;
if (p == null) Console.Write("Invalid input to timer function"); else { Console.WriteLine("Countdown initiated"); for (int i = 1; i <= p.Divisions; i++) { Thread.Sleep(10000); Console.WriteLine(i + "0 sec has passed"); SystemSounds.Exclamation.Play(); //plays a small windows sound to indicate the passing of 10 sec audible too. }
Thread.Sleep(p.Remainder); //runs the remainder sp.Play(); //plays the sound in a new thread Console.WriteLine("TIME IS UP!"); } } }
public class Parameters { private const int divisor = 10000;
private int _ms; private int _divisions; private int _remainder;
public int Ms { get { return _ms; } } public int Divisions { get { return _divisions; } } public int Remainder { get { return _remainder; } }
public Parameters(int ms) { _ms = ms; _divisions = Math.DivRem(ms, divisor, out _remainder); } } }
Jeg er dog stadig ikke helt gode venner med den måde tråden bliver hevet ned på... Thread.Abort er en lidt voldsom måde at afslutte en tråd på. Den burde pilles ned med en Thread.Join. Det er selvfølgelig lidt problematisk som den ser ud nu, fordi tråden så ville få lov at køre helt færdig før den kunne joine. Så skulle man lave kortere sleeps og checke en eller anden event ind imellem...
Av, jeg havde helt glemt det her topic igen. Det må I meget undskylde! Blev mere og mere klar over at det egentlig ikke var kodeoptimering jeg søgte, men god program struktur, fordi compileren netop kan klare de fleste ting under optimeringen.
Men I har helt sikkert gevet mig stof til det, selvom det ikke var det jeg spurgte om, så det var jo bare lækkert.. :)
Der er dog lige nogle korte ting jeg gerne vil have opklaret:
Parameters: Er det en god struktur at skrive sådan en klasse? Så man på den måde altså undgår at have statiske elementer? Og skulle den evt. skrives om til at følge Singleton mønsteret?
Static vs. instance: Vil det ikke være bedst at holde sig til instances hvor man kan? Om ikke andet, hvis man vælger at bruge det i et andet program, slipper man jo for problemer med threading.
Point: Hvordan deler jeg lige dem? Er det ok med dig arne hvis powerpunk får dem? Det er trods alt ham der har lagt mest arbejde i det? Men tak for hjælpen begge to i hvert fald! :D
Parameters: I tilfælde hvor der er en klar sammenhæng mellem variable, som her, vil jeg mene at det giver god mening at pakke dem ind i en klasse der kan håndtere denne sammenhæng. At navnet 'Parameters' så ikke er godt er noget andet... Jeg burde nok have kaldt den 'TimerArgs' eller lignende. Singleton pattern er nok ikke sagen her, da klassen som regel vil skulle bruges som input argument til timeren, og derfor følger en load->fire->forget livscyklus. Jeg har set en del Singleton misbrug, hvor klasser tilgås fra alle mulige og umulige steder i koden. Det er bestemt ikke hensigten. Singletons er gode til f.eks at sikre at adgang til en ressource foregår gennem éet objekt.
Static vs. instance: At holde sig til instance members er i hvert fald nok smart set fra et maintainability synspunkt. Der er mindre mulighed for misbrug á la ovenstående, og der er bedre styr på hvem der har adgang til dem... Dermed ikke sagt at de ikke kan have deres berettigelse. Statiske metodekald er som nævnt marginalt hurtigere end instance kald. Det er 'common best-practise' at erklære metoder statiske, hvis de ikke på nogen måde tilgår 'this' og altså kun opererer på det input de får.
Point: Jeg vil mene at systemet selv deler dem i X antal lige store bunker, hvis du accepterer X svar på en gang (altså 30/30) i dette tilfælde...
PS: Hvis du interesserer dig for 'best-practises' skulle du tage (hvis du ike allerede har gjort det) og kigge på værktøjet FxCop (http://kortlink.dk/microsoft/4tvy). Det analyserer din kode ud fra en række fastsatte (microsoft) best-practises. Nogle af dem er lidt pindehugger-agtige, men kan heldigvis slås fra. Andre giver god mening. Der er som regel en god forklaring på de enkelte regler...
namespace E { public class CountDown { private const int UNIT = 10000; private int ms; private bool abort; public CountDown() : this(0) { } public CountDown(int ms) { this.ms = ms; } public int Ms { get { return ms; } set { ms = value; } } public int Divisions { get { return ms / UNIT; } } public int Remainder { get { return ms % UNIT; } } public void RunMany() { while(true) { Thread t = new Thread(RunOne); abort = false; t.Start(); Console.WriteLine("Press any key to continue"); Console.ReadKey(); if(t.IsAlive) { abort = true; Console.WriteLine("Countdown aborted"); } t.Join(); } } public void RunOne() { Console.WriteLine("Countdown initiated"); for(int i = 1; i <= Divisions && !abort; i++) { Thread.Sleep(UNIT); Console.WriteLine((i * UNIT)/1000.0 + " sec has passed"); } Thread.Sleep(Remainder); Console.WriteLine("TIME IS UP!"); } } public class Program { private const int DEFAULT_MS = 60000; public static void Main(string[] args) { CountDown c = new CountDown(); if(args.Length > 0) { int msarg; if(int.TryParse(args[0], out msarg)) { c.Ms = msarg; } else { Console.WriteLine("Invalid argument - using default values"); c.Ms = DEFAULT_MS; } } else { Console.WriteLine("No argument - using default values"); c.Ms = DEFAULT_MS; } c.RunMany(); } } }
Er det noget du har testet? For så vidt jeg lige kan se, er der i hvert fald 2 problemer med din kode, som alle ligger samme sted: public void RunOne() { Console.WriteLine("Countdown initiated"); for(int i = 1; i <= Divisions && !abort; i++) { Thread.Sleep(UNIT); Console.WriteLine((i * UNIT)/1000.0 + " sec has passed"); } Thread.Sleep(Remainder); Console.WriteLine("TIME IS UP!"); } For det første, hvis der bliver trykke på knappen lige efter en unit er passeret, så vil man skulle vente i ~9 sek før den starter en ny tråd? Plus, de sidste to linjer vil vel altid blive eksekveret også? Så de skal vel også ind i en if sætning med !abort?
Det er nemlig derfor jeg selv bruger .abort(), fordi det er en effektiv måde at lukke ned på, og da trådene ikke indeholder noget væsentligt sker der heller ingen skade ved det. Eller er det ikke korrekt?
Ja. Det kan godt tage lidt tid inden den får aborteret. Det er derfor jeg tilføjede en Join oppe i RunMain. Hvis det er et problem så lader du den sleepe i kortere intervaller (men skriver kun ud en gang for hver divisions). Men jeg ville altså gerne af med den Abort.
Det sidste har du helt ret i - der bør en if(!abort) omkring remainder waitet.
Ahh, ja det var meget smartere egentlig.. :) Takker powerpunk.
Arne, du svarede aldrig på om det var i orden at powerpunk fik points'ne, og har heller ikke efterladt et svar, så for at lukke den lidt, så har jeg nu givet powerpunk point, men hvis jeg har været for hurtig, så sig endelig til, og så matcher jeg selvfølgelig points'ne til dig også. :)
Men når vi nu er i optimering, burde Thread t så ikke erklæres uden for while løkken, så der ikke skal findes en ny plads i ram til den hver gang?
t er kun en pointer på stakken - den betyder ikke noget.
ManualResetEvent kender jeg ikke, men den ser velegnet ud til problem stillingen.
Synes godt om
Ny brugerNybegynder
Din løsning...
Tilladte BB-code-tags: [b]fed[/b] [i]kursiv[/i] [u]understreget[/u] Web- og emailadresser omdannes automatisk til links. Der sættes "nofollow" på alle links.