c# if/else vs switch ranges

[Ron]

voor m'n werk ben ik bezig een applicatie te schrijven waar user input aan de hand van de waarde op de juiste plaats gezet wordt.

op het moment los ik het op de volgende manier op:
//between is een extension method op alle comparable types

code:

            foreach (var field in unknownFields)
            {
                var startAddress = field.StartAddress;


                if (startAddress.Between(0x8000, 0x807f))
                {
                    field.FieldType = CNvr1;
                }
                else if (startAddress.Between(0x8080, 0x80FF))
                {
                    field.FieldType = CNvr2;
                }
               ......
        }

in c# 7 is het mogelijk om ranges op te geven in switches, waarmee dit gereduceert kan worden tot

code:

            foreach (var field in unknownFields)
            {
                var startAddress = field.StartAddress;

                switch (startAddress)
                {
                    case int n when n >= 8000 && n <= 0x807F:
                        field.FieldType = CNvr1;
                        break;

                    case int n when n >= 0x8080 && n <= 0x80FF:
                        field.FieldType = CNvr2;
                        break;

                    case ....
                }
            }

zelf vind ik de c#7 manier mooier, maar ik vroeg me af wat anderen er over denken en of er nog andere manieren zijn om iets dergelijks te doen

EDIT: dit had dus een discussietopic moeten wezen geen vraag

[_Erikje_]

is het niet mooier om een map te maken met als key je FieldType en de value een object met de range? Dan kan je het makkelijker mappen en configurabel maken...

[DJMaze]

Ron schreef op vrijdag 4 mei 2018 @ 11:57:
in c# 7 is het mogelijk om ranges op te geven in switches, waarmee dit gereduceert kan worden tot

Het is geen reductie, het is een andere schrijfwijze.

Ron schreef op vrijdag 4 mei 2018 @ 11:57:
zelf vind ik de c#7 manier mooier, maar ik vroeg me af wat anderen er over denken en of er nog andere manieren zijn om iets dergelijks te doen

Ik vind het lelijker. Daarom rijden we ook niet allemaal in de zelfde auto en kiest de één voor Doutzen Kroes en de ander voor Adriana Lima.

[Ron]

_Erikje_ schreef op vrijdag 4 mei 2018 @ 12:01:
is het niet mooier om een map te maken met als key je FieldType en de value een object met de range? Dan kan je het makkelijker mappen en configurabel maken...

daar zeg je me wat, ik ben d'r altijd vanuit gegaan dat ik in een dictionary (of map, hoe je het ook wil noemen) statische waarden in moet voeren

code:

        public static readonly Dictionary<RegisterType, IEnumerable<int>> AddressFieldDictionary =
            new Dictionary<RegisterType, IEnumerable<int>>
                {
                    { RegisterType.CNvr1, Enumerable.Range(0x8000, 0x807F) },
                    { RegisterType.CNvr2, Enumerable.Range(0x8080, 0x80FF) },
                    ......
                }

en om 't aan te roepen:

code:

 foreach (var field in unknownFields)
            {
          field.FieldType = MemoryField.AddressFieldDictionary.SingleOrDefault(x => x.Value.Any(y => y == field.StartAddress)).Key;
            }

Ik had toch al een dictionary voor het startadres, dus dan kan ik die meteen aanpassen.

[_Erikje_]

Ron schreef op vrijdag 4 mei 2018 @ 12:28:
[...]
...

code:

 foreach (var field in unknownFields)
            {
          field.FieldType = MemoryField.AddressFieldDictionary.SingleOrDefault(x => x.Value.Any(y => y == field.StartAddress)).Key;
            }

kiek um goan Dat ziet er toch prachtig uit.

[Ron]

voor future reference:

code:

 .contains

is goedkoper dan

code:

.any

[whoami]

Ron schreef op vrijdag 4 mei 2018 @ 14:23:
voor future reference:

code:

 .contains

is goedkoper dan

code:

.any

Hoezo ?

Any is een LINQ extension method dus het kan perfect zijn dat die voor een IDictionary de Contains method achter de schermen aanroept.

Daarnaast: vergeet niet IEquatable te implementeren op de RegisterType class.

[ Voor 11% gewijzigd door whoami op 04-05-2018 15:27 ]

[ThomasG]

whoami schreef op vrijdag 4 mei 2018 @ 15:26:
[...]

Hoezo ?

Any is een LINQ extension method dus het kan perfect zijn dat die voor een IDictionary de Contains method achter de schermen aanroept.

Daarnaast: vergeet niet IEquatable te implementeren op de RegisterType class.

Any is altijd O(n), want het is een extention method die itereert over de collectie om de delegate toe te pasen. Contais is een implementatie van de collectie. Zo is het bij een List O(n), maar bij een HashSet O(1). Contais is goedkoper als de onderliggende collectie dat is. In dit geval zou Contais dus een betere keuze zijn, omdat de delegate van de Any geen toegevoegede waarde heeft over Contais, en langzamer is/kan zijn.

[whoami]

ThomasG schreef op vrijdag 4 mei 2018 @ 15:42:
[...]
Any is altijd O(n), want het is een extention method die itereert over de collectie om de delegate toe te pasen. Contais is een implementatie van de collectie. Zo is het bij een List O(n), maar bij een HashSet O(1). Contais is goedkoper als de onderliggende collectie dat is. In dit geval zou Contais dus een betere keuze zijn, omdat de delegate van de Any geen toegevoegede waarde heeft over Contais, en langzamer is/kan zijn.

Any hoeft niet over de hele collectie te itereren. Van zodra één element matcht met de predicate, kan deze al true returnen.

Maar, inderdaad, Any itereert altijd over de collectie en een Contains zal dus in ieder geval sneller zijn, ook al returned Any van zodra er een match gevonden is.
Ik had ergens verwacht dat Any het type waarop het werd uitgevoerd ging gaan inspecteren en op basis daarvan een andere implementatie ging gaan uitvoeren.

[Lye]

whoami schreef op vrijdag 4 mei 2018 @ 15:52:
[...]

Any hoeft niet over de hele collectie te itereren. Van zodra één element matcht met de predicate, kan deze al true returnen.

Dat veranderd niets aan de complexiteit, die blijft gewoon O(n).

Maar, inderdaad, Any itereert altijd over de collectie en een Contains zal dus in ieder geval sneller zijn, ook al returned Any van zodra er een match gevonden is.
Ik had ergens verwacht dat Any het type waarop het werd uitgevoerd ging gaan inspecteren en op basis daarvan een andere implementatie ging gaan uitvoeren.

Ik mag niet hopen dat Any dat doet, dat hoort Any namelijk helemaal niet te doen. Any hoort alleen te checken of een element aan het gegeven predicaat voldoet. Sowieso is het onmogelijk om een snellere manier te implementeren dan O(n) vanwege het feit dat het argument dat Any een predicaat accepteert en niet een element wat in de datastructuur zou moeten zitten. Dit predicaat kan compleet andere dingen specificeren, waarvan ik hoop dat ook jij zult begrijpen dat Any dit niet allemaal moet gaan zitten analyseren om te bepalen of het misschien sneller is om contains te gebruiken. any doet dus maar één ding en dat is kijken of er een element is in de gegeven datastructuur waarvoor het gegeven predicaat true returned. Dit gebeurt in O(n), ook al voldoet het eerste element al.

[Sandor_Clegane]

Waarom niet C# pattern matching? Het is niet zo goed als in F# maar het werkt wel mooi.

Zoals in het tweede voorbeeld, vind ik zelf een fijne manier.

[ Voor 27% gewijzigd door Sandor_Clegane op 06-05-2018 00:17 ]

[Alex)]

Welk probleem probeer je op te lossen? Zijn die FieldTypes dynamisch of heb je te maken met een hardcoded lijst?

Als ze hardcoded zijn ben je met een paar simpele if/else-structuren of een switch-oplossing al klaar. Maak het niet complexer dan het is, dat maakt onderhoud alleen maar moeilijker.

_Erikje_ schreef op vrijdag 4 mei 2018 @ 12:01:
is het niet mooier om een map te maken met als key je FieldType en de value een object met de range? Dan kan je het makkelijker mappen en configurabel maken...

En wat voegt dat toe?

_Erikje_ schreef op vrijdag 4 mei 2018 @ 12:47:
[...]


kiek um goan Dat ziet er toch prachtig uit.

Ik vind het vooral moeilijk om te lezen en het is mij niet meteen duidelijk wat er gebeurt.

Sandor_Clegane schreef op zondag 6 mei 2018 @ 00:15:
Zoals in het tweede voorbeeld, vind ik zelf een fijne manier.

Mee eens. Die constructie met dictionary's, LINQ en ranges voelt voor mij zwaar overengineered aan waardoor de leesbaarheid er zwaar onder lijdt. Toen ik dat stukje code voor het eerst zag had ik geen flauw idee wat het nou eigenlijk deed.

Een paar if/else'jes (of een C# 7 switch-based pattern matching-aanpak) is eenvoudig te lezen, eenvoudig te begrijpen, en performt ook nog eens prima.

[Sandor_Clegane]

In F# heb je iets minder boilerplate dan in C#:

code:

let setField field =
    match addressRange with
    | n when n >= 8000 && n <= 0x807F ->
        field.FieldType <- CNvr1
    | n when n >= 0x8080 && n <= 0x80FF ->
        field.FieldType <- CNvr2
    | _ -> 
        field.FieldType <- Default

[Ron]

Alex) schreef op dinsdag 8 mei 2018 @ 02:50:
Welk probleem probeer je op te lossen? Zijn die FieldTypes dynamisch of heb je te maken met een hardcoded lijst?

Als ze hardcoded zijn ben je met een paar simpele if/else-structuren of een switch-oplossing al klaar. Maak het niet complexer dan het is, dat maakt onderhoud alleen maar moeilijker.

[...]

Mee eens. Die constructie met dictionary's, LINQ en ranges voelt voor mij zwaar overengineered aan waardoor de leesbaarheid er zwaar onder lijdt. Toen ik dat stukje code voor het eerst zag had ik geen flauw idee wat het nou eigenlijk deed.

Het gaat hier om een applicatie waarbij van te voren niet helemaal duidelijk is tot hoeveel FieldTypes ik op moet schalen. daarnaast word diezelfde range gebruikt voor een aantal dingen:

1) om te controleren of een gegeven veld binnen de vooraf gedefinieërde velden voorkomt
2) bij een aantal overloads van de constructor voor een memoryveld (nog) niet bekende variabelen in te vullen.

Met .contains in plaats van any is het imho ook al een stuk duidelijker

code:

field.FieldType = MemoryField.AddressFieldDictionary
                    .SingleOrDefault(x => x.Value.Contains(field.StartAddress)).Key;

[ThoNohT]

Waarom gebruiken we hier een Dictionary waarin we de key opzoeken aan de hand van de value? Zijn dictionaries nu niet precies voor het omgekeerde gemaakt?

True, je wil hier een complexer predicaat testen dan een simpele key. Maar dan is een simpele List natuurlijk ook goed genoeg. Of je moet je ranges zo weten te hashen dat je elke waarde in die range met een efficiente indexing functie in de dictionary op kan zoeken door

code:

AddressFieldDictionary[hash]

te gebruiken.

[Gomez12]

Ron schreef op vrijdag 4 mei 2018 @ 12:28:
[...]

code:

        public static readonly Dictionary<RegisterType, IEnumerable<int>> AddressFieldDictionary =
            new Dictionary<RegisterType, IEnumerable<int>>
                {
                    { RegisterType.CNvr1, Enumerable.Range(0x8000, 0x807F) },
                    { RegisterType.CNvr2, Enumerable.Range(0x8080, 0x80FF) },
                    ......
                }

en om 't aan te roepen:

code:

 foreach (var field in unknownFields)
            {
          field.FieldType = MemoryField.AddressFieldDictionary.SingleOrDefault(x => x.Value.Any(y => y == field.StartAddress)).Key;
            }

Ik had toch al een dictionary voor het startadres, dus dan kan ik die meteen aanpassen.

Ik zou dit toch echt markeren voor een code-wtf.

Ik vind het onleesbaar (oh, je pakt een dictionary, dan wil je dus op keys zoeken. Maar ik heb helemaal geen key, hoe zit dat nou? Oh wacht je gaat op value zoeken) en qua prestatie blijft het volgens mij ook redelijk ondermaats (range is een for-loop vs een between)
En met een if/else / switch ben je wmb nog flexibeler ook, want je kan iets in 2 (of meer) cases laten vallen).

Oftewel wmb is dit echt een code-wtf en ik zou het echt bij iedereen afkeuren die dit bij ons zou gebruiken. Ja, het kan maar daar eindigt elke argument voor dan ook bij mij...
Ik heb niets tegen onleesbare code vanwege performance redenenen, maar dit is onleesbare code die volgens mij slechter performed...

En om terug te komen op de initiele vraag : if/else vs switch, dat hangt wmb af van de complexiteit van de onderliggende code en van het aantal cases. Bij 2 cases is het een if-else bij grofweg 4-10 cases wordt het wmb een switch en bij meer wordt het al snel refactoren of maatwerk

[ThoNohT]

Ron schreef op vrijdag 4 mei 2018 @ 12:28:

code:

        public static readonly Dictionary<RegisterType, IEnumerable<int>> AddressFieldDictionary =
            new Dictionary<RegisterType, IEnumerable<int>>
                {
                    { RegisterType.CNvr1, Enumerable.Range(0x8000, 0x807F) },
                    { RegisterType.CNvr2, Enumerable.Range(0x8080, 0x80FF) },
                    ......
                }

Overigens is dit ook niet de meest efficiente om een range tussen twee getallen op te slaan. Hier genereer je een IEnumerable, die elke waarde in de range teruggeeft. En daar ga je Contains op doen, waardoor je dus elke keer elke enumerable in de dictionary helemaal zal evalueren.

Eerder in het topic had je al een extension method om Between tedoen. Dus waarom niet gewoon een tuple (0x800, 0x807FF) opslaan en Between testen op dit tuple?

Dus dan hebben we een

code:

Dictionary<(int, int), RegisterType>

Maargoed, hierop kunnen we alsnog geen efficiente lookup doen.

Even een tussensprong op je huidige dictionary. Als je deze omdraait en platslaat dan heb je een dictionary die ruimte-inefficient is, maar wel hoe je hem zou willen gebruiken. Je mapt namelijk simpelweg elk mogelijk adres in de gespecificeerde ranges naar een RegisterType:

code:

var dictionary = new List<(IEnumerable<int> range, RegisterType type)>
    {
        (Enumerable.Range(0x8000, 0x807F), RegisterType.CNvr1),
        (Enumerable.Range(0x8000, 0x807F), RegisterType.CNvr2)
    }
    .SelectMany(t => t.range.Select(a => (address: a, type: t.type)))
    .ToDictionary(t => t.address, t => t.type);

Dit verdient nog steeds niet de schoonheidsprijs zou ik zeggen. En ik zou hier ook nog altijd de if of switch gebruiken. Beide zijn, als het niet te lange lijsten worden, mooier dan wat we tot nu toe hebben geproduceerd om daaromheen te komen.

[Ron]

Gomez12 schreef op dinsdag 8 mei 2018 @ 13:34:
[...]
oh, je pakt een dictionary, dan wil je dus op keys zoeken. Maar ik heb helemaal geen key, hoe zit dat nou? Oh wacht je gaat op value zoeken

Daar heb je wel een punt. uiteindelijk ben ik na wat nadenken toch voor de switch gegaan.
ook heb ik de dictionary volledig verwijderd. dit omdat hij eigenlijk alleen gebruikt werd op het moment dat bij het aanmaken van het memoryfield - object alleen het registertype bekend was.

hiermee een laag complexiteit verwijderd die eigenlijk nooit had moeten bestaan

[_Erikje_]

Alex) schreef op dinsdag 8 mei 2018 @ 02:50:

[...]

En wat voegt dat toe?

[...]

Ik vind het vooral moeilijk om te lezen en het is mij niet meteen duidelijk wat er gebeurt.

Een simpele lookup is makkelijker te lezen dan een uitgebreide if / else if. Het is ook makkelijker in onderhoud, stel er komt een nieuwe range bij in de toekomst, dan hoef je alleen de dictionary aan te passen, geen extra code vereist.

[Alex)]

Dat hangt af van hoe groot de range is. Als het gaat om minder dan 5 varianten dan blijft een constructie met if/else of switch/case veel eenvoudiger te lezen dan de code die lookups moet gaan doen in één of andere dictionary.

Als er keuzes groter zijn wordt het wel interessanter om een lookup te doen, inderdaad omwille van onderhoudbaarheid. Dan zou ik er echter niet een dictionary voor misbruiken, maar zou ik iets bouwen wat een Func<> accepteert

[Gomez12]

_Erikje_ schreef op dinsdag 8 mei 2018 @ 15:38:
[...]
Een simpele lookup is makkelijker te lezen dan een uitgebreide if / else if.

Een simpele wel, maar een dictionary pakken en dan naar de values gaan kijken is geen simpele lookup meer.

Het is ook makkelijker in onderhoud, stel er komt een nieuwe range bij in de toekomst, dan hoef je alleen de dictionary aan te passen, geen extra code vereist.

Dictionary aanpassen moet je dan wel even doen met een kronkel in je hoofd want je value is je key en je key is je value.
Oftewel pak er simpelweg geen dictionary voor, maar een bijna willekeurige andere collectie die iets logisch bevat hiervoor.

Naast dat ik verwacht dat het gewoon ruk zal presteren als je waarde niet ergens tegen het begin aanligt puur vanwege hoe IENumerable.Range werkt...

[_Erikje_]

Hebben we het over prestatie terwijl we de Dictionary maar een paar items bevat? Dat is een kwestie van milliseconden...

[Alex)]

De dictionary bevat weliswaar maar een paar items, maar er wordt geselect op basis van de values (huh?), waarin weer IEnumerable's zitten die elke keer opnieuw geënumereerd worden. En dat terwijl er alleen maar gecheckt hoeft te worden of een bepaalde waarde binnen een bepaalde range valt.

ThoNohT's oplossing is dan al ietsjes netter, maar zou ik er bij een code review alsnog uit laten halen. Nodeloos complex voor deze usecase (in zoverre we 'm kunnen beoordelen).

Afhankelijk van om hoeveel data het gaat is een oplossing met if/else of een switch prima. Als het gaat om véél statische data kun je altijd nog uitwijken naar een custom type en een simpele List<T> en een functie die de lookup doet.

Zoiets:

code:

public class FieldTypeMapping {
    public FieldTypeMapping(min, max, value)
    {
        Min = min;
        Max = max;
        Value = value;
    }

    public int Min { get; }
    public int Max { get; }
    public string Value { get; }
}

var mappings = new List<FieldTypeMapping>();
mappings.Add(new FieldTypeMapping(0, 10, "a"));
mappings.Add(new FieldTypeMapping(11, 20, "b"));
mappings.Add(new FieldTypeMapping(21, 30, "c"));

public string GetMapping(int value)
{
    return mappings.FirstOrDefault(x => x.Min <= value && x.Max >= value)?.Value;
}

[Twieka]

code:

            var startAddress = 500;

            int FieldType =
                startAddress >= 0x8000 && startAddress < 0x8080 ? 1 :
                startAddress >= 0x8080 && startAddress < 0x8100 ? 2 : 
                // ---
                0;

Weinig boilerplate, goed leesbaar, makkelijk onderhoudbaar voor een beperkt aantal ranges.

of zoiets:

code:

FieldType = (startAddress - 0x8000) / 0x80;

[Ron]

Alex) schreef op dinsdag 8 mei 2018 @ 22:33:
ing met if/else of een switch prima. Als het gaat om véél statische data kun je altijd nog uitwijken naar een custom type en een simpele List<T> en een functie die de lookup doet.

De uiteindelijke usecase is het samenvoegen van twee lijsten, namelijk een geordende en een waar allerlij waarden in kunnen zitten, met als enigste constraint de fieldType.

hierbij kijk ik dus in welk memory veld het ingevoerde adres zit.

voor nu is de pattern-matching switch een uitkomst. mocht ik ooit nog extra functionaliteit nodig hebben zal ik zowieso van het fieldType enum een apart type moeten maken, maar dat zien we dan wel.