Betreff: Hashfunktion
Servus. Hab mir mal ein bisschen den Code der Hashfunktionen angekuckt. Dabei hab ich mich ein paar dinge gefragt:
hash.h, Zeile 17, 18:
Code: Alles auswählen
ypedef unsigned long long uint64__;
typedef unsigned long uint32__;
B) unsigned long ist nicht immer 32 bit groß (auf 64 Bit Unix ist es 64 bit groß!), aber mindestens. Passt also eh, aber du sollst nicht überrascht sein.
hash.h, Zeile 20:
Code: Alles auswählen
using namespace std;
hash.h, Zeile 30-70:
1) Warum hast du denn die Funktionen so dicht gepackt? Sind dir die Leerzeilen ausgegangen Klar, ist geschmackssache, aber ich finde wenn alles zusammenklebt siehts nicht sehr Hübsch und Leserlich.
2) Gefallen dir die Dreifachstriche von Doxygen tatsächlich besser? Also ich mach da immer gern das:
Code: Alles auswählen
/**
*
*
*/
hash.h, Zeile 49:
Du typedef'st dir extra einen hashvar Typ und dann gibt die Funktion statt einem "Hash" (was auch immer das ist) ein uint32__ zurück?
hash.h, Zeile 63:
Ich finde dass overloads gruppiert werden sollten, ist aber Geschmackssache. (Also static uint32__ GetHash(void* data, uint64__ length); mit uint32__ GetHash())
Zeile 73 - 82:
1) Const correctness nicht so ganz toll... Ob das zurückgegebene Bool jetzt const ist oder nicht ist mir ziemlich Wurst. Aber wenn die funktion selbst nicht const ist, kannst du konstante Objekte der Hash klasse nicht vergleichen! Also ich würde das const vorne wegtun und dafür nach hinten verfrachten.
2) Da diese Klasse nur eine kleine Variable hast ist es recht egal. Allerdings ist es eher üblich die zu vergleichenden Klassen per Referenz zu übergeben, und nicht zu kopieren, da sonst eventuell viele Daten kopiert werden müssten. Also mach einfach aus const Hash32 h1 -> const Hash32& h1
3)
Die funktion könnte man auch kürzer schreiben:
Code: Alles auswählen
const bool operator== ( const Hash32& h1 )
{
return hash == h1.hash; }
hash.h: 92-ende:
Gleiches wie oben. Obwohl, da fällt mir auf: Warum eigentlich? Zweimal die gleiche Klasse? Ist doch Code duplication. Das wäre eigentlich ein Musterbeispiel für templates.
Willst
hash32.cpp, hash64.cpp:
Hm. Die Dateien sind eigentlich bis auf die Typen identisch
Ist das für dich leicht zu warten, wenn du jede änderung 2 mal machen musst? Bitte sieh dir wirklich mal Templates an. Ist nicht so kompliziert.
hash32.h, Zeile 7:
Code: Alles auswählen
using namespace std;
hash64.h, Zeile 5:
Code: Alles auswählen
#include <iostream>
Allgemein:
Du schreibst aber schon sehr dicht... Also ich finde man bräuchte mehr Leerzeilen, aber vor allem mehr Kommentare! Ist ja nicht so einfach zu lesen der Code. Hier und da mal ne erklärung wär schon toll ^^
Zu den Leerzeilen: Ich schreib eigentlich meistens zwischen jedem Anweisungsblock ne leere Zeile rein, und lass eben nur die Anweisungen die "Zusammengehören" ohne Leerzeile. Ich finde das hilft sehr bei der lesbarkeit.
hash32.cpp, Zeile 109:
Code: Alles auswählen
data = new char[104857600];
Achja stimmt, da hatten wir im Thread auch schon geredet. Egal, ich sags einfach nochmal:
1) Es ist nicht sehr kluk in jedem Schleifendurchlauf den speicher zu allokieren und dann wieder zu löschen
2) Waaarum genau brauchst du den Speicher? Forderst du einfach mal zur sicherheit 100 MB an? Du solltest eher genau die Benötigte Speichermenge allokieren, die kennst du ja durch die Datei! Erst ab einem bestimmten Schwellenwert solltest du das in einer Schleife machen. Und dann solltest du auch nicht einfach irgendeine Zahl reinschreiben, und zwar ganz ohne kommentare und nix...
3) Und warum muss ifstream *file jetzt ein Zeiger sein? Kannst du nicht einfach in jedem Durchlauf file.close() und file.open() machen?
AAALLSSOOO, das mit den Templates. Eigentlich ganz einfach:
Du schreibst über die Klasse drüber, dass du aus der Klasse gerne ein Template gemacht hättest:
Code: Alles auswählen
template <typename HashType>
class Hash
{
...
Wenn du die Klasse jetzt instanziierst musst du in spitze Klammern den Templateparameter übergeben:
Hash<long long int> hash;
Die Funktionen müssen dann leider aus den .cpp files in den Header. Am einfachsten tust du sie einfach in die Klasse rein, aber schöner wäre es außerhalb, die Syntax ist etwas komplexer. Die sieht so aus:
Code: Alles auswählen
template <typename HashType>
HashType Hash<HashType>::getHash(void* data, uint64__ length)
Und schon hast du ein Astreines Template
Ein bisschen herumspielen müsste man mit der Magischen Zahl, aber das würden wir auch noch hinkriegen, wenn du interessiert bist.
Ich habs dir jetzt per PN geschickt weil das vl. nicht für alle interessant ist. Wenn du willst kann ichs aber gern in den Thread posten.
Mfg, und Sorry für soviel Text,
fat-lobyte