Restituisce NULL in caso di successo

4

Questa domanda è una versione più chiara di una domanda Ho pubblicato su SO.

Ho un oggettoPlanner C ++% con un metodo che calcola un Route da un punto iniziale a un punto di destinazione. Planner è il proprietario del puntatore Route calcolato.

Ora, ho un secondo metodo (chiamalo TestRoute ), che prende l'oggetto del percorso e indica se la destinazione è raggiungibile dal punto di partenza, e in caso contrario cerca di trovare un percorso che passa attraverso le stazioni di rifornimento.

L'implementazione originale di TestRoute era di restituire un puntatore Route quando il viaggio è fattibile (con o senza rifornimento) e NULL in caso di errore:

  • se è necessario un rifornimento di carburante, TestRoute restituisce un puntatore alla nuova rotta;
  • se non è necessario il rifornimento, restituisce un puntatore a una copia della rotta originale, poiché il suo proprietario è Planner .

In entrambi i casi, è compito del chiamante di TestRoute distruggere l'oggetto Route restituito.

Bene, ora un mio collega vuole che il metodo restituisca NULL quando la rotta è fattibile senza fare rifornimento, per evitare inutili copia e distruzione.

Quindi il metodo restituirebbe:

  • NULL in caso di errore
  • un puntatore a Route in caso di successo quando viene calcolato un nuovo Route
  • NULL in caso di successo (!!) quando non è necessario il rifornimento.

Mi sembra un disegno terribile. Posso avere qualche opinione?

    
posta Greg82 25.10.2017 - 14:47
fonte

3 risposte

8

Restituire NULL in caso di successo (il caso più semplice per ottenere risultati positivi) è decisamente contrario a quello che la maggior parte delle persone si aspetterà.

Restituire un puntatore che l'utente deve distruggere manualmente non è neanche eccezionale. Ti suggerirei di usare C ++ 11 unique_ptr ma usare C ++ 11 potrebbe non essere fattibile nel tuo caso.

Un pensiero che ho avuto è di rendere TestRoute private e fare in modo che Planner lo chiami ogni volta che calcola una nuova rotta.

Se il test fallisce, restituisci NULL , altrimenti restituisci il percorso.

La cosa bella di questo approccio è che puoi implementare TestRoute , ma tu (o il tuo collega) per favore, e l'utente della classe non avrà bisogno di sapere i dettagli di come è implementato. L'utente chiederà semplicemente un percorso dal punto A al punto B e sarà garantito che è un percorso valido con i punti di rifornimento a patto che non ricevano NULL .

Puoi anche dividere il tuo metodo in diversi metodi se il rendimento non è eccessivo.

Ad esempio, per TestRoute , devi restituire true se il percorso è possibile, false se non lo è.

bool TestRoute(const Route* r)

Avere un altro metodo TestRouteNeedRefuel che restituisce true se il percorso richiederà il rifornimento, false se non

bool TestRouteNeedRefuel(const Route* r)

Quindi disponi di un metodo finale, GenerateRefuelRoute che restituisce una nuova rotta con i punti di rifornimento appropriati

Route* TestRoute(const Route* r)
//use this if at all possible
std::unique_ptr<Route> TestRoute(const Route* r)

Per quanto riguarda le prestazioni, ricorda di profilo prima di fare ipotesi. Se il tuo collega è preoccupato di copiare Route in più del necessario (e potrebbe avere buone ragioni per non sapere quanto sia costoso o che sia la piattaforma di destinazione), le prestazioni chiaramente sono un requisito importante. Suggerirei innanzitutto di implementare un'interfaccia pulita come si può fare, una profilazione per trovare dove sono VERAMENTE i colli di bottiglia e quindi l'implementazione di alcuni hack di velocità laddove necessario. Come si suol dire, Pre-ottimizzazione è la radice tutto il male:)

    
risposta data 25.10.2017 - 15:07
fonte
5

Considererei generalmente il ritorno di un puntatore da un metodo in C ++ a una progettazione errata e la miscelazione di stati di errore e dati di carico utile anche nel valore di ritorno; questa è una ricetta per codice non gestibile.

Modifica suggerita: restituisce lo stato di fail / success come valore int (o usa la logica ternaria, ad esempio boost :: tribool ) e passa l'argomento come riferimento non const:

/** @returns
    - 1 if a solution has been found. The argument will be updated.
    - 0 if the request has been processed sucessfully, 
      but no (immediate) solution has been found.
      The argument is not modified in this case.
    - -1 if the request failed. The argument is not modified. */
int findSolution(MyClass& argument);

Esempio di utilizzo, tralasciando l'ottimizzazione prematura per evitare copie "non necessarie":

MyClass objectToTest(originalUnmutableObject);

switch(findSolution(objectToTest))
{
    case 1:
        //Replace original with updated object, or whatever
        break;
    case 0:
        //Nothing to do (?)
        break;
    case -1:
        //Error handling
        break;
    default:
        //Unexpected return value
        assert(false);
}

Un approccio alternativo, più sofisticato e riutilizzabile potrebbe essere quello di raggruppare lo stato di errore e l'oggetto in una generica classe di risultati; questo modello è stato ispirato da Rust . Lascio l'implementazione di Result a te.

template<typename T>
class Result
{
    public:
        Result() = delete;
        Result(int error);
        Result(const T& data);
        Result(T&& data);

        //Methods
        bool isOk() const;
        bool isError(int error) const;
        int error() const;
        const T& data() const;

    private:
        //Variables
        int m_error = 0;
        T m_data;
};

...

Result<MyClass> findSolution(const MyClass& argument)
{
    int errorCode = 0;

    ...

    if(errorCode != 0)
        return Result(errorCode);
    else if(solutionFound)
        //Error code of result will be 0, Result::isOk() == true
        return Result(update(argument, solution));
    else
        //Error code of result will be 1, Result::isOk() == false
        return Result(1);
}
    
risposta data 25.10.2017 - 15:04
fonte
1

Si dovrebbe evitare di restituire un puntatore proprietario proprio.
Puoi usare il puntatore intelligente e in particolare shared_ptr

Supponendo che:

bool NoPath(const Route&);
bool NeedRefuel(const Route&);
std::unique_ptr<Route> RouteWithRefuel(const Route&);

puoi fare:

std::shared_ptr<Route> TestRoute(const std::shared_ptr<Route>& r)
{
    if (!r) {
        throw std::runtime_error("null pointer"); // Or more appropriate exception
    }

    if (NoPath(*r)) {
        // throw NoValidPathException{}; // Alternative
        return nullptr; // No valid Path
    } else if (NeedRefuel(*r)) {
        return RouteWithRefuel(*r); 
    } else {
        return r; // share route: Increase ref counting
    }
}
    
risposta data 26.10.2017 - 00:51
fonte

Leggi altre domande sui tag