3. [Code] FileExplorer - Verbesserungsvorschläge?

[Code] FileExplorer - Verbesserungsvorschläge?

  • V

    SideWinder schrieb:

    Was meint ihr, reicht das bereits - findet ihr grobe Fehler? Designverbesserungsvorschläge? Sonstiges?

    bitte male vorher ein uml-diagramm, damit ich deine schnittstellen erkenne und so.
    dann schaue ich mir es gerne an.

    0
  • S

    volkard schrieb:

    SideWinder schrieb:

    Was meint ihr, reicht das bereits - findet ihr grobe Fehler? Designverbesserungsvorschläge? Sonstiges?

    bitte male vorher ein uml-diagramm, damit ich deine schnittstellen erkenne und so.
    dann schaue ich mir es gerne an.

    http://root.sidewindershome.net/cache/directory.htm *g*

    MfG SideWinder

    0
  • V

    Habe für ein Projekt eine Klasse geschrieben mit der ein Schulkollege in seinem
    Projekt einfach Dateien in Verzeichnissen auslesen kann und auf bestimmte Dateien
    Konsolenbefehle ausführen kann.
    und dafür so viel code? uih!

    > // Eine Zeit (nicht initialisiert!) die als Standard verwendet wird für die File-Klasse.

    extern const FILETIME NO_TIME;
    Warum kann eine nicht initialisierte Zeit überhaupt existieren???

    > // Ein File-Objekt welches als Standardwert für "Keine Datei gefunden" verwendet wird.

    extern const File NO_FILE;
    dito!

    > // Eine Liste (genauer ein Vektor) von Dateien.

    typedef std::vector<File> FileList;
    kann der typedef lokal sein?

    const bool operator== (const File& rLeft, const File& rRight);
    UN ist out.

    const bool operator!= (const File& rLeft, const File& rRight);
    denke an
    inline const bool operator!= (const File& rLeft, const File& rRight)
    {
    return !(rleft==rRight):
    }

    schreibste variablen eigentlich gross oder klein? ich sehe beides. wenn du
    klassen konsequent gross schreibst, sollten variablen nicht gerade gross sein.

    const bool operator== (const DiskExplorer& rLeft, const DiskExplorer& rRight);
    durcheinander. schreib das doch direkt hinter die klasse DiskExplorer

    > File (const std::string& Name,
    > const std::string& Path,
    > const __int64 Size,
    > const DWORD Attributes,
    > const FILETIME CreationTime,
    > const FILETIME LastAccessTime,
    > const FILETIME LastAccessWithChangeTime);
    Ist schlimm. Wer wird wann und warum diesen CTor benutzen?

    __int64 muss weggetypedefed werden! bau die nen typedef __int64 i64, dann kannste
    den für jede maschine anpassen ohne ärger. hier nicht wichtig, aber sobald du funktionen
    baust, die mit i64 arbeiten und nicht nur mit files zu tun haben.

    > // Füllt das Objekt mit Werten die aus der übergebenen WIN32_FIND_DATA-Struktur ausgelesen werden.
    > File (const WIN32_FIND_DATA& rData);
    ok, aber sollte der so öffentlich sein?

    > const int docmd (const std::string& rCommand, const std::string& rCommandPart2 = "") const;
    und wieder UN? diesmal r vor ner ref auf const. aber die ref auf const soll sich so anfühlen, als
    sei der parameter by value übergeben. ich finde, dann sollte er sich auch so anhören.

    ah, evtl sollte File lokal in ner anderen klasse stecken.

    > DiskExplorer (const char rPartition = 'C');
    ok, der gefällt mir mal.
    aber warum const??? nee, dafür hab ich kein verständnis.

    > // Erstellt ein DiskExplorer-Objekt mit den gleichen Daten wie das übergebene
    > // Objekt. Bereits laufende Suchen werden nicht übernommen.
    > DiskExplorer (const DiskExplorer& rToCopy);
    aye, aye, sir. ist konsistent mit dem op==, aber wird beine brechen.

    > // Standard-Destruktur
    > ~DiskExplorer ();
    kommentar schlimm. es kann nur einen geben. daher bringste dem freunde mit "standard"-dtor
    käse bei.

    > DiskExplorer& operator= (const DiskExplorer& rToAssign);
    den op= immer direkt zum copy-ctor schreiben! gleiches zu gleichem!

    > friend const bool operator== (const DiskExplorer& rLeft, const DiskExplorer& rRight);
    > friend const bool operator!= (const DiskExplorer& rLeft, const DiskExplorer& rRight);
    oh, wenn du die hier deklarierst, können die unaufgeröumten deklarationen da oben ja weg. fein.

    > const bool up ();
    sicher, daß da ein const sinnvoll ist? das ist ja die reinste seuche.

    besser ist immer, bool isroot() nebst void up(). der user kann besser arbeiten, wenn er
    vorher testen kann, ob es klappen wird, und nicht immer nach dem arbeiten testen muss.

    > const File getfirstfile (const std::string& rPattern = ".") const;
    > const File getnextfile () const;
    das stimmt doch ales nicht! wie kann denn dein diskexplorer ne "Suche" haben? das ist ja,
    als wenn mein vector nen iterator hätte! nee, Suche bzw iterator muss ne extra-klasse sein.

    > const FileList getfilelist (const std::string& rPattern = ".") const;
    dagegen spricht wieder nix.

    > const int docmd (const std::string& rCommand, const std::string& rPattern = ".") const;
    sicher, daß docmd nicht besser methode von FileList wäre?
    also ein DiskExplorer('c').getFileList().doCmd("notepad");
    finde ich logischer (ohne handbuch sofortiger verständlich), als ein
    DiskExplorer('c').doCmd("notepad");

    > mutable HANDLE m_SearchHandle;
    jo, das mutable ist die gerechte strafe dafür, daß der iterator keine eigene klasse ist.

    const int File::docmd (const std::string& rCommand, const std::string& rCommandPart2) const
    {
    > std::string Command;
    > if(m_Path[m_Path.length()-1] != '\')
    > {
    > Command = rCommand + " " + m_Path + "\" + m_Name + " " + rCommandPart2;
    > }
    > else
    > {
    > Command = rCommand + " " + m_Path + m_Name + " " + rCommandPart2;
    > }
    > return(system(Command.c_str()));
    }
    auch gerechte strafe. der user kann damit leben, wenn pfade IMMER mit nem \ aufhören müssen.

    und überhaupt:
    statt
    docmd("copy","d:")
    lieber
    docmd("copy ! d:")
    dann kann der user auch locker mal schreiben
    docmd("copy ! d: && copy ! e:")

    DiskExplorer::DiskExplorer (const char rPartition)
    > :
    m_CurPath (""),
    m_Root (static_caststd::string(&rPartition)+":"),
    m_SearchHandle (0)
    {
    }
    static_cast? bist du ganz sicher? ich nicht.

    DiskExplorer::~DiskExplorer ()
    {
    }
    süß

    > return(*this);
    diese klammern sagen: ich habe zu viele c-bücher gelesen und dabei ganz
    vergessen, daß return kein funktionsaufruf wie exit() ist.

    void DiskExplorer::backtoroot ()
    {
    > m_CurPath = "";
    > FindClose(m_SearchHandle);
    > m_SearchHandle = 0;
    }
    jo, das FindClose steckt eigentlich im Dtor des iterators.

    const bool DiskExplorer::up ()
    {
    > if(m_CurPath != "")
    > {
    > if(m_CurPath.find('\') != std::string::npos)
    > {
    > m_CurPath.erase(m_CurPath.begin()+m_CurPath.find_last_of('\'),m_CurPath.end());
    > }
    > else
    > {
    > m_CurPath = "";
    > }

    > return(true);
    > }
    > else
    > {
    > return(false);
    > }
    ohoh! so viel code ist eindeutig ein fehler. bestimmt ist der entwurf schlecht.
    male doch mal ein uml-diagramm, um klarheit zu bekommen.

    const bool DiskExplorer::down (const std::string& rNewPath)
    {
    > std::string RealNewPath;
    > for(unsigned long int i = 0; i < rNewPath.length(); ++i)
    > {
    > if( rNewPath[i] != '/' &&
    > rNewPath[i] != ':' &&
    > rNewPath[i] != '*' &&
    > rNewPath[i] != '?' &&
    > rNewPath[i] != '"' &&
    > rNewPath[i] != '<' &&
    > rNewPath[i] != '>' )
    > {
    > RealNewPath += rNewPath[i];
    > }
    > }

    > while(RealNewPath[0] == '\')
    > RealNewPath.erase(RealNewPath.begin());

    > while(RealNewPath[RealNewPath.length()-1] == '\')
    > RealNewPath.erase(RealNewPath.end());

    > std::vectorstd::string Directories;

    > while(RealNewPath.find('\') != std::string::npos)
    > {
    > Directories.push_back(RealNewPath.substr(0,RealNewPath.find_first_of('\')));
    > RealNewPath.erase(0,RealNewPath.find_first_of('\')+1);
    > }

    > Directories.push_back(RealNewPath);

    > FindClose(m_SearchHandle);
    > m_SearchHandle = 0;

    > std::vectorstd::string::iterator Runner = Directories.begin();
    > std::string PathToDir = m_Root + m_CurPath;
    > while(Runner != Directories.end())
    > {
    > WIN32_FIND_DATA FindData;
    > if(INVALID_HANDLE_VALUE != (m_SearchHandle = FindFirstFile((PathToDir+"\"+*Runner).c_str(),&FindData)) && FindData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
    > {
    > FindClose(m_SearchHandle);
    > m_SearchHandle = 0;

    > PathToDir += '\' + *Runner;
    > }
    > else
    > {
    > return(false);
    > }

    > ++Runner;
    > }

    > m_CurPath += PathToDir.substr(2);
    > return(true);

    }
    das ist ein scherz, ich weiß. sonst machste keine funktion über 6 zeilen lang.
    wolltest mich nur mal testen. aber ich hab's bemerkt.

    const int DiskExplorer::docmd (const std::string& rCommand, const std::string& rPattern) const
    {
    > std::string Command;

    > if(m_CurPath != "")
    > {
    > Command = rCommand + " " + getpath() + "\" + rPattern;
    > }
    > else
    > {
    > Command = rCommand + " " + getpath() + rPattern;
    > }

    > return(system(Command.c_str()));

    }
    ich hab aber jetzt ein deschawü-erlebnis. ist das fair, mir sowas anzutun?

    const bool operator== (const File& rLeft, const File& rRight)
    {
    > if(rLeft.m_Attributes == rRight.m_Attributes && rLeft.m_Name == rRight.m_Name && rLeft.m_Path == rRight.m_Path)
    > return(true);
    > return(false);
    }
    süß, um meinen unregistrierten vorredner zu bestätigen.

    const bool operator!= (const File& rLeft, const File& rRight)
    {
    > if(rLeft.m_Attributes != rRight.m_Attributes || rLeft.m_Name != rRight.m_Name || rLeft.m_Path != rRight.m_Path)
    > return(true);
    > return(false);
    }
    ich hab schon wieder so ein erlebnis. da ist doch irgendwo dopelter code im spiel.

    const bool operator== (const DiskExplorer& rLeft, const DiskExplorer& rRight)
    {
    > if(rLeft.m_Root+rLeft.m_CurPath == rRight.m_Root+rRight.m_CurPath)
    > return(true);
    > return(false);
    }
    weißt du, wie teuer ne string-aneinaderhängung ist? und wie verdammt billig ein && ist?

    const bool operator!= (const DiskExplorer& rLeft, const DiskExplorer& rRight)
    {
    > if(rLeft.m_Root+rLeft.m_CurPath != rRight.m_Root+rRight.m_CurPath)
    > return(true);
    > return(false);
    }
    dito (teuer) und dito(deschawü).

    Aber würdet ihr das Interface als okay empfinden?
    Verbesserungsvorschläge?
    versuch mal, nen iterator zu bauen. der macht FindFirstFile im ctor und FindClose
    im Dtor. der entwurf wird aber sehr schwierig, weil die begrifflichkeiten sehr
    unklar sind. zum beispiel hattest du gar keine files, sondern nur file-beschreiber
    in deinem bisherigen ansatz. aber File war der name. das wird noch viel schlimmer.

    du weißt schon, da hilft kein nachdenken und kein ausprobieren. musst ein uml-diagramm
    malen. dann wird alles klar.

    0
  • S

    Ja und dafür soviel Code, er hat mir auch schon einiges zugeschoben, soll er ne ordentliche Implementation erhalten.

    Die Sache mit der eignenen FileList-Klasse die Befehle und Suchen auf sich ausführen kann finde ich interessant!

    Die Namensgebung ist eigentlich klar. Referenzen kriegen ein r davor, Pointer ein p und sonst kriegt niemand was davor. Namen sind groß (egal ob Klasse oder Variablenname).

    Der Rest wird ebenfalls bei Gelegenheit bearbeitet, die const bleiben aber.

    Das was falsch sein muss wenn mutable so 100% anwendbar ist kam mir auch schon früher 🙄

    Gibts eigentlich wirklich sinnvolle Anwendungungsbeispiele für mutable? volatile kann ich mir ja wenigstens noch vorstellen...

    MfG SideWinder

    0
  • V

    SideWinder schrieb:

    die const bleiben aber.

    das glaube ich nicht, tim.

    SideWinder schrieb:

    Gibts eigentlich wirklich sinnvolle Anwendungungsbeispiele für mutable?

    class string{
  mutable int size;
  ...
  int getsize() const{
     if(size==-1) size=calcsize();
     return size;
  }
  void replace(string toreplace,string replacement){
     size=-1;
     ...
  }
    0
  • S

    Was stellst du dir statt NO_FILE vor? Mir fällt da nichts besseres ein als ein Standardobjekt zu definieren, dass == keiner Datei bedeutet.

    Ich kann ja nicht vom User verlangen, dass der jedesmal try/catch macht wenn er einen Befehl ausführen will, also sind Exceptions auch nicht das Wahre.

    Was nun?

    MfG SideWinder

    0
  • V

    SideWinder schrieb:

    Was stellst du dir statt NO_FILE vor? Mir fällt da nichts besseres ein als ein Standardobjekt zu definieren, dass == keiner Datei bedeutet.
    Ich kann ja nicht vom User verlangen, dass der jedesmal try/catch macht wenn er einen Befehl ausführen will, also sind Exceptions auch nicht das Wahre.

    nein, try/catch auf keinen fall.
    aber warum kann der user überhaupt ein objekt NO_FILE in der hand halten?
    weil du den returnwert überladen hast!
    normalerweise würde man zwei sachen zurückgeben, nen bool und ne File.
    kann man vorher fragen, ob noch ein File da ist? wohl kaum, da man sich nicht zu weit von der API entfernen mag.
    manche mögen es, gleich das ganze verzeichnis zu lesen, und dann ginge es.
    aber wenn nachher ein iterator draus wird, muß man, um es stl-ähnlich zu halten, mit blabla.end() ja eh ein ungültiges objekt geben können.
    ok, darfst NO_FILE benutzen. (aber ich hab ein ungutes gefühl dabei.)

    0
  • S

    Muss mal bei Amazon nach Büchern für Klassendesign suchen, finde das sehr interessant und will nicht jedesmal wegen jedem Mist hier nachfragen 🙂

    MfG SideWinder

    0
  • ?

    Der Rest wird ebenfalls bei Gelegenheit bearbeitet, die const bleiben aber.

    Und *warum*?

    0
  • S

    Erstmal ist alles konstant und dann nimmt man die const weg wo man muss damit es auch funktioniert. Dann kann man so wenig wie möglich falsch machen.

    Warum sollten sie also weg? Welchen Grund gibts dafür? "Es sieht nicht so gut aus" ist nicht gerade die Antwort 🙄

    MfG SideWinder

    0
  • ?

    Also ich meinte nur bei den Rückgabewerten.

    Bsp: const bool up()

    Das hat überhaupt keinen Sinn.

    0
  • V

    volkard schrieb:

    ok, darfst NO_FILE benutzen. (aber ich hab ein ungutes gefühl dabei.)

    doch nicht.
    der iterator muß ja ein HANDLE haben und praktischerweise auch ein WIN32_FIND_DATA.
    kannst wenn FindNext FALSE liefert, ruhig gleich FindClose machen und das handle invalidieren. dann kann man am wert des handles erkennen, ob man auf blabla.end() steht. kein bedarf für NO_FILE.

    0
  • V

    SideWinder schrieb:

    Muss mal bei Amazon nach Büchern für Klassendesign suchen, finde das sehr interessant und will nicht jedesmal wegen jedem Mist hier nachfragen 🙂

    wirst pech haben. aber falls du uml magst, das wirste in so büchern finden. 😃

    0
  • S

    Also das Entwurfsmusterbuch und OOP für Dummies finde ich ziemlich gut, es geht
    hier doch auch um das Klassendesign und dafür ist UML doch ausgelegt, und in
    OOP für Dummies lernt man ja auch wie man das nacher in Code umsetzt, wobei man
    hier ja eigentlich _nur_ noch die fertigen Methoden mit Code füllt.

    0
  • V

    SirLant schrieb:

    hier doch auch um das Klassendesign und dafür ist UML doch ausgelegt

    aber unbrauchbar. siehe thread "Informatiklehrer"

    0
  • R

    Toooom schrieb:

    Also ich meinte nur bei den Rückgabewerten.

    Bsp: const bool up()

    Das hat überhaupt keinen Sinn.

    bool foo() { return true; } //<--- ohne const

int main() {
  foo()=false;
}

    > g++ -Wall -W -std=c++98 const_ret.cc
const_ret.cc: In function `int main()':
const_ret.cc:4: error: non-lvalue in assignment

    vola. Das const ist dort so unnötig, wie das auto keyword 🙂

    btw.

    File (const WIN32_FIND_DATA& rData);

    ich würde wenn du schon ein Wrapper schreibst die Datenstrukturen komplett wegwrappen und nicht mit so Win32 Strukturen arbeiten

    *hust*boost::filesystem*hust*

    0
  • S

    Ich kann zwar nicht viel, aber dass mir das const beim Rückgabetyp nichts bringt weiß ich auch. Aber wie gesagt es ist eben mein Stil und drückt nochmals aus, dass der Rückgabewert konstant ist. Falls das irgendjemand nicht weiß und dein tolles Beispiel nicht gesehen hat 😉

    Frage: Was bringt -Wall bei g++?

    Und wie soll ich die WIN32_FIND_DATA komplett wegwrappen? Ich mein irgendwie muss ich ja aus der Datenstruktur die mir FindFirstFile()/FindNextFile() gibt ein FileInfo-Objekt machen - was gibts da besseres als einen Konstruktor?

    MfG SideWinder

    0
  • V

    mein tip: tu es nicht wegwrappen. aber geheimhalten in der klasse kannste es. und den ctor evtl private machen.

    0
Anmelden zum Antworten