What would be a better approach to write methods which returns several different string outputs?

Question

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;
        }

Doc Brown · Accepted Answer

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.

radarbob · Answer

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!"

VoiceOfUnreason · Answer

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.

jmoreno · Answer

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.

LoztInSpace · Answer

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"}    
]
}

What would be a better approach to write methods which returns several different string outputs?

5 Answers

Add your own answers!

Ask a Question