8

I have some code similar to shared below, which returns different kinds of response messages to the caller. If the return value is empty string, the process is continued. If a message is returned it will show the message to user. (Please consider this is demonstration code, not really used so I might have some syntax problems)

When I am writing unit tests for this code I am actually comparing the different hard coded string values with the output of the function. It makes me uncomfortable because changing the output string syntax or even fixing spelling mistakes will break my tests.

Is there a better approach to this code? Is there a better design pattern to follow? Thanks in advance.

        public string BookRentCheck(string customerId, string bookId)
        {
            var responseMessage = "";
            bool isPaymentOk = GetPaymentOk(customerId);
            if (!isPaymentOk)
            {
                if (GetAllowedOnCredit(customerId))
                {
                    double availbleCredit = GetAvailableCreditBalance(customerId);
                    double bookRent = GetRentForBook(bookId);
                    if (availbleCredit < bookRent)
                    {
                        responseMessage = "Your credit limit is over";
                        return responseMessage;
                    }
                }
            }
            else
            {
                responseMessage = "Your payment is not clear.";
                return responseMessage;
            }
            if (!bookAvailable(bookId))
            {
                responseMessage = "Book not availble.";
                return responseMessage;
            }
            if (!bookQuotaAvailable(customerId))
            {
                int rentedBookCount = GetRentedBookCount(customerId);
                responseMessage = "You have already rented " + rentedBookCount + ".";
                return responseMessage;
            }
            return responseMessage;
        }
  • 3
    For a start, you can write return "Book not available."; instead of responseMessage = "Book not available."; return responseMessage; – user253751 Nov 2 at 14:50
  • 3
    And remove the variable 'responseMessage' entirely. The only value that can be returned at the end is "". Make that obvious: return ""; – JimmyJames Nov 2 at 16:50
  • Do not remove responseMessage variable. Initializing to the defined/intended "nothing" value of "empty" is the right thing to do. Strings default to null, not String.Empty. Because Initializing variables is a strong signal to the maintenance programmer of intent. – radarbob Nov 2 at 22:21
  • 1
    ... however, only a final return responseMessage is needed.Remove all the intermediate returns. A single return is much more maintenance friendly, especially given multiple ifs with multiple lines. Now, I have used multiple returns but only in very short methods; this case however would not pass code review in my shop. – radarbob Nov 2 at 22:28
14

I would suggest to introduce a special result type, something along the lines of

class RentalCheckResult
{
     public enum CheckState 
     {
         PaymentUnclear, 
         CreditLimitReached,
         BookNotAvailable,
         QuotaExceeded,
         Ok
     }

     public CheckState State {get;private set;}

     private int NoOfBooks;
     
      // "noOfBooks" currently is only used for QuotaExceeded,
      // but introducing an extra subclass just for this state,
      // (or for every CheckState) seems overdesigned.
     public RentalCheckResult(CheckState state, int noOfBooks=0)
     {
        State=state;
        NoOfBooks=noOfBooks;
     }

     public override string ToString()
     {
         switch(State)
         {
              case PaymentUnclear:
                   return "Your payment is not clear.";
              case CreditLimitReached:
                   return "Your credit limit is over";
              case BookNotAvailable:
                   return "Book not availble."
              case QuotaExceeded:
                   return $"You have already rented {NoOfBooks}.books";
              default:
                   return "";

         }
     }
}

I guess the usage in BookRentCheck is clear, it needs to return a RentalCheckResult object instead of a string. This will make it possible to write unit tests for BookRentCheck which are independent from spelling corrections or translations.

RentalCheckResult itself is simple enough that it does not require any unit tests for itself. If it seems necessary, the enum can be replaced to a class hierarchy with subclasses RentalCheckResultPaymentUnclear, RentalCheckResultCreditLimitReached, and so on, where NoOfBooks will only exist as a member of RentalCheckResultQuotaExceeded.

| improve this answer | |
  • Thanks for such detailed organized answer. This suits my needs, I am thinking to use a generalized version of this, so that I can use it in different places. – Tau Nov 2 at 13:04
  • 5
    This solution will also make translation to other languages more easy. – MTilsted Nov 2 at 14:04
  • 1
    I like that you accidentally(?) swapped two values, which will require a fix ;) – Quentin Nov 2 at 14:25
  • In the absence of sum types, should this directly go towards a class hierarchy? I'm somewhat uncomfortable with that Parameter being sometimes used and sometimes not. And what of needing a different type of parameter, or multiple of them? – Matthieu M. Nov 2 at 15:06
  • @Quentin: that was not intended, thanks for proofreading. – Doc Brown Nov 2 at 15:45
5

You are right to be uncomfortable.

You should return invariant status codes and let the caller work out what that means in terms of displaying to a user or taking further action.

There are all sorts of reasons for this

  • Multi-lingual systems
  • Text changes as you say
  • Different messages for different form factors
  • Callers may be processes, not people

etc.

Another consideration in this is that many of those things might be true at once. e.g. you can have low credit and the book is not available.

So consider an array of returns, again, for the purpose of the client system being able to make the best use of the information in a way that makes sense to its context.

so you've got a few options:

  • return a code "NOCRED"
  • return a code + other information { "TOOMANY","7" }
  • return an array of codes [ {"TOOMANY","7"}, {"NOCRED"} ]
  • return an array of codes and overall outcome { OK=TRUE }

or

{
OK=FALSE,    
[    
 {"TOOMANY","7" },    
 {"NOCRED"}    
]
}
| improve this answer | |
  • Can you please give me idea how such status codes are structured? like primitive types like 1,2 etc.? or is there some other generally used standards? – Tau Nov 2 at 3:00
  • 1
    Yeah just an integer is fine though I actually prefer a code so when you're looking at logs or databases it's a bit easier. So something like` "LOWCRED", "NOCLEAR","NOTAVAIL"` etc. If you need to return more detail then a structure with a supplemental field can be useful, e.g. your return type is struct { string Code; string Extra' } – LoztInSpace Nov 2 at 3:34
  • 1
    @Tau, rather than a string-encoded code, as @LoztInSpace suggests, you could also use an enumeration.` – Bart van Ingen Schenau Nov 2 at 7:30
4

You are returning a message for the user, from a method that should be returning a result to the caller.

Consider your function as the equivalent of SQL code. Would it make sense for the SQL query: select count(*) from book b where b.id = @id to return “Book not available.”? You could certainly modify it so that it does so, but it would certainly look weird. Going by the name and signature, your method is supposed to be checking to see if the user can rent a book, not telling the user whether they can rent a book or not. The distinction may seem trivial, but in actuality it’s vast.

I would recommend reading up on the Single Responsibility Principle (SRP) and the re-access your design.

| improve this answer | |
  • I will definitely check on SRP. I didn't think much about the method name and signature, but I can see the problem now you pointed it out. Thanks overall! This is kinda new to me, so every bit helps :) – Tau Nov 2 at 12:56
  • Your comment about name (so purpose) of the function is on point. Certainly not SQL query results, but ironically, SQL error messages do contain a lot of information in plain text. :-( – Pablo H Nov 2 at 20:50
  • @PabloH: sql errors have both status codes and error text, codes for calling methods and text for users/logging. It’s a bit of a mixed environment. – jmoreno Nov 2 at 22:34
2

It makes me uncomfortable because changing the output string syntax or even fixing spelling mistakes will break my tests.

I'd like to call attention to some of the higher level patterns at work here.

One: it is possible that you are being punished for overfitting your tests to the current behavior. As is, your tests are being written at a low abstraction level (the output shall match this sequence of bytes exactly), in contrast to the test you want, which is that the module reports a problem with the credit limit.

Two: it is very uncomfortable, as you have discovered here, to test the stable parts of your code via an unstable interface. This is primarily a coupling problem - you have a "unit" that is a composition of A (your text interface) and B (your underlying logic).

Test-first/test-driven approaches at this point would note that discomfort, and attack it directly - can the design of the code be changed, so that the stable parts can be tested in isolation (as their own "unit")? That's a common refactoring goal: to extract from code units/elements/modules that are easy to test.

Expressed another way - stable behaviors and unstable behaviors require different testing strategies; therefore you prefer designs that allow you to separate those elements so that you can apply the appropriate testing strategy to each.

| improve this answer | |
1

When I am writing unit tests for this code I am actually comparing the different hard coded string values with the output of the function. It makes me uncomfortable because changing the output string syntax or even fixing spelling mistakes will break my tests.

I have written many tests where this is a desirable effect.

I see duplicate literal data like double entry bookkeeping. The concept is that the same data is entered twice and if the two sides of the ledger do not agree then something, somewhere is wrong. Here the two sides are the code and its tests. A fail alerts me that (1) I forgot to update the test (2) data is no longer valid for our program, or (3) that a code change broke a valid test case. Having broken tests that pass - that are forever lying - is a very, very bad thing.

Do not trivialize string details. "Don't, Stop!" is not the same as "Don't Stop!"

| improve this answer | |
  • By 'break my tests', I meant the test will not give me the desired output. I also test with string literals, but in this case the string just describes a status/state. I was confused at first but with the help of answers given, I realize that I want to actually test for the output state/status. – Tau Nov 4 at 0:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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