Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have two different ideas on how to handle this Update method in my MVC Model class "DataAccess" And I'm wondering if in the first, I'm handling my class incorrectly or ambiguously, because I'm using members from the same object for two different purposes.

The man Class has 2 relevant members. ID, which is the primary key of the entity, and Name, which is the description

    public DbResult Update(Man man)
    {
        DbResult dbResult; 

        var query =
            from person in _MyDBEntities.Men
            where person.ManID == man.ManID // Man with ManID is what we're looking for.
            select person;

        try
        {
            foreach (Man m in myQuery)
            {
                m.Name = man.Name; // man.Name is the new name
            }
            dbResult = DbResult.Success("Record updated");
        }
        catch (Exception e)
        {
            dbResult = DbResult.Failed(e.ToString());   
        }
        return dbResult;
    }

And this version of the method, which uses the variables passed in differently.

    public DbResult Update(int idToLookFor, string newName)
    {
        DbResult myResult; 

        var query =
            from person in _MyDBEntities.Men
            where person.ManID == idToLookFor
            select person;

        try
        {
            foreach (Man m in myQuery)
            {
                m.Name = newName;
            }
            dbResult = DbResult.Success("Record updated");
        }
        catch (Exception e)
        {
            dbResult = DbResult.Failed(e.ToString());   
        }
        return dbResult;
    }

is my first method a better or ok approach to the problem? Because what if I have an update method later on that uses 8 members, and need to create more versions of this function for updating other entities?

## UPDATED ##

Including a new class here:

public class InputValidation//<T> where T: class
{
    /// <summary>
    /// Delegate that matches the signature of TryParse, method defined for all primitives.
    /// </summary>
    /// <typeparam name="T">Output type of This Delegate</typeparam>
    /// <param name="input">input for this Delegate to translate to type T</param>
    /// <param name="output">The translated variable to return via out parameter</param>
    /// <returns>Whether the Parse was successful or not, and output as output</returns>
    public delegate bool TryParse<T>(string input, out T output);
    public bool ValidateInputType<T>(string input, TryParse<T> TypeValidator, out T result)
    {
        return TypeValidator(input, out result);
    }
    public bool ValidateInputRange<T>(ref T result, int lower, int upper)
    {
        // How can I use relational operators like > < = on T? without forcing T result to an int?
        return isValidRange(int.Parse(result.ToString()), lower, upper);
    }
    public bool isValidRange(int item, int lower, int upper)
    {
        return (lower <= item && item <= upper);
    }
}
share|improve this question
add comment

1 Answer 1

up vote 1 down vote accepted

The second one seams to me a better approach but you say that the ManID is a primary key, so why are you handling it as a non primary key and using the query result as a sequance?

public DbResult Update(int idToLookFor, string newName)
{
    return Update(_MyDBEntities.Men.Single(p => p.ManID == idToLookFor), newName);
}

public DbResult Update(Man person, string newName)
{
    try
    {
        person.Name = newName;
        //saving missing?
        return DbResult.Success("Record updated");
    }
    catch (Exception e)
    {
        return DbResult.Failed(e.ToString());
    }
}

And the real saving missing or it's placed to elsewhere so why are you setting here the DbResult?

share|improve this answer
    
I have my own DataAccess.SaveChanges() method that's called by the controller after program flow returns to the controller. However, now you have me thinking it would be just better to put the standard savechanges method in the update method instead. –  Matt Rohde Dec 7 '12 at 22:00
    
No-no-no, it is a good think that you saving changes somewhere outside this update method becouse this way you can handle transaction-s (even nested transactions) correctly. The bad smell is the usage of the DbResult. –  Peter Kiss Dec 7 '12 at 22:05
    
DbResult is a wrapper class that I use to encapsulate a bool and string, the bool tells if the database operation was successful and the string holds the message (which either is a "success! you did it" or "exception goes here" everyone tells me it's a bad idea to return an exception so i'm returning a string that has the exception in it. But now i'm starting to understand why that may be bad to do. The user doesn't care what went wrong, he wants it to go right. –  Matt Rohde Dec 7 '12 at 22:32
    
Actually, How else can I return the information without using the wrapper class DbResult? I have been told about the bad smell of DbResult, and I'm trying to figure out a way around it, but you see, the controller calls this method, and expects to know what went wrong, if anything. so that the controller can pass the error message to the UI class for printing to console. –  Matt Rohde Dec 7 '12 at 22:40
    
You are missing one point in the saving process: validation. Validation is that step which can provide useful messages to the user anything beyound that is exception, general error, nothing to the end user. –  Peter Kiss Dec 7 '12 at 22:42
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.