1
\$\begingroup\$

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);
 }
}
asked Dec 7, 2012 at 0:13
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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?

answered Dec 7, 2012 at 7:04
\$\endgroup\$
8
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 7, 2012 at 22:00
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 7, 2012 at 22:05
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 7, 2012 at 22:32
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 7, 2012 at 22:40
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 7, 2012 at 22:42

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.