Code Smell of the Week: Obsessed with Out Parameters
Consider the following code to retrieve a user’s profile:
public bool GetUserProfile(string userName, out Profile userProfile, out string msg) { msg = string.Empty; userProfile = null; if (some_validations_here)) { msg = string.Format("Insufficient data to get Profile for username: {0}.", userName); return false; } IList<User> users = // retrieve from database if (users.Count() > 1) { msg = string.Format("Username {0} has {1} Profiles", userName, users.Count()); return false; } if (users.Count() == 0) { userProfile = Profiles.Guest; } else { userProfile = users.Get(0).Profile; } return true; } |
Notice the bool return value and the use of out parameters. This code is heavily influenced by COM & C Programming. We don’t operate under the same constraints these days.
If we were to write a test for this method, what would it look like?
[TestClass] public class ProfileControllerTest { private string msg; private Profile userProfile; //create a fakeDB private ProfileController controller = new ProfileController(fakeDB); private const string UserName = "Naresh.Jain"; [TestMethod] public void ValidUserNameIsRequiredToGetProfile() { var emptyUserName = ""; Assert.IsFalse(controller.GetUserProfile(emptyUserName, out userProfile, out msg)); Assert.IsNull(userProfile); Assert.AreEqual("Insufficient data to get Profile for username: " + UserName + ".", msg); } [TestMethod] public void UsersCannotHaveMultipleProfiles() { //fake DB returns 2 records Assert.IsFalse(controller.GetUserProfile(UserName, out userProfile, out msg)); Assert.IsNull(userProfile); Assert.AreEqual("Username "+ UserName +" has 2 Profiles.", msg); } [TestMethod] public void ProfileDefaultedToGuestWhenNoRecordsAreFound() { //fake DB does not return any records Assert.IsTrue(controller.GetUserProfile(UserName, out userProfile, out msg)); Assert.AreEqual(Profiles.Guest, userProfile); Assert.IsNull(msg); } [TestMethod] public void MatchingProfileIsRetrievedForValidUserName() { //fake DB returns valid tester Assert.IsTrue(controller.GetUserProfile(UserName, out userProfile, out msg)); Assert.AreEqual(Profiles.Tester, userProfile); Assert.IsNull(msg); } } |
This code really stinks.
What problems do you see with this approach?
- Code like this lacks encapsulation. All the out parameters could be encapsulated into an object.
- Encourages duplication in both client code and inside this method.
- The caller of this method needs to check the return value first. If its false then they need to get the msg and do the needful. Its very easy to ignore the failure conditions. (In fact with this very code we saw that happen in 4 out of 6 places.)
- Tests have to validate multiple things to ensure the code is functions correctly.
- Overall more difficult to understand
We can refactor this code as follows:
public Profile GetUserProfile(string userName) { if (some_validations_here)) throw new Exception(string.Format("Insufficient data to get Profile for username: {0}.", userName)); IList<User> users = // retrieve from database if (users.Count() > 1) throw new Exception(string.Format(""Username {0} has {1} Profiles", userName, users.Count())); if (users.Count() == 0) return Profiles.Guest; return users.Get(0).Profile; } |
and Test code as:
[TestClass] public class ProfileControllerTest { //create a fakeDB private ProfileController controller = new ProfileController(fakeDB); private const string UserName = "Naresh.Jain"; [TestMethod] [ExpectedException(typeof(Exception), "Insufficient data to get Profile for username: .")] public void ValidUserNameIsRequiredToGetProfile() { var emptyUserName = ""; controller.GetUserProfile(emptyUserName); } [TestMethod] [ExpectedException(typeof(Exception), "Username "+ UserName +" has 2 Profiles.")] public void UsersCannotHaveMultipleProfiles() { //fake DB returns 2 records controller.GetUserProfile(UserName); } [TestMethod] public void ProfileDefaultedToGuestWhenNoRecordsAreFound() { //fake DB does not return any records Assert.AreEqual(Profiles.Guest, controller.GetUserProfile(UserName)); } [TestMethod] public void MatchingProfileIsRetrievedForValidUserName() { //fake DB returns valid tester Assert.AreEqual(Profiles.Tester, controller.GetUserProfile(UserName)); } } |
See how simple the client code (tests are also client code) can be.
My heart sinks when I see the following code:
public bool GetDataFromConfig(out double[] i, out double[] x, out double[] y, out double[] z)... public bool AdjustDataBasedOnCorrelation(double correlation, out double[] i, out double[] x, out double[] y, out double[] z)... public bool Add(double[][] factor, out double[] i, out double[] x, out double[] y, out double[] z)... |
I sincerely hope we can find a home (encapsulation) for all these orphans (i, x, y and z).
