Benutzerdefinierter Prüfsummenalgorithmus

2429
greatwolf

Vor einiger Zeit habe ich einen Prüfsummenalgorithmus aus einem MMO zurückentwickelt, um die Gültigkeit eines mit Chat verknüpften Elements zu überprüfen (ähnlich wie WoW ). Die Idee ist, dass, wenn die Prüfsumme ungültig ist, der Spielclient den Link beim Klicken ignoriert. Andernfalls würden beim Klicken auf den Element-Link im Chat die Statistik und das Attribut für dieses Element angezeigt.

ushort16 CreateChecksum(const string &itemlink)
{
    stringstream parseitemlink(itemlink);
    uint32 hexform[ITEMLINKGROUPCOUNT] = {0};
    uint32 hexsum = 0;

    //Parse itemLink string into hexform array
    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
        parseitemlink >> hex >> hexform[i];

    //sum all the itemlink group together
    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
        hexsum += hexform[i];

    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
    {
        uint32 ebx = hexform[i], edi = ebx * i;

        //if loop iteration is odd store MSB 2-bytes.
        //Otherwise, store working hexgroup as is untouched
        if (i & 0x01)
            ebx = hexform[i] >> 16; // aka same as dividing hexform[i] by 65,536

        // (hexform[i] / 65,536) + (hexform[i] * i) + hexsum -- for odd groups
        //  hexform[i] + (hexform[i] * i) + hexsum           -- for even groups
        ebx += edi + hexsum;
        hexsum = ebx ^ hexform[i];
    }

    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
    {
        // the more familiar high-level form would be
        // hexform[i]^2 + hexform[i] * hexsum
        uint32 ecx = (hexform[i] + 1) * hexsum,
               eax = ecx * hexform[i];

        eax >>= 16;
        eax += ecx;
        hexsum = eax ^ hexform[i];
    }

    //return the lower 2-bytes of hexsum
    //as the final checksum
    return hexsum & 0xFFFF;
}//CreateChecksum

Das Format von itemlinkbesteht aus einer Gruppe von Hexadezimalzeichen, die durch ein Leerzeichen im Zeichenfolgenformat getrennt sind. Es wird main()als Argument übergeben, wenn das Programm ausgeführt wird.

So itemlinkkönnte ein Hex-String aussehen:

const string EXAMPLELINK = "36c6a 0 3f000a54 d0f1 0 0 0 0 0 0 20d0";

Gibt es Codegerüche oder Lesbarkeitsprobleme in diesem Codesegment? Kann irgendein Teil davon verbessert werden?

Antworten
18

4 Antworten auf die Frage

9
CB Bailey

Wenn Sie die Standardbibliothek Klassen mit dem gleichen Namen verwenden, würde ich den folgenden Namen die richtigen Namensraum Qualifier geben: std::string, std::stringstream, std::hex.

In C ++ funktioniert das genauso gut, meiner Meinung nach ist es etwas idiomatischer.

uint32 hexform[ITEMLINKGROUPCOUNT] = {};

ebx, edi, ecx, eaxSind nicht gut Variablennamen, wenn Sie sie sinnvoller Namen geben können, dann tun.

    uint32 ecx = (hexform[i] + 1) * hexsum,
           eax = ecx * hexform[i];

Ich persönlich denke, das ist klarer:

    uint32 ecx = (hexform[i] + 1) * hexsum;
    uint32 eax = ecx * hexform[i];

Der Kommentar ist wirklich schlecht, weil er darüber spricht, hexform[i]^2 + hexform[i] * hexsumwährend er ecxden Wert hexform[i] * hexsum + hexsumund eaxden Wert erhält hexform[i]^2 * hexsum + hexform[i] * hexsum. Ich denke, der Kommentar benötigt ein Paar Klammern, wenn der Code das tut, was Sie meinten.

Um robust zu sein, sollten Sie prüfen, ob die Analyse funktioniert hat.

parseitemlink >> hex >> hexform[i];

Sie können die ersten beiden für Schleifen auch trivial kombinieren.

Die Variablennamen stammen vermutlich aus einer C ++ - Übersetzung einer Assembler-Routine. Wenn Sie also den Assembler-Code als "Spezifikation" betrachten, sind die Namen "eax, ecx" usw. eine gute Wahl. Roddy vor 10 Jahren 0
re: Namespace-Qualifikationsmerkmale: Wenn dieser Code in einer Header-Datei enthalten ist, würde ich zustimmen. Aber in einer .cpp-Datei würde ich persönlich lieber eine `using`-Anweisung sehen. Roddy vor 10 Jahren 1
@Roddy: Meinen Sie eine Verwendungsdeklaration oder eine Verwendungsrichtlinie? Ich wäre mit ausreichend using-Deklarationen in Ordnung - obwohl es sich für den `std`-Namespace kaum lohnt. Ich denke, dass Anwendungsrichtlinien nur äußerst selten eine gute Idee sind. CB Bailey vor 10 Jahren 2
7
Jamal
  • Funktions- und Variablennamen in C ++ sind normalerweise camelCase oder snake_case, während Großbuchstaben für benutzerdefinierte Typen stehen.

    Wenn ITEMLINKGROUPCOUNTeine Variable oder eine Konstante ist, sollte dies auch camelCase oder snake_case folgen. All-Caps-Benennung wird im Allgemeinen für Makros verwendet. Die verschiedenen Wörter sollten mit Unterstrichen getrennt werden.

  • uint32ist weder Standard C ++ noch ist es sogar C (was ist uint32_t). Gebrauch std::uint32_tvon <cstdint>. Mehr Infos dazu hier und hier .

  • Anstelle dieser Summenschleife:

    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
        hexsum += hexform[i];
    

    Verwendung std::accumulateals saubere und C ++ - wie Alternative:

    uint32 hexsum = std::accumulate(hexform, hexform+ITEMLINKGROUPCOUNT, 0);
    

    An anderer Stelle hexsumsollte hier initialisiert werden, da es dort zuerst verwendet wird. Halten Sie Variablen immer so nahe wie möglich.

5
Jerry Coffin

Es scheint mir, dass Ihr Code derzeit die Tatsache anzeigt, dass er aus der Assembler-Sprache etwas mehr Reverse-Engineering gemacht wurde, als ich möchte.

Ich würde versuchen, es in einer Form zu schreiben, die der von C ++ normalerweise ähnlich ist, anstatt die Assemblersprache im Wesentlichen in die C ++ - Syntax zu übersetzen, aber den Großteil der Assembler-Sprache beizubehalten (bis auf die Registernamen und noch immer einschließlich) für Ihre Variablen).

Das Lesen der Eingabezeichenfolge und das Konvertieren von Zeichenfolgen in einen Vektor von uint32 könnte beispielsweise wie folgt aussehen:

class hex_word {
    uint32 val;
public:
    friend std::istream& operator>>(std::istream &is, hex_word &h) { 
        return is >> hex >> h;
    }
    operator uint32() { return val; }
};

std::vector<uint32> hexform{std::istream_iterator<hex_word>(parseitemlink),
                            std::istream_iterator<hex_word>()};

Um diese zusammenzurechnen, können Sie Folgendes verwenden std::accumulate:

uint32 hexsum = std::accumulate(hexform, hexform+ITEMLINKGROUPCOUNT, 0);

Wenn Sie ein wenig weitergehen, könnte Ihre letzte Schleife Folgendes verwenden std::accumulate:

struct f {
    uint32 operator()(uint32 accumulator, uint32 val) {
        uint32 c = (val+1)*accumulator; 
        uint32 a = c * val;
        return ((a>>16)+c) ^ accumulator;
    }
};

return std::accumulate(hexform.begin(), hexform.end(), hexsum, f) * 0xffff;

Ich bin nicht sicher, ob dies zu aufregenden neuen Erkenntnissen oder einer dramatischen Vereinfachung des Codes führt, aber es erscheint mir immer noch einfacher zu verstehen als bei all dem Code, der zusammengefügt wurde.

4
indyK1ng

Die einzigen Dinge, die ich mit Problemen sehen kann, sind die Namen der Akronymvariablen, dh (eax, ecx, ebx und edi) erklären nicht, was die Variablen für jemanden, der keine Erfahrung mit Prüfsummen hat, eindeutig speichern.

Die andere Sache, die ich für Lesbarkeitsprobleme sehen kann, ist, dass alle Kleinbuchstaben-Variablennamen, die beschreibend sind (dh parseitemlink), camelCased sein sollten, damit ich die verschiedenen Wörter (z. B. parseItemLink) erkennen kann. Das ist das einzige, was mir in Sachen Lesbarkeit einfällt.