Hashfunktion - Feedback

Proggen.org - Lernprojekt: Duplikatefinder
Benutzeravatar
fat-lobyte
Beiträge: 1398
Registriert: Sa Jul 05, 2008 12:23 pm
Wohnort: ::1
Kontaktdaten:

Hashfunktion - Feedback

Beitrag von fat-lobyte » Mo Jul 18, 2011 1:15 pm

Hallo, nach einer Diskussion mit cloidnerux per PN, die etwas länger wurde als geplant haben wir uns entschieden das in einem öffentlich lesbaren Thread weiterzuführen. Bitte lesen und evtl. mitdiskutieren.

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__;
A) long long ist nicht wirklich (noch nicht) standardkonform, obwohl das die Compiler unterstützen sollten. Wollt ich dir nur mitteilen :-)
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;
Wieso eigentlich? In Headern ist das eher unüblich, da ALLE Source-Files, die den Header inkludieren nun den namespace std:: usen, was vl. nicht gewünscht ist. Packs doch einfach in die Source files, du verwendest im Header anscheinend eh keine std:: Funktionen.

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

/**
*
*
*/
Aber das ist ebenfalls geschmackssache.

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;
Siehste. Brauchst du im Header gar nicht :-)


hash64.h, Zeile 5:

Code: Alles auswählen

#include <iostream>
fehlt.

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];
Whoooww... Was hast du denn damit vor? Willst du ne Bank überfallen?? Was machst du mit so viel Speicher?!?
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
{
...
Typename ist ein Schlüsselwort, und bedeutet dass das Template einen Typ als parameter erwartet. Diesen Parameter haben wir HashType genannt.

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)
Im Code musst du jetzt nur mehr jedes Vorkommnis von uint64__ und uint32__ durch HashType ersetzen.
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
Haters gonna hate, potatoes gonna potate.

Benutzeravatar
fat-lobyte
Beiträge: 1398
Registriert: Sa Jul 05, 2008 12:23 pm
Wohnort: ::1
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von fat-lobyte » Mo Jul 18, 2011 1:16 pm

cloidnerux hat geschrieben:
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.
Wers geschrieben hat, kommt mit allem zurecht^^
Aber ich mag es nicht wenn es zu sehr auseinandergefleddert ist.
2) Gefallen dir die Dreifachstriche von Doxygen tatsächlich besser? Also ich mach da immer gern das:
Die Dreifachstriche habe ich eher aus "Faulheit" gemacht, denn sie sind Standard in C# als Kommentar und so muss ich mir nicht 2 Syntaxe merken...
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?
Ein hash ist immer eine Zahl, je nach Klasse 32 oder 64-Bit groß. Die Idee hinter hashvar war egt, das man statt den uint32__/64 hashvar schreibt und wenn man auf 64-Bit umsteigen will, ändert man das typedef, was aber nicht so funktioniert hat.
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
danke für den Hinweis :D
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.
Das stimmt wohl, was mir aber während der Entwicklung nicht so bewusst war. Ich hab die hash32-Klasse geschrieben und dann einfach die hash64 dazugepackt, weil es sich ja nicht so groß unterscheidet.
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?
Klug ist es warscheinlich nicht, aber eine Lösung. Du hast aber recht, es ist egt unnötig. Das mit den 100MB sind auch mehr "faulheit": Warum soll ich eine Routine schreiben, die erst den maximalen RAM ausliest, dann die Größe der Datei ermittelt und dann entscheidet ob es jetzt Sinnvoll wäre, es in kleineren Packeten zu Lesen oder nicht. Das egt Problem ist halt, das ich nicht mehr Speicher anfordern kann als verfügbar, da es sonst zu Fehlern kommt. Zudem sind die Festplatten nicht so schnell, als das wir so viel zeit gewinnen würden, alles an einem Stück zu lesen.
Mfg, und Sorry für soviel Text,
fat-lobyte
Es soll nicht mein Problem sein, wenn du viel schreibst. Aber ich bedanke mich für das Feedback.

MfG cloidnerux.
Haters gonna hate, potatoes gonna potate.

Benutzeravatar
fat-lobyte
Beiträge: 1398
Registriert: Sa Jul 05, 2008 12:23 pm
Wohnort: ::1
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von fat-lobyte » Mo Jul 18, 2011 1:16 pm

cloidnerux hat geschrieben:
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.
Wers geschrieben hat, kommt mit allem zurecht^^
Aber ich mag es nicht wenn es zu sehr auseinandergefleddert ist.
Das ist fair, aber in einem Projekt solltest du Rücksicht auf deine Mitarbeiter nehmen. Nachdem ich keinen Code beisteuere hab ich kein Mitspracherecht, aber frag mal Bebu und Xin was die davon halten.

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?
Ein hash ist immer eine Zahl, je nach Klasse 32 oder 64-Bit groß. Die Idee hinter hashvar war egt, das man statt den uint32__/64 hashvar schreibt und wenn man auf 64-Bit umsteigen will, ändert man das typedef, was aber nicht so funktioniert hat.
Aha, alles klar. Ein weiterer Punkt für Templates :-)

Klug ist es warscheinlich nicht, aber eine Lösung. Du hast aber recht, es ist egt unnötig. Das mit den 100MB sind auch mehr "faulheit": Warum soll ich eine Routine schreiben, die erst den maximalen RAM ausliest, dann die Größe der Datei ermittelt und dann entscheidet ob es jetzt Sinnvoll wäre, es in kleineren Packeten zu Lesen oder nicht.
Eigentlich nur eine Zeile, und den Ausdruck verwendest du schon:

Code: Alles auswählen

char data = new char[(104857600 > length)?(length):(104857600)]
Das mit den 100 MB ist fair, wenn man über die Zahl selbst auch diskutieren könnte. Aber wenn man weniger braucht sollte man weniger anfordern. Wenn man mehr braucht sollte man dann (mit dem gleichen Speicher!) blockweise einlesen.
Das egt Problem ist halt, das ich nicht mehr Speicher anfordern kann als verfügbar, da es sonst zu Fehlern kommt. Zudem sind die Festplatten nicht so schnell, als das wir so viel zeit gewinnen würden, alles an einem Stück zu lesen.
Du musst ja nicht mehr anfordern als verfügbar ist, eben nur diese 100 oder wie auch immer MB.
Naja, mach erstmal wie du meinst, ich denke du hast selbst schon eine Idee wie du das am besten löst und ich will dir nicht zuviel reinreden.
Es soll nicht mein Problem sein, wenn du viel schreibst. Aber ich bedanke mich für das Feedback.
Naja, lesen musst es trotzdem du :-)
Freut mich, hoffe es hilft auch.
Haters gonna hate, potatoes gonna potate.

Benutzeravatar
fat-lobyte
Beiträge: 1398
Registriert: Sa Jul 05, 2008 12:23 pm
Wohnort: ::1
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von fat-lobyte » Mo Jul 18, 2011 1:17 pm

cloidnerux hat geschrieben:
Das ist fair, aber in einem Projekt solltest du Rücksicht auf deine Mitarbeiter nehmen. Nachdem ich keinen Code beisteuere hab ich kein Mitspracherecht, aber frag mal Bebu und Xin was die davon halten.
Das ist wieder ein Typisches Problem der Gruppenarbeit. Du kannst egt nicht gleichzeitig mit mehreren an derselben Sache Arbeiten, da stehen sie sich alle nur auf den Füßen, wie hier geschehen.
Das haben schon andere Erkannt und die Projektplanung eingeflochten: Es wird ein Interface definiert und die Aufgaben auf die Entwickler aufgeteilt. Jeder macht sein Teil und wenn es Probleme gibt, sollen sie mit dem Entsprechenden Entwickler abgesprochen werden, eigentlich. Wenn man natürlich schnell testen will und Code wegen ein paar Syntax-Fehlern nicht Funktioniert, da will man ihn halt selber ändern, sodass man eben nicht ewig auf den Fix des anderen warten muss. Und hier beginnt das Chaos, denn jetzt müssen 2 bis 3 Versionen des Selben Quelltextes verwaltet werden(Branch - Trunk - Fix) und zusammengeflochtet werden. Und das gibt definitiv Probleme.
Wo wir wieder bei dem Sinn des ganzen sind.
Das code lesbar sein soll, ist verständlich, es darf aber nicht unter dem Vorwand entstehen, das mehrere auf einmal daran Arbeiten können.

Das eher schlimmere Problem daran ist egt, das mein Source-Code weit entfernt von perfekt ist, weil jetzt das Komplette Klassenkonzept über den Haufen geschmissen wird.

Mein Konzept sah vor, die hash-Klasse als Wrapper zu bauen, welcher den hash und die Generierungsfunktionen generisch kapselt und zur Verfügung stellt, was man an den Funktionssignaturen sehen kann(void *data, uint64__ length).
Dann bat mich Bebu um eine Funktion die sein Filestream an nimmt und jedem FileInfo Objekt seinen hash übergibt, aber nicht als Klasse sondern als Zahl. Damit wird die ganze Klasse egt unsinnig, da erst eine Instanz der Klasse erstellt werden muss um dann in einer egt Statischen Funktion die Dateien zu hashen.
Hinzu kommt das Problem mit den großen Dateien, 32/64-Bit und den templates.
Ich könnte das Komplette Design über den Haufen werfen und eine 30 Zeilen Implementation der "HashMultiplyFileInfo"-Funktion bauen, die das dann einfach macht.

Und hier sind wir wider bei der Projektplanung: Ich baute eine Generische Klasse ohne weitere nicht-Standard Abhängigkeiten und dem Ziel, das die Klasse nur Daten hashte und sonst nix. Und dann hat man mich gebeten, einen Teil der Funktionalität, die egt nicht in der Hashklasse liegen sollte, eben dort zu Implementieren: Das Abarbeiten eines FileInfo-Streams. Nun muss ich in einer "generischen" in einer egt "statischen" Memberfunktion mich mit dem Laden und Auswerten von Daten herumschlagen und das ist meiner Meinung nach schwachsinnig, auch weil bebu das Interface zwischenzeitlich wieder geändert hat...
Das mit den 100 MB ist fair, wenn man über die Zahl selbst auch diskutieren könnte. Aber wenn man weniger braucht sollte man weniger anfordern. Wenn man mehr braucht sollte man dann (mit dem gleichen Speicher!) blockweise einlesen.
Erstmal war das ganze ein Fix, der selber gefixt werden muss :D
Was ich noch falsch gemacht habe, war das mit dem häufigen "new" und "delte", das beansprucht zu sehr die Speicherveraltung des Betriebssystems und das muss ja nicht sein, einmal angefordert hätte gereicht, bzw ein normales Array.
Dann zu den 100MB an sich: Jeder etwas moderne Rechner hat genug RAM(1GB und mehr) und verkraftet sowas. Die Meiste Zeit ist der Ram sowieso halb lehr, also kann es nicht das Problem sein. Zum anderen kann ich aber keine 4GB Dateien auf einmal in den Ram laden, wie vorher geschehen. Deswegen habe ich den Kompromiss gewählt: Egal wie groß die Datei ist, ich fordere 100MB an, lade bis zur Dateigröße oder 100MB, hashe sie und lade die nächsten 100MN, bzw bis zum Dateiende. So muss ich nicht auf ganz so viel Rücksicht nehmen...(faulheit ^^)
Naja, mach erstmal wie du meinst, ich denke du hast selbst schon eine Idee wie du das am besten löst und ich will dir nicht zuviel reinreden.
Kritik schadet nicht, Diskutieren hält auf :D
Ich habe nichts dagegen über den Sinn und Unsinn (meiner) Algorithmen zu Diskutieren, denn ich bin auch nicht der intelligenteste Mensch der Welt und bei weitem nicht Fehlerfrei, daher kann eine andere Sichtweise nicht schaden.
Naja, lesen musst es trotzdem du
Freut mich, hoffe es hilft auch.
Ich muss es weder Lesen noch beantworten, das ich es aber mache, zeigt doch das ich nicht ganz so Ignorant bin^^
Kritik zwingt zum nachdenken und das hat bisher immer geholfen :)

Wenn des aber so weiter geht, sollten wir das ganze auf anderen Kommunikationswegen weiterführen.

MfG cloidnerux
Haters gonna hate, potatoes gonna potate.

Benutzeravatar
fat-lobyte
Beiträge: 1398
Registriert: Sa Jul 05, 2008 12:23 pm
Wohnort: ::1
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von fat-lobyte » Mo Jul 18, 2011 1:17 pm

cloidnerux hat geschrieben:Das haben schon andere Erkannt und die Projektplanung eingeflochten: Es wird ein Interface definiert und die Aufgaben auf die Entwickler aufgeteilt. Jeder macht sein Teil und wenn es Probleme gibt, sollen sie mit dem Entsprechenden Entwickler abgesprochen werden, eigentlich.
Sollte auch machbar sein.
Wenn man natürlich schnell testen will und Code wegen ein paar Syntax-Fehlern nicht Funktioniert, da will man ihn halt selber ändern, sodass man eben nicht ewig auf den Fix des anderen warten muss. Und hier beginnt das Chaos, denn jetzt müssen 2 bis 3 Versionen des Selben Quelltextes verwaltet werden(Branch - Trunk - Fix) und zusammengeflochtet werden. Und das gibt definitiv Probleme.
Richtig. Und genau deswegen haben Syntaxfehler in der Trunk nix verloren. Rein gar nichts. Zuerst testen, testen, testen und DANN erst einchecken. Auf den branches ist das was anderes, da kannst du machen was du willst. Das war übrigens auch mein Vorschlag in den Subversion-Richtlinien: http://www.proggen.org/doku.php?id=proj ... _the_build
Hätte keiner den Build kaputtgemacht, hätte auch keiner in die Arbeit der anderen hineinpfuschen müssen. Und den build hast bis jetzt nur du kaputtgemacht.

Das code lesbar sein soll, ist verständlich, es darf aber nicht unter dem Vorwand entstehen, das mehrere auf einmal daran Arbeiten können.
Nein, das war auch nicht der Vorwand. Der Grund sind zwei Dinge:
1) Wenn du in 4 jahren den Code nochmal ankuckst hast du keine Ahnung mehr was der tut. Das ist tatsache. Das heißt der Code ist im prinzip unwartbar. Also im prinzip kannst du ihn gleich in die Tonne kloppen.
2) Solltest du, aus welchen Gründen auch immer, aus dem Projekt aussteigen kann den Code auch keiner mehr lesen. Es gibt nichts schlimmeres als wenn man sich code von anderen leuten akuckt und sich Fragt warum er was geschrieben hat und ob man das jetzt gefahrlos ändern kann. Selbst kommentiert ist das schwer genug, aber so unmöglich. Also kann man den Code dann ebenfalls in die Tonne kloppen.

Nimms nicht persönlich, aber dein Argument ist jetzt "ich weiß eh wies funktioniert". Bin ich nicht so der fan davon.
Mein Konzept sah vor, die hash-Klasse als Wrapper zu bauen, welcher den hash und die Generierungsfunktionen generisch kapselt und zur Verfügung stellt, was man an den Funktionssignaturen sehen kann(void *data, uint64__ length).
Dann bat mich Bebu um eine Funktion die sein Filestream an nimmt und jedem FileInfo Objekt seinen hash übergibt, aber nicht als Klasse sondern als Zahl. Damit wird die ganze Klasse egt unsinnig, da erst eine Instanz der Klasse erstellt werden muss um dann in einer egt Statischen Funktion die Dateien zu hashen.
Hä? Das war jetzt ein Satz mit vielen Buzzwords. Eigentlich ist eine Hashfunktion, wie der Name schon sagt eine "Funktion". Und zwar ein Musterbeispiel davon. Sie kriegt daten als Eingang und spuckt andere Daten als ausgang aus. Warum man da überhaupt ne Klasse braucht versteh ich nicht so ganz. Ok, vielleicht könnte man noch intern ein Struct mit den Zustandsvariablen anlegen, fals es welche gibt, aber eigentlich ist es eben eine funktion.
Wenn du unbedingt eine Klasse machen willst, kannst du die Berechnung in den Konstruktor legen und einen cast-operator bereitstellen, der den integerwert zurückgibt:

Code: Alles auswählen

template<typename HashType>
struct Hash
{
    Hash(void* data, size_t length)
        : _hash(calculateHash(data, length))
    { }

    static HashType calculateHash(void* data, size_t length)
    {
         // dein Zeugs 
    }

    HashType operator HashType () const
    { return _hash; }
private:
    HashType _hash;
};
Ich könnte das Komplette Design über den Haufen werfen und eine 30 Zeilen Implementation der "HashMultiplyFileInfo"-Funktion bauen, die das dann einfach macht.
Wenns dadurch einfacher wird, wieso nicht?
Und dann hat man mich gebeten, einen Teil der Funktionalität, die egt nicht in der Hashklasse liegen sollte, eben dort zu Implementieren: Das Abarbeiten eines FileInfo-Streams.
Warum sollte das nicht in der Hashklasse liegen? Und bevor du antwortest: das ist eine Frage die du mit Bebu klären solltest, nicht mit mir.
Nun muss ich in einer "generischen" in einer egt "statischen" Memberfunktion mich mit dem Laden und Auswerten von Daten herumschlagen und das ist meiner Meinung nach schwachsinnig, auch weil bebu das Interface zwischenzeitlich wieder geändert hat...
Wir sind noch am Anfang des Projektes. Es kann gar nicht sein, dass in diesem Stadium die Interfaces schon feststehen. Ich halte es generell für eine schlechte Idee sich auf best. Interfaces zu versteifen. Ist schon klar, eine gewisse Konstanz muss gegeben sein. Hier ist Kommunikation das Schlüsselwort.

Dann zu den 100MB an sich: Jeder etwas moderne Rechner hat genug RAM(1GB und mehr) und verkraftet sowas. Die Meiste Zeit ist der Ram sowieso halb lehr, also kann es nicht das Problem sein. Zum anderen kann ich aber keine 4GB Dateien auf einmal in den Ram laden, wie vorher geschehen. Deswegen habe ich den Kompromiss gewählt: Egal wie groß die Datei ist, ich fordere 100MB an, lade bis zur Dateigröße oder 100MB, hashe sie und lade die nächsten 100MN, bzw bis zum Dateiende. So muss ich nicht auf ganz so viel Rücksicht nehmen...(faulheit ^^)
Ich würde etwas aufpassen mit "Jeder Rechner". Was deine 100 MB sind (oder genauer gesagt die 104857600 bytes, was übrigens eine Konstante sein solte) ist nichts anderes als eine "Magic Number", eine Zahl die du Annimst und nicht dazuschreibst wieso du sie so hoch annimmst. Temporär ist das in Ordnung, aber man sollte das dann auch in Kommentar und variablenname dokumentieren, und diese Zahl dann irgendwann durch eine etwas intelligenter gewählte ersetzen.

Ich muss es weder Lesen noch beantworten, das ich es aber mache, zeigt doch das ich nicht ganz so Ignorant bin^^
Kritik zwingt zum nachdenken und das hat bisher immer geholfen :)
Sehr gut, so gehört sich das :-)
Wenn des aber so weiter geht, sollten wir das ganze auf anderen Kommunikationswegen weiterführen.
Mit deinem Einverständnis würde ich diese Diskussion gerne in einen eigenen Thread im Dedupe unterforum kopieren und dort weiterdiskutieren. Ist dir das recht?
Haters gonna hate, potatoes gonna potate.

Benutzeravatar
fat-lobyte
Beiträge: 1398
Registriert: Sa Jul 05, 2008 12:23 pm
Wohnort: ::1
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von fat-lobyte » Mo Jul 18, 2011 1:17 pm

cloidnerux hat geschrieben:
Ist dir das recht?
Ja, es geht hier ja auch um generelle Sachen wie der Umgang mit dem SVN, den interfaces und der Kontinuität des Trunks.
Daher wäre es Warscheinlich auch für die anderen Interessant.
Haters gonna hate, potatoes gonna potate.

Benutzeravatar
cloidnerux
Moderator
Beiträge: 3123
Registriert: Fr Sep 26, 2008 4:37 pm
Wohnort: Ram (Gibts wirklich)

Re: Hashfunktion - Feedback

Beitrag von cloidnerux » Mo Jul 18, 2011 1:36 pm

Und den build hast bis jetzt nur du kaputtgemacht.
mhmm, niemand ist perfekt, das hätte aber nicht sein müssen, auch weil es teilweise ziemlich dämliche Fehler waren...
Zuerst testen, testen, testen und DANN erst einchecken
Jetzt habe ich eine Compilierbare Version von Filesearch.
Was ich aber noch nicht kann, ist das ganze Projekt zu Konfigurieren und zu Generieren mit Cmake weil mir sqlite Header und Librarys habe und auch keine ncourses.

Der Cmakelist.txt des hashes fehlt ein cmake_minimum_version_requiered.(oder so...)
Nimms nicht persönlich, aber dein Argument ist jetzt "ich weiß eh wies funktioniert". Bin ich nicht so der fan davon.
Klingt sinnvoll.
Warum man da überhaupt ne Klasse braucht versteh ich nicht so ganz.
Als das mit der hashfunktion aufkam, gab es im Grunde nichts anderes, konnte also auch keine erweiterte Funktionalität Implementieren.
Daher hab ich das ganze so gestaltet, dass man um einen Hash zu erzeugen eine Instanz der Klasse anlegt, dem Objekt die Daten gibt, später evt zusätzliche Daten gibt und dann 2 Hashobjekte vergleicht.
Das wurde allesamt "verworfen" und die Klasse im Grunde auf 1 Funktion reduziert, die einen Filestream bekommt, die Daten lädt, die Daten hasht und als Zahl wieder in das Korrespondierende FileInfo Objekt schreibt.
Damit wurde die Klasse quasi nutzlos.

MfG cloidnerux
Bemerkung am Rande: fileinfo.cpp zeile 28: Das "==" sollte vlt gegen ein "=" ausgetauscht werden. Ebenso in zeile 39
Redundanz macht wiederholen unnötig.
quod erat expectandum

Benutzeravatar
Xin
nur zu Besuch hier
Beiträge: 8858
Registriert: Fr Jul 04, 2008 11:10 pm
Wohnort: /home/xin
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von Xin » Mo Jul 18, 2011 1:43 pm

Ganz kurz nur, soviel Text schaffe ich nicht am Stück auf der Arbeit.

Ich habe gestern kurz die hash32/hash64.cpp gesehen. Ich denke, dass wir im Bezug auf Coderedundanz hier eine Lösung finden müssen und da springen mir ebenfalls Templates ins Gesicht. Falls cloidnerux mit Templates noch nicht vertraut ist, finde ich die gewählte Lösung in Ordnung, aber eben verbesserungsfähig und das Projekt ist ja dazu da, um dazuzulernen.

Mir ist auch aufgefallen, dass der Code sehr eng geschrieben ist. Code sollte so strukturiert werden, dass man den Ablauf erahnen kann. Bei der Strukturierung helfen auch Leerzeilen, die mehr oder weniger unabhängige Abschnitte von einander optisch trennen.

Und auch bei der Magic-Number habe ich meine Schwierigkeit. Es ist in Ordnung, übergangsweise eine Test-Funktion mit Magic-Numbers auszustatten und ich akzeptiere auch, dass hier willkürlich 100MB genommen werden. Diese Willkür ist allerdings kopiert worden, das bedeutet, dass sich in diesem Bereich Willkür ausbreitet. Testfunktionalität, die ausreichend mit suchbaren Schlüsselwörtern wie "FIXME", "TODO", "WARNING"... kommentiert sind, sind übergangsweise in Ordnung. Sie unkommentiert in mehrere Dateien zu verteilen finde ich auch nicht so gut.
cloidnerux hat geschrieben:
Das ist fair, aber in einem Projekt solltest du Rücksicht auf deine Mitarbeiter nehmen. Nachdem ich keinen Code beisteuere hab ich kein Mitspracherecht, aber frag mal Bebu und Xin was die davon halten.
Das ist wieder ein Typisches Problem der Gruppenarbeit. Du kannst egt nicht gleichzeitig mit mehreren an derselben Sache Arbeiten, da stehen sie sich alle nur auf den Füßen, wie hier geschehen.
Es müssen nicht mehrere Leute gleichzeitig an einer Sache arbeiten. Das geht, aber das muss organisiert sein.

Es muss aber möglich sein, dass mehrere Leute zu unterschiedlichen Zeiten an einer Sache arbeiten können.
Jeder muss aber in der Lage sein, den vorhandenen Code zu überarbeiten, ohne ihn neu zu schreiben zu müssen. Daher muss jeder Code so gehalten sein, dass ein anderer ihn modifizieren kann.


Da diese Unterhaltung hier ausschließlich über cloidnerux' Code geht, möchte ich am Rande auch bemerken, dass gestern auch Bebus Code von mir als "Prosa" bezeichnet wurde und Bebu und Fat-Lobyte eins meiner Interfaces nicht gefallen hat.
Keiner schreibt perfekten Code. Wir alle arbeiten dran.

Wer keinen Code eincheckt, geht auch nicht das Risiko ein, dass jemand anderem der Code nicht gefällt und zur Diskussion stellt. Das geht jetzt nicht gegen fat-lobyte, es sagt nur aus, dass cloidnerux überhaupt Code zur Verfügung stellt, über den man reden kann und damit Projekt wie auch die Diskussion darüber eher weiterbringt, als wenn er gar nichts machen würde. Mir ist wichtig, dass hier eine Leistung vorliegt, die optimiert werden kann und mehr wert ist, als eine nicht vorhandene Leistung, über die hier entsprechend nicht gesprochen werden kann. Das Rampenlicht, in das cloidnerux durch diesen Thread gestellt wird, sollte also auch die gemachte Leistung nicht unter den Scheffel stellen.
Merke: Wer Ordnung hellt ist nicht zwangsläufig eine Leuchte.

Ich beantworte keine generellen Programmierfragen per PN oder Mail. Dafür ist das Forum da.

Benutzeravatar
fat-lobyte
Beiträge: 1398
Registriert: Sa Jul 05, 2008 12:23 pm
Wohnort: ::1
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von fat-lobyte » Mo Jul 18, 2011 2:01 pm

cloidnerux hat geschrieben:
Und den build hast bis jetzt nur du kaputtgemacht.
mhmm, niemand ist perfekt, das hätte aber nicht sein müssen, auch weil es teilweise ziemlich dämliche Fehler waren...
Macht nix, da es erledigt ist, ist ja nicht weiter schlimm. Wenns ab jetzt funktioniert ist alles in Butter :-)

Was ich aber noch nicht kann, ist das ganze Projekt zu Konfigurieren und zu Generieren mit Cmake weil mir sqlite Header und Librarys habe und auch keine ncourses.
Könntest du bitte deine Probleme in den Build-Thread posten? So richtig mit deinen Befehlszeilen und Ausgabe von CMake und make? Genau darum wollte ich mich nämlich kümmern. Link: http://www.proggen.org/forum/viewtopic.php?f=66&t=2401
Der Cmakelist.txt des hashes fehlt ein cmake_minimum_version_requiered.(oder so...)
Das soll nur einmal pro projekt aufgerufen werden, und wird meines wissens schon im Top-Level CMakeLists.txt. Wenn nicht, dann werde ich das heut abend fixen.
Als das mit der hashfunktion aufkam, gab es im Grunde nichts anderes, konnte also auch keine erweiterte Funktionalität Implementieren.
Daher hab ich das ganze so gestaltet, dass man um einen Hash zu erzeugen eine Instanz der Klasse anlegt, dem Objekt die Daten gibt, später evt zusätzliche Daten gibt und dann 2 Hashobjekte vergleicht.
Das wurde allesamt "verworfen" und die Klasse im Grunde auf 1 Funktion reduziert, die einen Filestream bekommt, die Daten lädt, die Daten hasht und als Zahl wieder in das Korrespondierende FileInfo Objekt schreibt.
Damit wurde die Klasse quasi nutzlos.
Alles klar.

Xin hat geschrieben:... dass cloidnerux überhaupt Code zur Verfügung stellt, über den man reden kann und damit Projekt wie auch die Diskussion darüber eher weiterbringt, als wenn er gar nichts machen würde. Mir ist wichtig, dass hier eine Leistung vorliegt, die optimiert werden kann und mehr wert ist, als eine nicht vorhandene Leistung, über die hier entsprechend nicht gesprochen werden kann. Das Rampenlicht, in das cloidnerux durch diesen Thread gestellt wird, sollte also auch die gemachte Leistung nicht unter den Scheffel stellen.
Ich hoffe dass ich hier nicht falsch verstanden werde. Mit meinem "feedback" hatte ich gehofft ein bisschen beizutragen. Wenn ich deine Arbeit für Müll gehalten hätte, hätte ich einfach gar nichts gemacht und kein Wort geschrieben. Sieh mein "Feedback" als Zeichen des Respekts :-)
Und auch wenns nicht so rübergekommen ist, auch ich bin froh dass etwas da ist. Auch wenns verbesserungswürdig ist, aber das ist es bei allen, ohne Ausnahme. Aber du hast etwas geschrieben und das find ich gut.
Haters gonna hate, potatoes gonna potate.

Benutzeravatar
Xin
nur zu Besuch hier
Beiträge: 8858
Registriert: Fr Jul 04, 2008 11:10 pm
Wohnort: /home/xin
Kontaktdaten:

Re: Hashfunktion - Feedback

Beitrag von Xin » Mo Jul 18, 2011 6:31 pm

fat-lobyte hat geschrieben:
Xin hat geschrieben:... dass cloidnerux überhaupt Code zur Verfügung stellt, über den man reden kann und damit Projekt wie auch die Diskussion darüber eher weiterbringt, als wenn er gar nichts machen würde. Mir ist wichtig, dass hier eine Leistung vorliegt, die optimiert werden kann und mehr wert ist, als eine nicht vorhandene Leistung, über die hier entsprechend nicht gesprochen werden kann. Das Rampenlicht, in das cloidnerux durch diesen Thread gestellt wird, sollte also auch die gemachte Leistung nicht unter den Scheffel stellen.
Ich hoffe dass ich hier nicht falsch verstanden werde. Mit meinem "feedback" hatte ich gehofft ein bisschen beizutragen. Wenn ich deine Arbeit für Müll gehalten hätte, hätte ich einfach gar nichts gemacht und kein Wort geschrieben. Sieh mein "Feedback" als Zeichen des Respekts :-)
Und auch wenns nicht so rübergekommen ist, auch ich bin froh dass etwas da ist. Auch wenns verbesserungswürdig ist, aber das ist es bei allen, ohne Ausnahme. Aber du hast etwas geschrieben und das find ich gut.
Ihr habt euch schon besprochen, offenbar ohne großen Streß per PN. Jetzt geht das ganze in eine öffentliche Runde und da müssen halt für alle Seiten kurz ein paar Sachen klargestellt werden. Zum einen, dass hier cloidnerux nicht an den Pranger gestellt wird und auch sonst niemand, der vielleicht mal irgendwo Code beiträgt.
Merke: Wer Ordnung hellt ist nicht zwangsläufig eine Leuchte.

Ich beantworte keine generellen Programmierfragen per PN oder Mail. Dafür ist das Forum da.

Antworten