struct z bezsensowną wartością domyślną

12

W moim systemie często działają z kodami Airport ( "YYZ", "LAX", "SFO", itp), są one zawsze dokładnie w tym samym formacie (3 litery, reprezentowane jako wielkie litery). System zazwyczaj zajmuje się 25-50 z tych (różnych) kodów na każde żądanie API, z sumą ponad tysiąca alokacji, są one przekazywane przez wiele warstw naszej aplikacji i dość często porównywane.

Zaczęliśmy od przekazywania ciągów znaków, co działało trochę dobrze, ale szybko zauważyliśmy wiele błędów programistycznych, przekazując niewłaściwy kod, gdzie oczekiwano 3-cyfrowego kodu. Natknęliśmy się również na problemy, w których mieliśmy wykonać porównanie bez rozróżniania wielkości liter, a zamiast tego nie wystąpiły błędy, co spowodowało błędy.

Z tego postanowiłem przestać przekazywać ciągi znaków i stworzyć Airportklasę, która ma jednego konstruktora, który pobiera i sprawdza kod lotniska.

public sealed class Airport
{
    public Airport(string code)
    {
        if (code == null)
        {
            throw new ArgumentNullException(nameof(code));
        }

        if (code.Length != 3 || !char.IsLetter(code[0]) 
        || !char.IsLetter(code[1]) || !char.IsLetter(code[2]))
        {
            throw new ArgumentException(
                "Must be a 3 letter airport code.", 
                nameof(code));
        }

        Code = code.ToUpperInvariant();
    }

    public string Code { get; }

    public override string ToString()
    {
        return Code;
    }

    private bool Equals(Airport other)
    {
        return string.Equals(Code, other.Code);
    }

    public override bool Equals(object obj)
    {
        return obj is Airport airport && Equals(airport);
    }

    public override int GetHashCode()
    {
        return Code?.GetHashCode() ?? 0;
    }

    public static bool operator ==(Airport left, Airport right)
    {
        return Equals(left, right);
    }

    public static bool operator !=(Airport left, Airport right)
    {
        return !Equals(left, right);
    }
}

Dzięki temu nasz kod był znacznie łatwiejszy do zrozumienia i uprościliśmy nasze kontrole równości, słownik / zestaw zastosowań. Wiemy teraz, że jeśli nasze metody zaakceptują Airportwystąpienie, które będzie zachowywać się tak, jak się spodziewamy, uprościło nasze sprawdzanie metod do zerowego sprawdzania referencji.

Zauważyłem jednak, że zbieranie śmieci działało znacznie częściej, co prześledziłem do wielu przypadków Airportzbierania.

Moim rozwiązaniem tego było przekonwertowanie classpliku na struct. Przeważnie była to tylko zmiana słowa kluczowego, z wyjątkiem GetHashCodei ToString:

public override string ToString()
{
    return Code ?? string.Empty;
}

public override int GetHashCode()
{
    return Code?.GetHashCode() ?? 0;
}

Do obsługi przypadku, w którym default(Airport)jest używany.

Moje pytania:

  1. Czy tworzenie Airportklasy lub struktury było ogólnie dobrym rozwiązaniem, czy też rozwiązuję niewłaściwy problem / rozwiązuję go w niewłaściwy sposób, tworząc typ? Jeśli nie jest to dobre rozwiązanie, jakie jest lepsze rozwiązanie?

  2. Jak moja aplikacja powinna obsługiwać instancje, w których default(Airport)jest używana? Rodzaj aplikacji default(Airport)jest nonsensowny dla mojej aplikacji, więc robiłem to if (airport == default(Airport) { throw ... }w miejscach, w których uzyskanie wystąpienia Airport(i jego Codewłaściwości) ma kluczowe znaczenie dla operacji.

Uwagi: Przejrzałem pytania dotyczące struktury C # / VB - jak uniknąć przypadku z zerowymi wartościami domyślnymi, które są uważane za nieprawidłowe dla danej struktury? , i użyj struct lub nie przed zadaniem pytania, jednak myślę, że moje pytania są na tyle inne, że uzasadniają swój post.

Mateusz
źródło
7
Czy wyrzucanie elementów bezużytecznych ma istotny wpływ na działanie aplikacji? Innymi słowy, czy to ma znaczenie?
Robert Harvey,
W każdym razie tak, rozwiązanie klasowe było „dobre”. Sposób, w jaki wiesz, że rozwiązał problem bez tworzenia nowych.
Robert Harvey,
2
Jednym ze sposobów rozwiązania tego default(Airport)problemu jest po prostu niedozwolone wystąpienia domyślne. Możesz to zrobić, pisząc konstruktor bez parametrów i rzucając go InvalidOperationExceptionlub NotImplementedExceptionw nim.
Robert Harvey,
3
Na marginesie, zamiast potwierdzać, że łańcuch inicjujący ma w rzeczywistości 3 znaki alfanumeryczne, dlaczego nie porównać go do skończonej listy wszystkich kodów lotniska (np. Github.com/datasets/airport-codes lub podobnych)?
Dan Pichelman,
2
Chcę postawić kilka piw, że nie jest to przyczyną problemów z wydajnością. Normalny laptop może przydzielić w kolejności 10 mln obiektów / sekundę.
Esben Skov Pedersen

Odpowiedzi:

6

Aktualizacja: Przepisałem swoją odpowiedź, aby odnieść się do niektórych niepoprawnych założeń dotyczących struktur C #, a także OP informującego nas w komentarzach, że używane są internowane łańcuchy.


Jeśli możesz kontrolować dane przychodzące do systemu, skorzystaj z klasy zamieszczonej w pytaniu. Jeśli ktoś uruchomi się default(Airport), odzyska nullwartość. Pamiętaj, aby napisać swoją prywatną Equalsmetodę zwracającą fałsz przy porównywaniu pustych obiektów lotniska, a następnie pozwól NullReferenceException, aby latały gdzie indziej w kodzie.

Jeśli jednak pobierasz dane do systemu ze źródeł, którymi nie kontrolujesz, nie musisz zawieszać całego wątku. W takim przypadku struct jest idealny, ponieważ prosty fakt default(Airport)da ci coś innego niż nullwskaźnik. Zrób oczywistą wartość reprezentującą „brak wartości” lub „wartość domyślną”, abyś miał coś do wydrukowania na ekranie lub w pliku dziennika (na przykład „---”). W rzeczywistości chciałbym po prostu zachować codeprywatność i w ogóle nie ujawniać Codenieruchomości - po prostu skup się na zachowaniu tutaj.

public struct Airport
{
    private string code;

    public Airport(string code)
    {
        // Check `code` for validity, throw exceptions if not valid

        this.code = code;
    }

    public override string ToString()
    {
        return code ?? (code = "---");
    }

    // int GetHashcode()

    // bool Equals(...)

    // bool operator ==(...)

    // bool operator !=(...)

    private bool Equals(Airport other)
    {
        if (other == null)
            // Even if this method is private, guard against null pointers
            return false;

        if (ToString() == "---" || other.ToString() == "---")
            // "Default" values should never match anything, even themselves
            return false;

        // Do a case insensitive comparison to enforce logic that airport
        // codes are not case sensitive
        return string.Equals(
            ToString(),
            other.ToString(),
            StringComparison.InvariantCultureIgnoreCase);
    }
}

W gorszym przypadku konwersja default(Airport)na ciąg znaków jest drukowana "---"i zwraca wartość false w porównaniu z innymi prawidłowymi kodami lotniska. Każdy „domyślny” kod lotniska nie pasuje do niczego, w tym inne domyślne kody lotniska.

Tak, struktury mają być wartościami przydzielonymi na stosie, a wszelkie wskaźniki pamięci sterty w zasadzie negują zalety wydajności struktur, ale w tym przypadku domyślna wartość struktury ma znaczenie i zapewnia dodatkową odporność na pociski na resztę podanie.

Z tego powodu zgiąłbym trochę reguły.


Oryginalna odpowiedź (z pewnymi błędami faktycznymi)

Jeśli możesz kontrolować dane przychodzące do twojego systemu, zrobiłbym to, co zasugerował Robert Harvey w komentarzach: Utwórz konstruktora bez parametrów i wyrzuć wyjątek, gdy zostanie wywołany. Zapobiega to przedostawaniu się nieprawidłowych danych do systemu za pośrednictwem default(Airport).

public Airport()
{
    throw new InvalidOperationException("...");
}

Jeśli jednak pobierasz dane do systemu ze źródeł, którymi nie kontrolujesz, nie musisz zawieszać całego wątku. W takim przypadku możesz utworzyć niepoprawny kod lotniska, ale sprawiający wrażenie oczywistego błędu. Wymagałoby to stworzenia konstruktora bez parametrów i ustawienia Codeczegoś takiego jak „---”:

public Airport()
{
    Code = "---";
}

Ponieważ używasz a stringjako kodu, nie ma sensu używać struct. Struktura jest przydzielana na stosie, tylko w celu Codeprzypisania jej jako wskaźnika do łańcucha w pamięci sterty, więc nie ma tutaj różnicy między klasą a strukturą.

Jeśli zmienisz kod lotniska na 3-elementową tablicę znaków, struktura zostanie w pełni przydzielona na stosie. Nawet wtedy ilość danych nie jest tak duża, aby coś zmienić.

Greg Burghardt
źródło
Gdyby moja aplikacja używała wewnętrznych ciągów dla Codewłaściwości, czy zmieniłoby to twoje uzasadnienie dotyczące tego, że ciąg jest w pamięci sterty?
Matthew
@Matthew: Czy korzystanie z klasy powoduje problemy z wydajnością? Jeśli nie, przerzuć monetę, aby zdecydować, którego użyć.
Greg Burghardt,
4
@Matthew: Naprawdę ważną rzeczą jest scentralizowanie kłopotliwej logiki normalizacji kodów i porównań. Po tym „klasa kontra struktura” to tylko dyskusja akademicka, dopóki nie zmierzysz wystarczająco dużego wpływu na wydajność, aby uzasadnić dodatkowy czas programisty na dyskusję akademicką.
Greg Burghardt,
1
To prawda, nie przeszkadza mi od czasu do czasu dyskusja akademicka, jeśli pomoże mi to w tworzeniu bardziej świadomych rozwiązań w przyszłości.
Matthew
@Matthew: Tak, masz absolutną rację. Mówią „rozmowa jest tania”. Jest to z pewnością tańsze niż nie mówienie i budowanie czegoś źle. :)
Greg Burghardt,
13

Użyj wzoru Flyweight

Ponieważ lotnisko jest, niezmiennie, niezmienne, nie ma potrzeby tworzenia więcej niż jednego wystąpienia konkretnego, powiedzmy, SFO. Użyj Hashtable lub podobnego (uwaga, jestem facetem Java, nie C #, więc dokładne szczegóły mogą się różnić), aby buforować lotniska po ich utworzeniu. Przed utworzeniem nowego sprawdź w Hashtable. Nigdy nie zwalniasz lotnisk, więc GC nie musi ich zwalniać.

Jedną dodatkową niewielką zaletą (przynajmniej w Javie, nie jestem pewien co do C #) jest to, że nie musisz pisać equals()metody, wystarczy proste ==. To samo dotyczy hashcode().

użytkownik949300
źródło
3
Doskonałe wykorzystanie wzoru flyweight.
Neil
2
Zakładając, że OP nadal używa struktury, a nie klasy, czy internowanie łańcucha nie obsługuje już wartości ciągu wielokrotnego użytku? Struktury już działają na stosie, łańcuchy są już ponownie używane, aby uniknąć powielania wartości w pamięci. Jakie dodatkowe korzyści można uzyskać dzięki wzorowi masy własnej?
Flater,
Coś, na co trzeba uważać. Jeśli lotnisko zostanie dodane lub usunięte, będziesz chciał zbudować sposób, aby odświeżyć tę listę statyczną bez ponownego uruchamiania aplikacji lub ponownego wdrażania. Lotniska nie są często dodawane ani usuwane, ale właściciele firm często się denerwują, gdy prosta zmiana staje się tak skomplikowana. „Czy nie mogę po prostu dodać go gdzieś ?! Dlaczego musimy zaplanować ponowne uruchomienie wersji / aplikacji i niedogodności dla naszych klientów?” Ale na początku myślałem też o użyciu statycznej pamięci podręcznej.
Greg Burghardt,
@Flater Rozsądny punkt. Powiedziałbym, że mniejsze potrzeby młodszych programistów do uzasadnienia stosu zamiast sterty. Plus zobacz mój dodatek - nie trzeba pisać równa się ().
user949300,
1
@Greg Burghardt Jeśli getAirportOrCreate()kod jest poprawnie zsynchronizowany, nie ma technicznego powodu, aby nie tworzyć nowych portów lotniczych w razie potrzeby podczas działania. Mogą istnieć powody biznesowe.
user949300,
3

Nie jestem szczególnie zaawansowanym programistą, ale czy nie byłoby to idealne zastosowanie dla Enum?

Istnieją różne sposoby konstruowania klas enum z list lub ciągów. Oto jeden, który widziałem w przeszłości, ale nie jestem pewien, czy to najlepszy sposób.

https://blog.kloud.com.au/2016/06/17/converting-webconfig-values-into-enum-or-list/

Adam B.
źródło
2
Gdy potencjalnie istnieją tysiące różnych wartości (jak w przypadku kodów lotniskowych), wyliczenie po prostu nie jest praktyczne.
Ben Cottrell,
Tak, ale link, który zamieściłem, pokazuje, jak ładować ciągi jako znaki wyliczeniowe. Oto kolejny link do załadowania tabeli odnośników jako wyliczenia. To może być trochę pracy, ale skorzystałoby z mocy enum. wyjątekfound.net/…
Adam B
1
Lub listę prawidłowych kodów można załadować z bazy danych lub pliku. Następnie sprawdza się kod lotniska, aby znaleźć się na tej liście. To jest zwykle wykonywane, gdy nie chcesz już kodować wartości i / lub lista staje się zbyt długa do zarządzania.
Neil
@BenCottrell właśnie do tego służą kody gen i szablony T4.
RubberDuck,
3

Jednym z powodów, dla których widzisz więcej aktywności GC, jest to, że tworzysz teraz drugi ciąg znaków - .ToUpperInvariant()wersję oryginalnego ciągu. Oryginalny ciąg kwalifikuje się do GC zaraz po uruchomieniu konstruktora, a drugi kwalifikuje się w tym samym czasie, co Airportobiekt. Możesz być w stanie zminimalizować go w inny sposób (zwróć uwagę na trzeci parametr string.Equals()):

public sealed class Airport : IEquatable<Airport>
{
    public Airport(string code)
    {
        if (code == null)
        {
            throw new ArgumentNullException(nameof(code));
        }

        if (code.Length != 3 || !char.IsLetter(code[0])
                             || !char.IsLetter(code[1]) || !char.IsLetter(code[2]))
        {
            throw new ArgumentException(
                "Must be a 3 letter airport code.",
                nameof(code));
        }

        Code = code;
    }

    public string Code { get; }

    public override string ToString()
    {
        return Code; // TODO: Upper-case it here if you really need to for display.
    }

    public bool Equals(Airport other)
    {
        return string.Equals(Code, other?.Code, StringComparison.InvariantCultureIgnoreCase);
    }

    public override bool Equals(object obj)
    {
        return obj is Airport airport && Equals(airport);
    }

    public override int GetHashCode()
    {
        return Code.GetHashCode();
    }

    public static bool operator ==(Airport left, Airport right)
    {
        return Equals(left, right);
    }

    public static bool operator !=(Airport left, Airport right)
    {
        return !Equals(left, right);
    }
}
Jesse C. Slicer
źródło
Czy nie daje to różnych kodów skrótu dla równych (ale różnie skapitalizowanych) lotnisk?
Hero Wanders
Tak, wyobrażam to sobie. Cholera.
Jesse C. Slicer,
To bardzo dobra uwaga, nigdy o tym nie myślałem. Spróbuję wprowadzić te zmiany.
Matthew
1
W odniesieniu do GetHashCode, powinien po prostu użyć StringComparer.OrdinalIgnoreCase.GetHashCode(Code)lub podobny
Matthew