Kodkritik - historybox

Ursprungligen inskrivet av vb
En lätt sak vore att ändra så att historypointen tar en params object[] istället för object[] bara. Då blir den lite smidigare o använda. Iof kanske det är onödigt att alls ta in en parameterlista? Låt historyDelegate vara en Action istället, så får man nöja sig med att ta med parametrar direkt istället:

var x = "kalle"
var d = new DelegateHistoryPoint(delegate () { Console.WriteLine("hej "+x);});

Ursprungligen inskrivet av vb
Eller vid närmare eftertanke känns det lite oklart om hela upplägget funkar.. Låt säga att man vill ha sin historybox som i firefox, hur ska man då implementera nerpilen/högerklick som visar x antal av dom senaste besökta platserna?

Ursprungligen inskrivet av vb
En annan grej vore att bryta ut renderingskoden. Tex så skulle man då kunna använda historyboxen på en asp.net-sida. Kanske att det t.o.m. skulle vara värt o ärva från ICollection eller IEnumerable?

Ursprungligen inskrivet av vb
Du skrev ju iof att du ville ha kritik på allt möjligt i första posten

Det jag menade var inte att du behöver lägga till det nu, utan att jag inte ser att det alls går att göra en sådan funktion på något smidigt sätt så som koden är strukturerad nu. Dvs den är onödigt komplex för att vara så begränsad som den är.

Ursprungligen inskrivet av vb
Jo, med det menar jag precis det jag skrev Då skulle man kunna använda samma HistoryBox-klass även i ett projekt där man inte bygger UIt med Forms utan istället kanske en websida med ASP.Net. Som det är nu skulle man få skrota hela HistoryBox-klassen om man inte vill använda Forms för att rita sitt UI.

Ursprungligen inskrivet av vb
Givetvis är båda dessa poänger inte viktiga om du bara behöver koden till precis det den gör nu, men o andra sidan tycker jag den är alldeles för tillkrånglad i så fall Tex är det onödigt med hela DelegateHistoryPoint-klassen, HistoryBox skulle lika gärna kunna ha en lista på Actions i Items-listan (det skulle den kanske annars oxå?).

Ursprungligen inskrivet av vb
1. Att skriva "// increment x" som kommentar till en rad som ökar x med 1 tillför ingenting, istället krånglar den bara till det genom att göra koden längre och mer svårläst.

Ursprungligen inskrivet av vb
2. Att använda unsigned ints är inte standard i c#. Tex används den inte i standardbiblioteket (utom när den verkligen behövs givetvis). Så jag tycker du borde köra på int istället.

Ursprungligen inskrivet av vb
3. Jag gillar inte riktigt vad du namngett med HelloWorld respektive helloWorld. privata variablar alltid lowercase i början hade inte varit så dumt tex..

Ursprungligen inskrivet av vb
4. Känns lite snålt o inte ge MaxHistoryPoints ett defaultvärde.

Ursprungligen inskrivet av vb
5. Du har en del onödiga parenteser här och var i koden, tex rad 90 i HistoryBoxen.

Ursprungligen inskrivet av vb
Tja, man skulle kunna ta bort ett interface och en klass (ca 25% av koden) utan att förändra funktionen, det kallar jag onödigt komplext
Dock får jag nog ta tillbaka att det skulle vara så svårt o göra en history-lista, ärver man ifrån boxen är det egentligen bara ett par privata variabler som ställer till det och det går ju o fixa ganska lätt.

Ursprungligen inskrivet av vb
Är aldrig dumt o fundera ett steg extra. Du själv kanske behöver en HistoryBox i ett annat projekt i framtiden och det blir även lite tydligare om koden som ritar upp saker är skilt från själva logiken.. Om du skulle skriva ett test för att se ifall din historybox funkar som den ska måste du nu skapa en hel HistoryBox med en massa GUI inbakat.
I vilket fall borde det vara något du funderat över, sen kanske man snabbt konstaterar att i det här fallet så är det enklare o skita i det och så skippar man det. Men poängen med hela tråden är ju o tänka ett steg längre än precis som koden ser ut idag, eller?

Ursprungligen inskrivet av vb
Som nån skrev på stackoverflow, "when in rome, do as the romans do". i det här fallet tycker jag best practice är bättre att följa. Men om du ändå vill använda uint, borde då inte Stage mfl oxå vara sådana? Iof är Count alltid en int så det går väl inte o fixa riktigt om du inte byter till LongCount Dessutom finns även nu ett stort spann av ogiltiga värden man kan sätta på MaxHistoryPoints, så fort den går över IntMax slutar koden att fungera även om det är liten chans det händer i praktiken. Jag ser inte riktigt hur du har minskat på chansen att göra fel här faktiskt..

Ursprungligen inskrivet av vb
6. Såg en till sak nu, vore det inte mysigare med en using på System.Windows.Forms & System.Drawing så behöver du inte skriva ut hela sökvägen?

Ursprungligen inskrivet av vb
Ja, om det är något mer komplicerat som ska ske är det nog bättre som det är nu, det håller jag med om. Men jag vet inte riktigt om jag gillar att ett så tjockt objekt skulle läggas in i historyboxen, det känns lite tungt. Fast jag har inte tänkt så mkt på det, och det är såklart svårt när man inte ser den in action tillsammans med annan kod.

Ett annat sätt att lösa det på kanske vore med någon event-lösning. När en history-point ska visas skickar den ett event istället till dom som har behov av att något ska hända. Aja, är inte säker på att det vore bättre heller så

Ursprungligen inskrivet av vbTa bort allt forms från boxen, typ såhär: http://pastebin.ca/1754946 Och sen göra en ny BoxControl som innehåller bilderna mm och dessutom har den förminskade boxen i sig.

I nån mening kommer det göra koden lite mer komplex dvs säga emot det jag argumenterat för tidigare, så om det är värt o göra i det här fallet eller ej tål att funderas över. Uppenbart är iaf att logiken på boxen blir tydlig och lättläst om den ligger i en egen klass utan GUIt.

Ursprungligen inskrivet av vb
Microsoft har ju ingen egentlig kodstandard på MSDNen förutom det som skriver om class library design.. Dock är det inte CLS-compliant att använda unsigned types. Förutom det får man nog nöja sig med att läsa på forum o liknande, inte många nämner unsigned verkar det som.

Ursprungligen inskrivet av vb
Kan för övrigt tipsa om visual studio-pluginen Resharper om du inte redan testat den, hjälper till en hel del med sådant vi pratat om här (kostar en slant, men finns en trial man kan testa)

Ursprungligen inskrivet av vb
Nej, Actions tar alltid in ett fixt antal parametrar, så om du behöver olika antal är nog Delegate bättre.

Resharper är mer en hjälp under tiden man skriver koden än efteråt när man kompilerar. Blir väldigt tydligt o fint när man skriver något den klagar på, och man kan då rätta till det med en snabbknapp/musklick. Men den gör mycket annat oxå, tex Goto Implementation tycker jag är en väldigt smidig funktion när F12 går till Interfacet och inte implementationen. Jag känner mig lite handikappad om jag inte har Resharper igång, så man blir lite lat