Avatar billede johny Nybegynder
28. januar 2008 - 12:39 Der 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!");
        }
    }
}
Avatar billede powerpunk Nybegynder
28. januar 2008 - 13:30 #1
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!");
    }
}
Avatar billede arne_v Ekspert
28. januar 2008 - 15:08 #2
1)

Du boer ikke lave omfattende ting i konstruktor. Og det hjaelper altsaa ikke at flytte
det ud i et metode kald.

Lad Main kalde Run.

2)

Du boer ikke have public fields i en klasse. Brug private fields og public properties.

3)

Static fields som bruges i non-static metoder er ikke fint. De parametre boer vaere
non-static.

4)

Du kunne undgaa goto ved at flytte de 3 assignments op i try blokken. Meget paenere.
Avatar billede arne_v Ekspert
28. januar 2008 - 15:09 #3
5)

Du boer ikke catche Exception men en mere specifik exception.
Avatar billede arne_v Ekspert
28. januar 2008 - 15:15 #4
power>

Det er absolut ikke et godt design med alt static.

Paent design er langt vigtere end de milliardtedele sekunder du parer ved static kald.

Performancet problemet ved brugen af exception er ogsaa ikke-eksisterende, men
TryParse er stadig en fin loesning.
Avatar billede powerpunk Nybegynder
28. januar 2008 - 15:24 #5
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
Avatar billede powerpunk Nybegynder
28. januar 2008 - 15:39 #6
Arne_v -> "Paent design er langt vigtere end de milliardtedele sekunder du parer ved static kald."

principielt enig. Jeg er lidt miljøskadet af at rode med CompactFramework, hvor tidsbesparelserne er lidt mere målbare i stor skala...

Også enig i at der helt sikkert er pænere alternativer end at lave en stor statisk klasse. Evt. kunne man lege lidt med en

public struct Parameters
{
  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 = ms / 10000;
    _remainder = ms % 10000;
  }
}

som så kunne smides til Run Metoden. Og til tråden med en ParameterizedThreadStart delegate.
Avatar billede powerpunk Nybegynder
28. januar 2008 - 15:55 #7
Hvad med noget i denne stil:

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);
        }
    }
}
Avatar billede powerpunk Nybegynder
28. januar 2008 - 16:01 #8
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...
Avatar billede powerpunk Nybegynder
19. februar 2008 - 08:21 #9
Er det noget der kunne bruges?
Avatar billede johny Nybegynder
19. februar 2008 - 16:40 #10
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
Avatar billede powerpunk Nybegynder
20. februar 2008 - 09:28 #11
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...

Happy Coding!
Avatar billede arne_v Ekspert
25. februar 2008 - 05:20 #12
Mit forslag:

using System;
using System.Threading;

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();
        }
    }
}
Avatar billede johny Nybegynder
25. februar 2008 - 09:22 #13
Hej Arne,

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?
Avatar billede powerpunk Nybegynder
25. februar 2008 - 09:55 #14
Et alternativ til sleep kunne være en ManualResetEvent.

Thread.Sleep(UNIT)

bliver så til

din_ManualResetEvent_instans_her.WaitOne(UNIT, false);

Så ventes der op til UNIT antal millisekunder på at eventen bliver sat. Hvis den bliver sat returnerer WaitOne true med det samme...
Avatar billede arne_v Ekspert
26. februar 2008 - 04:09 #15
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.
Avatar billede powerpunk Nybegynder
26. februar 2008 - 08:24 #16
Det er netop der jeg mener at en waitevent ville være smart - Så venter du til et antal millisekunder er gået, eller eventen signaleres.

ManualResetEvent abortEvent = new ManualResetEvent(false);
-snip-
public void RunMany()
{
  while(true)
  {
    Thread t = new Thread(RunOne);
    abortEvent.Reset();   
    t.Start();
    Console.WriteLine("Press any key to continue");
    Console.ReadKey();
    if(t.IsAlive)
    {
      abortEvent.Set();
      Console.WriteLine("Countdown aborted");
    }
    t.Join();
  }
}
public void RunOne()
{
  Console.WriteLine("Countdown initiated");
 
  for(int i = 1; i <= Divisions && !abortEvent.WaitOne(UNIT, false); i++)
  {
    Console.WriteLine((i * UNIT)/1000.0 + " sec has passed");
  }
  if(!abortEvent.WaitOne(Remainder, false));
    Console.WriteLine("TIME IS UP!");
}

Så afsluttes der med det samme når myWaitEvent.Set() kaldes i stedet for at vente potentielt UNIT sekunder.
Avatar billede johny Nybegynder
26. februar 2008 - 08:57 #17
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?
Avatar billede arne_v Ekspert
27. februar 2008 - 05:13 #18
t er kun en pointer på stakken - den betyder ikke noget.

ManualResetEvent kender jeg ikke, men den ser velegnet ud til problem stillingen.
Avatar billede Ny bruger Nybegynder

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.

Loading billede Opret Preview
Kategori
IT-kurser om Microsoft 365, sikkerhed, personlig vækst, udvikling, digital markedsføring, grafisk design, SAP og forretningsanalyse.

Log ind eller opret profil

Hov!

For at kunne deltage på Computerworld Eksperten skal du være logget ind.

Det er heldigvis nemt at oprette en bruger: Det tager to minutter og du kan vælge at bruge enten e-mail, Facebook eller Google som login.

Du kan også logge ind via nedenstående tjenester