2015-03-13 5 views
14

sind const Ich habe ein Klasse-Design ähnlich den folgenden:Mitgliedfunktionen, die manchmal

class MyClass { 
public: 
    bool IsValid() const; 
    void MakeValid(); 
private: 
    bool CheckValidity(bool fix); 
}; 

bool MyClass::IsValid() const { 
    // Check validity, but don't fix any problems found. Doesn't work. 
    return CheckValidity(false); 
} 

void MyClass::MakeValid() { 
    // Check validity and fix problems found. 
    CheckValidity(true); 
} 

IsValid sollte const sein, da es keine Änderungen vornehmen. MakeValid sollte nicht-const sein, weil es Änderungen vornimmt. Sie teilen sich die gleiche Implementierung, CheckValidity, aber weil CheckValidity kann oder nicht Änderungen vornehmen, kann es nicht const markiert werden.

Was ist der beste Weg, damit umzugehen? Der einfachste Ansatz ist, einfach const_cast zu verwenden, aber Wegwerfen const fühlt sich ein bisschen schmutzig:

bool MyClass::IsValid() const { 
    // Check validity, but don't fix any problems found. 
    return const_cast<MyClass*>(this)->CheckValidity(false); 
} 

Ist dies eine legitime Verwendung von const_cast? Gibt es einen besseren Ansatz?

+14

Split-Checks und "Fixieren" in zwei verschiedene Funktionen? Checks ist 'const' und fix nicht? – crashmstr

+6

Ich stimme @crashmstr, mit einer Funktion, die zwei verschiedene Dinge ist ein schlechter Design-Geruch. –

+5

Eigentlich sagt schon der Name 'CheckValidity', dass diese Funktion nur eine Prüfung durchführt und const sein kann. Wenn Sie 'MakeValid' wollen, ist es etwas anderes (und es sollte nicht const sein) – user463035818

Antwort

16

ich Ihre Implementierung gehe davon aus sieht ähnlich wie:

bool CheckValidity(bool fix) 
{ 
    // Actually check validity. 
    bool isValid = ...; 

    if (!isValid && fix) 
    { 
     // Attempt to fix validity (and update isValid). 
     isValid = ...; 
    } 

    return isValid; 
} 

Sie haben wirklich zwei verschiedene Funktionen in einem geschoben. Einer der Schlüsselindikatoren für diese Art der Verschränkung ist das boolesche Argument für die Funktion ..., das riecht, weil der Aufrufer nicht sofort erkennen kann, ob er wahr oder falsch setzt, ohne Code/Dokumente zu referenzieren.

die Methode Aufgeteilt:

bool CheckValidity() const 
{ 
    // Actually check validity. 
    bool isValid = ...; 
    return isValid; 
} 

void FixValidity() 
{ 
    // Attempt to fix validity. 
    // ... 
} 

Und dann Ihre öffentlichen Methoden, die Anrufe entsprechend mehr machen kann.

bool IsValid() const 
{ 
    // No problem: const method calling const method 
    return CheckValidity(); 
} 

void MakeValid() 
{ 
    if (!CheckValidity()) // No problem: non-const calling const 
    { 
     FixValidity(); // No problem: non-const calling non-const 
    } 
} 
+1

In diesem Fall ist die Prüflogik kompliziert (geschachtelte Unterstrukturen durchlaufen und auf Konsistenz prüfen), und zur Behebung von Problemen muss man wissen, welche Teilelemente Probleme haben, also habe ich Mühe, einen sauberen Weg zu finden "CheckValidity" und "FixValidity" trennen, ohne die Logik zu duplizieren oder zu verkomplizieren. –

+5

@JoshKelley Es kann sein, dass Sie traversal von Prüfung von Manipulation trennen können. Oder Sie können feststellen, dass Check vs Fix schwierig zu trennen ist, weil Ihr vorhandener Code/Ihre vorhandenen Strukturen vollständig sind. Eine "einfache" Möglichkeit, in Check und Fix zu teilen, besteht einfach darin, Ihre aktuelle CheckValidity (bool) -Methode zu duplizieren. Mit der ersten Annahme, dass bool in dieser Methode überall falsch ist, entferne toten Code und entferne dann den bool-Parameter. Benennen Sie im zweiten Fall FixValidity um, nehmen Sie an, dass bool true ist, entfernen Sie den toten Code und entfernen Sie den bool-Parameter. Dann sehen Sie, was immer noch zwischen den beiden und Refactor gemeinsam ist. –

+0

Weiter darüber nachdenken, scheint die Spaltung der Traversale der richtige Weg zu sein. Vielen Dank. –

6

Hier ist ein Ansatz, der in einigen Fällen nützlich sein könnte. Es könnte für Ihre spezielle Situation übertrieben sein.

Ihre CheckValidity-Funktion könnte ein Handler-Objekt übergeben werden. Die CheckValidity-Funktion würde herausfinden, was nicht gültig ist, und eine geeignete Methode des Handler-Objekts aufrufen. Sie könnten viele verschiedene Methoden für verschiedene Arten von Gültigkeitsverletzungen haben, und diesen Methoden könnten genügend Informationen übergeben werden, damit das Problem bei Bedarf behoben werden kann. Um IsValid zu implementieren, müssen Sie nur einen Handler übergeben, der ein Flag setzt, das anzeigt, dass ein Problem aufgetreten ist. Um MakeValid zu implementieren, können Sie einen Handler übergeben, der das Problem tatsächlich behebt. Das const-Problem wird behoben, indem der Fixierungshandler eine nicht konstante Referenz auf das Objekt behält. Hier

ein Beispiel:

class MyClass { 
public: 
    bool IsValid() const 
    { 
     bool flag = false; 
     CheckValidity(FlagProblems{flag}); 
     return flag; 
    } 

    void MakeValid() 
    { 
     CheckValidity(FixProblems{*this}); 
    } 

private: 
    struct FlagProblems { 
     bool& flag; 

     void handleType1(arg1,arg2)  const { flag = true; } 
     void handleType2(arg1,arg2,arg3) const { flag = true; } 
     . 
     . 
     . 
    }; 

    struct FixProblems { 
     MyClass& object; 
     void handleType1(arg1,arg2)  const { ... } 
     void handleType2(arg1,arg2,arg3) const { ... } 
     . 
     . 
     . 
    }; 

    template <typename Handler> 
    bool CheckValidity(const Handler &handler) const 
    { 
     // for each possible problem: 
     // if it is a type-1 problem: 
     //  handler.handleType1(arg1,arg2); 
     // if it is a type-2 problem: 
     //  handler.handleType2(arg1,arg2,arg3); 
     // . 
     // . 
     // . 
    } 
}; 

unter Verwendung der Schablone für maximale Effizienz ermöglicht. Alternativ bietet die Verwendung einer Basisklasse mit virtuellen Funktionen für den Handler möglicherweise eine kleinere ausführbare Größe.

Wenn die Art und Weise, in der das Objekt ungültig sein kann, einfacher ist, dann kann die CheckValidity-Rückgabe eine Struktur, die die relevanten Informationen enthält, einfacher sein.

+0

Definitiv eine gültige Möglichkeit, obwohl 'CheckValidity' mit dieser Art von Trennung geeignetererweise' 'Traverse' 'oder etwas Ähnliches genannt wird, das anzeigt, dass es sich durch die Daten bewegt und Handler anwendet, anstatt selbst zu überprüfen. –

+1

@MatthewMoss: Ich meinte wirklich, dass wir jedes mögliche Problem durchgehen, und nicht jedes mögliche Objekt in der Datenstruktur. Ich werde es umschreiben. –

+0

Ah, verstanden. –

2

Sie können eine Vorlagenspezialisierung verwenden, um die Teile zu trennen, die nur einen Zweck für ein nicht-konstantes Objekt haben.

Folgendes ist eine Implementierung für eine Spielzeugklasse. Es hat ein einzelnes C-Array-Member v mit 10 Ints, und für unsere Zwecke ist es nur gültig, wenn jeder einzelne von ihnen gleich Null ist.

class ten_zeroes { 
    int v[10]; 
    void fix(int pos) {v[pos] = 0;} 

    public: 
    ten_zeroes() { // construct as invalid object 
    for (int i=0;i<10;i++) { 
     v[i] = i; 
    } 
    } 
}; 

Sehen Sie, dass ich bereits ein Funktionselement hergestellt, das eine ungültige Position fixiert, und einen schönen Konstruktor, der es als ein ungültiges Objekt initialisiert (tu das nicht: D) ​​

Da wir gehen Verwenden Sie Templates, wir müssen die Implementierung des Check/Fix-Zyklus außerhalb der Klasse verschieben. Damit die relevanten Funktionen auf v und die fix() Methode zugreifen können, machen wir ihnen Freunde. Unser Code sieht jetzt aus wie:

class ten_zeroes { 
    int v[10]; 
    void fix(int pos) {v[pos] = 0;} 

    public: 
    ten_zeroes() { // construct as invalid object 
    for (int i=0;i<10;i++) { 
     v[i] = i; 
    } 
    } 

    template<typename T> 
    friend void fix(T& obj, int pos); 

    template<typename T> 
    friend bool check(T& obj); 
}; 

check() ‚s Implementierung ist einfach:

// Check and maybe fix object 
template<typename T> 
bool check(T& obj){ 
    bool result = true; 
    for(int i=0;i<10;i++) { 
    if (obj.v[i]) { 
     result = false; 
     fix(obj, i); 
    } 
    } 
    return result; 
} 

Hier ist der schwierige Teil. Wir möchten, dass unsere fix()-Funktion das Verhalten basierend auf der Konstanz ändert. Dafür müssen wir die Vorlage spezialisieren. Bei einem nichtkonstanten Objekt wird die Position fixiert. Für eine const ein, wird es nichts tun:

// For a regular object, fix the position 
template<typename T> 
void fix(T& obj, int pos) { obj.fix(pos);} 

// For a const object, do nothing 
template<typename T> 
void fix(const T& obj, int pos) {} 

Schließlich schreiben wir unsere is_valid() und make_valid() Methoden, und hier haben wir die vollständige Umsetzung:

#include <iostream> 

class ten_zeroes { 
    int v[10]; 
    void fix(int pos) {v[pos] = 0;} 

    public: 
    ten_zeroes() { // construct as invalid object 
    for (int i=0;i<10;i++) { 
     v[i] = i; 
    } 
    } 

    bool is_valid() const {return check(*this);} // since this is const, it will run check with a const ten_zeroes object 
    void make_valid() { check(*this);} // since this is non-const , it run check with a non-const ten_zeroes object 

    template<typename T> 
    friend void fix(T& obj, int pos); 

    template<typename T> 
    friend bool check(T& obj); 
}; 

// For a regular object, fix the position 
template<typename T> 
void fix(T& obj, int pos) { obj.fix(pos);} 

// For a const object, do nothing 
template<typename T> 
void fix(const T& obj, int pos) {} 

// Check and maybe fix object 
template<typename T> 
bool check(T& obj){ 
    bool result = true; 
    for(int i=0;i<10;i++) { 
    if (obj.v[i]) { 
     result = false; 
     fix(obj, i); 
    } 
    } 
    return result; 
} 

int main(){ 
    ten_zeroes a; 
    std::cout << a.is_valid() << a.is_valid(); // twice to make sure the first one didn't make any changes 
    a.make_valid(); // fix the object 
    std::cout << a.is_valid() << std::endl; // check again 
} 

Ich hoffe nicht, den Geist main() Funktion dort. Es wird unser kleines Spielzeug testen und 001 ausgeben, wie erwartet. Jede Wartung dieses Codes wird sich nicht mehr mit der Duplizierung von Code befassen müssen, was Sie wahrscheinlich vermeiden wollten. Ich hoffe, das war hilfreich.

Wenn Sie diese Implementierungsdetails vor dem endgültigen Benutzer verbergen möchten, sollten Sie sie natürlich in einen geeigneten Detail-Namespace verschieben. Ich überlasse das Ihnen :)

Verwandte Themen