Responsabilità singola di una funzione

3

Vorrei sapere se comprendo correttamente il principio di responsabilità singola.

La funzione seguente suppone di restituire l'archivio ID utente nel database utilizzando il nome dell'account SAM per passarvi.

Guarda i seguenti codici commentati, è vero riguardo alla singola responsabilità di una funzione / metodo?

Questa funzione viola il principio di responsabilità singola se quei codici non sono commentati?

public static int GetUserId(string domainName)
{
    string message = "";

    //Do not get badge number from this function,
    //function should stay as single responsibility

    //string badgeNumber = AsmUser.AsmUserHandler.GetBadgeNumber(domainName);
    //if (badgeNumber == null)
    //{
    //    message = "Badge number is null.";
    //    LogHandler.LogDebug(logger, message);
    //    return 0;
    //}
    //else
    {
        using (var db = new SurfaceTreatment.SurfaceTreatmentEntities())
        {
            string domain = domainName.Split('\')[0].Trim();
            string samAccountName = domainName.Split('\')[1].Trim();
            var user = db.t_ST_User
                //.Where(_user => string.Compare(
                    // badgeNumber, 
                    // _user.badgeNumber, 
                    // StringComparison.OrdinalIgnoreCase) == 0)
                .Where(_user => string.Compare(
                    domain, 
                    _user.domainName.Trim(), 
                    StringComparison.OrdinalIgnoreCase) == 0)
                .Where(_user => string.Compare(
                    samAccountName, 
                    _user.samAccountName.Trim(), 
                    StringComparison.OrdinalIgnoreCase) == 0)
                .Where(_user => _user.deleted == false)
                    .FirstOrDefault();
            if (user == null)
            {
                message = "Failed to get user by badge number: " + domainName;
                LogHandler.LogDebug(logger, message);
                return 0;
            }
            else
            {
                return user.userId;
            }
        }
    }
}
    
posta Pop 06.11.2017 - 04:20
fonte

4 risposte

4

Questa funzione unica responsabilità è ottenere l'ID utente associato a un nome di dominio. Sfortunatamente in questo sistema, apparentemente, non è un compito semplice. Avere un'unica responsabilità non significa che il compito sia semplice. Significa che il compito è focalizzato. Dovrebbe avere sempre una sola ragione per cambiare.

Il mio più grande problema con questo codice è proprio qui:

 message = "Failed to get user by badge number: " + domainName;

Ci sono altri 3 modi per ottenere un utente nullo e finire qui, tuttavia questo messaggio di registro incolpa sempre il numero del badge. Se stavo cercando di eseguire il debug di questo non vorresti che io sappia dove vivi.

Questo è un problema di tracciabilità. Non un singolo problema di responsabilità. Dove questo potrebbe essere un singolo problema di responsabilità è se eliminare utenti basati su badgeNumber non è necessario per estirpare a un risultato. Come se lo facessi solo come un pazzo tipo di controllo di integrità. Sarebbe una responsabilità extra inutile. Voglio dire, cosa succede se AsmUser.AsmUserHandler.GetBadgeNumber(domainName) decide di voler essere sicuro che domainName produca un utente con un ID utente? Ti farà solo girare in cerchio. No. Rendi solo badgeNumber parte di ottenere un ID utente se ne hai davvero bisogno per ottenere un ID utente. Effettua i tuoi controlli di integrità separatamente.

E Euforico ha un punto sul ritorno 0. Questo assomiglia al codice C #. Qualche ragione per cui non stai lanciando eccezioni?

    
risposta data 06.11.2017 - 06:31
fonte
1

SRP si applica alle classi (o ai moduli), non alle funzioni, quindi questa è una specie di domanda strana. Interpreterò la tua domanda nel senso di "Come posso migliorare la struttura di questo codice per la manutenibilità?" senza riferimento a nessun principio specifico.

Personalmente, prenderei in considerazione di suddividere la funzione in funzioni separate nella stessa classe. In questo modo posso avere funzioni piccole e ben definite che fanno una cosa specifica (hanno una "responsabilità unica", per così dire). Le funzioni che non desidero vengano esposte avranno come private .

In questo caso è una classe su un utente, quindi la chiamerò MyUserClass (probabilmente dovresti cambiare il nome).

class MyUserClass
{
    private readonly string _domainNameString;
    private int _badgeNumber = 0;

    public MyUserClass(string domainName)
    {
        _domainNameString = domainName;
    }

    private int GetBadgeNumber()
    {
        var result = AsmUser.AsmUserHandler.GetBadgeNumber(_domainNameString);
        if (result == 0)
        {
            throw ArgumentException(String.Format("No badge number found for domain {0}", _domainNameString), "domainName");
        }
        return result;
    }

    private int GetBadgeNumberWithCache()
    {
        if (_badgeNumber == 0) 
        {
            _badgeNumber = GetBadgeNumber();
        }
        return _badgeNumber;
    }

    private string Domain
    {
        get 
        {
            return _domainNameString.Split('\')[0].Trim();
        }
    }

    private string AccountName
    {
        get
        {
            return _domainNameString.Split('\')[1].Trim();
        }
    }

    private bool IsMatch(User candidate)
    {
        return string.Compare(candidate.domainName, this.Domain, StringComparison.OrdinalIgnoreCase) == 0
            && string.Compare(candidate.samAccountName, this.AccountName, StringComparison.OrdinalIgnoreCase) == 0
            && string.Compare(candidate.badgeNumber, this.GetBadgeNumberWithCache(), StringComparison.OrdinalIgnoreCase) == 0;
    }

    private User GetUser()
    {
        using (var db = new SurfaceTreatment.SurfaceTreatmentEntities())
        {
            var user = db.t_ST_User
                .Where(u => !u.deleted && this.IsMatch(u))
                .SingleOrDefault();
            if (user == null)
            {
                throw new InvalidOperationException("Failed to get user.");
            }
            return user;
        }
    }

    public int GetUserID(string domainName)
    {
        return this.GetUser().userId;
    }
}

Questo approccio separa la logica per

  • Analisi della stringa del dominio per ottenere il dominio
  • Analisi della stringa del dominio per ottenere il nome utente
  • Confronto tra due utenti per vedere se corrispondono
  • Chiamare un servizio per ottenere il numero del badge
  • Cache risultati della chiamata di servizio per evitare di recuperare ripetutamente lo stesso numero di badge
  • Interpretazione di qualsiasi numero magico restituito dal servizio (ad esempio 0 )

Dovresti quindi utilizzare il codice in questo modo:

public static int GetUserID(string domainName)
{
    var u = new MyUserClass(domainName);
    return u.GetUserID();
}
    
risposta data 06.11.2017 - 22:37
fonte
0

Mi avvicinerò a questo da una prospettiva logica.

Il codice commentato, se mai non commentato (eseguito), introduce una precondizione per il resto del codice (esistente). Se GetBadgeNumber restituisce null, il resto del codice esistente non verrà eseguito. In altre parole, ciò che fa la funzione è GetUserIdIfBadgeNotNull .

Questa nuova precondizione cambierà significativamente la logica e il flusso dell'intera funzione? La risposta a questa domanda dipende in modo significativo dalla relazione tra i dati del numero del badge e la ricerca della stringa in base al nome del dominio.

Situazione 1 - se sono tutti coerenti

Significa che le seguenti tre condizioni sono o completamente vere o false.

  • il nome di dominio esiste nel database
  • Il numero del badge
  • viene restituito da AsmUser.AsmUserHandler.GetBadgeNumber
  • Il numero del badge e il nome del dominio corrispondono a ciò che si trova nel database.

Se sono tutti coerenti, non altera la logica della funzione. Può essere visto come un caching di query su database o hack di ottimizzazione. Fornisce inoltre un messaggio di errore diverso rispetto a quando la query del database non ha restituito un risultato.

Queste sono le responsabilità sotto la situazione 1: in genere, per essere coerente con un database (attivo), è meglio lasciare che sia gestito dal database. Se si intende il primo numero di badge di ricerca come cache, è necessario eseguire una ricerca con il database anche se la cache non ha colpito. Infine, è necessario assicurarsi che il nome di un dominio sia sempre collegato allo stesso numero di badge (e viceversa), anche se un nome di dominio viene temporaneamente disattivato e quindi riattivato nel sistema. Questo in genere significa che non si elimina mai completamente la riga del database; viene semplicemente contrassegnato come inattivo in modo che possa essere ripristinato, se necessario.

Situazione 2 - se non sono intesi per essere coerenti (lo stesso) e il risultato di GetBadgeNumber è più importante del risultato della query del database.

Questo farà sì che AsmUser.AsmUserHandler.GetBadgeNumber si comporti come un guardiano di porta. Se restituisce null, non avviene alcuna query sul database.

In questo caso, consiglierei di rinominare la funzione in GetUserIdIfBadgeNotNull per renderlo evidente agli altri programmatori.

Situazione 3 - se non intendono essere coerenti (uguali), ma GetBadgeNumber è meno importante del risultato dalla query del database.

Congratulazioni! Hai trovato / piantato un bug nel sistema.

    
risposta data 07.11.2017 - 07:19
fonte
0

Dipende dalla singola responsabilità che si desidera che questa funzione fornisca. Sembra che le sezioni commentate stiano semplicemente imponendo che questa funzione restituisca solo un ID utente di quelli con numeri di badge nel tuo sistema.

Se rinominate il metodo per renderlo più chiaro, come getUserIdWithValidBadgeNumber, renderà il vostro codice più facile da capire - ma poi i colleghi a cui sono stati insegnati i nomi dei metodi non dovrebbe essere più lungo di X caratteri che lo contesteranno.

Inoltre, considera il factoring dello snippet che convalida il numero del badge e inserisci quel codice in un altro metodo chiamato hasBadgeNumber o qualcosa del genere. Metti i dettagli di come viene determinato se il numero del badge esiste o meno in quel metodo, e ora hai una linea semplice per controllare il numero del badge prima di procedere.

Ciò faciliterà la manutenibilità del codice, perché è molto probabile che ci sarà più di un posto nella tua base di codice in cui stai controllando l'esistenza di un numero di badge. Se in tutti questi luoghi chiami questo metodo, se dovessi cambiare la struttura dei dati sottostanti che influisce sulla tua logica nel determinare un numero di badge, hai un posto dove cambiare per farlo accadere.

Il tuo codice trarrebbe vantaggio anche dall'aggiunta di metodi dedicati per analizzare il dominio e userid da domainName come MYDOMAIN \ myId.

    
risposta data 09.11.2017 - 15:18
fonte

Leggi altre domande sui tag