TDD and Fail Code

I don't think I agree about throwing vs not. If authorization failure is a normal and expected condition, and not an exceptional case, I don't like throwing an Exception.

Exceptions are a tough judgment call that I suppose are more of a personal preference.

In this case, I side with Kevin Dante; I don't think a failed authorization or capture is very exceptional, since I see them several times per day.

What would be exceptional is an error that is semantically unrelated to the authorization/capture action, such as a network timeout, a SOAP deserialization error, etc....

I see your points but in this case my method returns a Transaction - not a boolean. If it can't return a transaction I say throw...

But that begs the question - is this a good method structure? I could use a boolean I spose - thoughts?

If that class is for testing, why not just use a Mock?!

You could create an expectation that a method is called on the interface and throw an exception.

Its a tricky one...

I recently implemented a payment service, and have a similar interface to above, but my return object returns a success boolean along with a failure reason enum (amongt other info)

There are many different responses that the calling code needs to be aware of other than success true/false.

Gateway busy - user should try again

Invalid details - passed our local checks, but not the payment gateways. User to correct details

Declined - User to try a different card

etc

I'm not sure that payment failure is that "exceptional"

You could return a IPaymentServiceResult which contains your transaction if successful and other information (and a null transaction) if not.

+1 for Mocking this.

Krzysztof Cwalina just posted up a new note on the time old "Exceptions are Exceptional" debate blogs.msdn.com/.../default.aspx

Ben

It's hard to judge without seeing the structure of your Transaction object. If it includes a "decision" enumeration (accept, decline, review) you could use the Transaction to reflect exceptions by adding an error value.

If not, i'm with Ben. with payment stuff, you always want to return and record all "payment events", even if they weren't successful. Most payment gateways charge per-request fees and the accounting types like to be able to reconcile those charges with "payment event" records in the database.

Michael.

Mocking is a reasonable idea - it's also more code :). Good points though...

In terms of returning a result - I still don't think that works as flaws in the logic could actually result in the transaction happening. Yes, I could have an IPaymentResult - but isn't that a Transaction? Yes, you could have a boolean on it that says "did it go through" or you could test nullability - but this seems a little "codey" to me.

If it fails and a reason is returned - use that as the Exception message and throw it as it all resolves down to an InvalidOperation yes?

I think I'm stuck on the idea that if a Transaction can't be returned you need to throw. In terms of recording this event - I don't know if I'd want it down on that level?

I like this discussion :) it's healthy.

Mocking is code, but it is code in your test. It keeps the test and it's dependencies together as a nice unit.

I am making some assumptions about what your Transaction is. I am assuming it is an object you can only create/get IF you get a valid response from your payment provider. If that is a correct assumption, a PaymentResult is not a transaction.

You can always return a payment result, even if you cannot talk to your payment provider. A PaymentResult may or may not CONTAIN a transaction depending on the outcome of the call to the payment service. And forget the bools, let's have a nice enum that tells us the outcome of the service call (success, timeout, auth failure etc)

InvalidOperation does not feel right. The framework docs define InvalidOperation as "The exception that is thrown when a method call is invalid for the object's current state.". This is not the case here. The method call is valid, it just failed.

As for the "seems a little 'codey' to me"... c'mon Rob. Writing code that expresses the intent of our domain, is what we get paid for!

If any of the above does not make sense, I blame it on the fact that I just got back from the pub. I may or may not try and make more sense in the next day or so.

Keep up the good work.

Ben

Thanks Ben - good points...

>>>Mocking is code, but it is code in your test<<<

Well strictly speaking so is the code above; it's a stub and it's pretty simple to write. My point is: "why do i need to wire up mocking to throw an Exception"? Seems silly.

>>>a PaymentResult is not a transaction<<<

Interesting. I know where you're going, but if you say that sentence aloud a few times it sounds funny :).

I know what InvalidOperation is :) and in this sense, if I can't create a transaction then my order is invalid - yes?

>>>As for the "seems a little 'codey' to me"... c'mon Rob. Writing code that expresses the intent of our domain, is what we get paid for!<<<

Wiring/mocking an exception throw is ridiculous - you have to gimme that...

>>I just got back from the pub<<

Beeeeeerrrr...

So I understand the points being made, but their really theoretical to this point and so far, in my eyes, point to "what docs say" and "what others say I should do".

A transaction (in a Payment sense) is when money changes hands. Why do I need to decorate this? If I can't make a sale, I can't make a transaction. If I can't make a transaction happen ... then what?

Yah I can read an enum. Yes I can read message from the service, and yes I can bury my transaction object inside this PaymentResponse object.

But what you have is the "non-exception" way of handling a boolean state "payment=false" with some extra information "bad card number".

I can have one less object, less handling code, and a cleaner process that stops execution - I'd like to know why this is bad.

I would suggest returning error information for each call if the call itself has been successfully made and reserve exceptions for things like network failures.

Imagine a scenario where a user tries to Authenticate an order but the actual Authentication service is down. This would raise an exception but in your scenario you would not easily be able to tell why without making further calls (to a service that is down) - this could be time consuming. (Yes I know the exception type can be tested but this is fraught with danger).

But more importantly I believe there is a bigger issue here...

You have created an IPaymentService interface for (I presume) the purpose of allowing people to change the payment service the web site uses.

As you cannot guarantee whether a third party will (a) throw exceptions or (b) return nulls, it is necessary to test for both scenarios.

By making each method return an object that encapsulates return status and any error messages it (more explicitly) guides any third party developer to "do the right thing".

Infact, any time tests involve interfaces, these tests must be robust enough to cover ANY scenario that may be presented by a third party.

Thanks for keeping us all thinking and keep up the good work

Tony

"If a method can't do what it's supposed to, throw" is a bit too simplistic for me. It implies that you could rename the method AuthorizeIfPossible and then it shouldn't throw an exception, which I don't think should be a valid solution to this problem.

I think that an exception should be thrown in this case. I also understand that the whole debate is pretty subjective and I can see how some people would take the other side. But in my opinion, failure to authorize payment is an exceptional condition; its not on the "happy path". The fact that we expect that it will happen occasionally does not make it less exceptional (which of course raises the question of what "exceptional" means; don't ask me, I'm just a programmer).

Hi Tony - thanks for the thoughts.

>>>Imagine a scenario where a user tries to Authenticate an order but the actual Authentication service is down. This would raise an exception but in your scenario you would not easily be able to tell why<<<

Interesting thought - in some ways this is a perfect example; if the service/network is down, you'll get an error of some kind - probably a timeout, that you will have to handle at some point in the stack. Most likely it would be in your Business Logic and you'd communicate that somehow...

The thing is (and I've assumed this from the start) you'll have to wrap the call to IPaymentService with a try/catch anyway and it really doesn't matter if the service is down, the card is bad - whatever; you can catch the error and communicate it as needed to the end user (and log it, etc).

If the payment isn't authorized - sure you can examine a return object and see why, and communicate it back, but you have more code overhead now.

>>>As you cannot guarantee whether a third party will (a) throw exceptions or (b) return nulls, it is necessary to test for both scenarios.<<<

Agreed.

>>>By making each method return an object that encapsulates return status and any error messages it (more explicitly) guides any third party developer to "do the right thing".<<<

Disagree. If they would return a null transaction, they are just as likely to return a null Response object.

>>>Infact, any time tests involve interfaces, these tests must be robust enough to cover ANY scenario that may be presented by a third party.<<<

100% agree, and it's precisely why I want to keep this simple :). If you add a Response object into the mix you now how to account for its nullability. And what about a non-null Response with Success, but the Transaction is null - is that Exceptional? I say it is - so let's just cut to the chase shall we?

>>>"If a method can't do what it's supposed to, throw" is a bit too simplistic for me. It implies that you could rename the method AuthorizeIfPossible and then it shouldn't throw an exception, which I don't think should be a valid solution to this problem.<<<

You've pinpointed a major issue with coders :). I could reword it to say "If a method is prevented from executing and returning valid data, it should throw". I agree that changing the name is weird :).

By the way - great conversation and I really appreciate the input thus far. I've gotten some flack for "my way/highway" behavior so let this be the requisite "love you man... willing to change my mind but convince me..." comment.

Seriously - I'm completely open to being convinced otherwise; I just don't want to write more code if I don't have to...

When you communicate the issue down to the end user, what message are you gonna display? A return status code lets you break it out into different messages depending on the severity or type of failure. And you may need to localize that message. An exception with a embedded status code would work.

"I can have one less object, less handling code, and a cleaner process that stops execution - I'd like to know why this is bad."

I don't think this is bad. I would just do it differently :)

You are of course right about the stub. I just prefer the actual unit test method body to be self-contained. e.g.

[Fact]

public void Authorize_PassedValidOrder_ThrowsInvalidOperation()

{

var order = Order.MakeAGoodOrder();//for example brevity

var mock = new Mock<IPaymentService>();

mock.Expect(ps => ps.Authorize(order)).Throws(new InvalidOperationException());

mock.Object.Authorize(order);

}

[Note: I am pretty sure I have got the Moq syntax above wrong. However, I have no dev env to hand in order to check. For some reason my Mum does not have Visual Studio installed.]

As for returning a PaymentServiceResponse. Yes, in the exception throwing world you write one less object. However, the value of having that object is far greater (in my opinion) than the cost of writing it (especially if you use Resharper, which makes it basically free ;).

I will try and write up a concrete example so I do not appear theoretical. I am offline until Tuesday, so I will check back then and see if anyone still cares ;)

Ben

Not really answering anything here, just adding an extra part to the puzzle. Would both methods deal equally well with something like 3D Secure? I think this is used in the US and the UK?

In this example you could get a valid transaction back but still need the user to do some work and there could be the possibility of the service to fail, payment to be accepted or declined.

The name of the method should tell you. In the case of a method Authorize(), I agree with you that it should throw an exception.

However, in this example, as others have pointed out that authorisation failures are valid, throwing an exception probably is not the best way.

To me, the problem is in the name - it't not clear what the method is doing. It's not simply authorising, it's *trying* to authorise and may succeed or not - both are valid. So my solution would be to rename the method using the .Net TryXxxx convention:

bool TryAuthorize(Commerce.MVC.Data.Order order);

If you need some kind of authorisation result, add an out parameter. Or return a struct that encapsulates success or failure and the authorisation result.

That way, it's clear to anyone that the method isn't guaranteed to authorise - that non authorisation is one of the expected results, not an exceptional one.

I keep seeing the word failure thrown about and think we need to remember that there are 2 types of failure in the Authorize() method (this seems to be the one people are focusing on because there are 2 types of failures in it). There is the "card declined" type failure and the "oh nos, the internet is down" failure. My opinion is the first type of failure should not throw an exception because the method did everything it should have done. Throwing an exception just because you got a false Authorization would be like me writing a FirstNameIsBob(Person p) method and throwing an exception if the first name wasn't Bob. At least that is the way it feels to me. On the other side, if the authorization service doesn't respond, or gives an unexpected result, then I would throw an exception.

Rob, given the clarification in your comments above, I agree with you that exeptions are the way to go however...

I feel the key issue here is what information we are going to pass back to the user. In most business scenarios we cannot just return the text from an exception (and then there is i8n to worry about). We can of course create a PayementServiceException that contains the error status and throw that but is this not replacing a response object with an excception object.

One final thought. At some later stage you may want to allow (authorised) business partners to place orders directly through your payment service (web services). It is not practical (they may be running Java et al) to throw errors in this scenario, you may wrap the whole thing but now you are by definition creating a response type object.

I guess what I am trying to say here is that it very much depends on (a) your requirements and (b) your prefered coding style. I do not think either way is better or wrong just that one way may be more suitable in a given circumstance.

Tony (and others) - excellent point RE client consumption and also localization of error messages. Localization is a very big concern and you're correct, throwing and returning the message is just not a good idea.

I think what's stuck in my head is that I want a Transaction back and to me, a Transaction involves money. This isn't fair because (as you guys are pointing out) I'm not allowing for failure and disclosure.

Soooo I can do one of two things - add in an enum (as someone suggested above) with TransactionStatus or create a PaymentResponse object.

I think I like the idea of an enum on Transaction, which encapsulates the response types.

It does separate (in a much nicer way as @Kevin points out) the idea of a network/service exception vs. payment issue - which is important.

I like it - this is good...

Why not have an InitializeTransaction() method that returns a transaction and pass that transaction to the Authorize method? Seems like your Authorize method has too many responsibilities. Is it authorizing? Or is it starting a transaction?

It feels odd to me that you throw in the Authorize method. As you stated:

"If a method can't do what it's supposed to, throw."

But your method *was* able to do what it's supposed to. It "authorizes" a transaction. For example, would you throw an exception in a login method if the user mistypes a password? That's hardly an exceptional condition.

A better example of where you would throw an exception is in InitializeTransaction(). The method is supposed to start up a transaction. If it can't, that would be the place to throw an exception. It wasn't able to do what it was supposed to.

"I think I like the idea of an enum on Transaction, which encapsulates the response types."

Just to continue my contrary train... I don't think the Transaction object should have any knowledge of the response types. A response type is not a logical member of a Transaction. Also, I am assuming you will only have a valid Transaction object (representing a Money movement or reservation) when Authorize or Refund succeed. If this is correct, then when it fails (e.g. card declined) you will need to return a NullTransaction with the enum set to a failure code.

How about if I cunningly rename IPaymentServiceResult to ITransactionRequestResponse!? It might look a little something like this...

public interface ITransactionRequestResponse

{

PaymentServiceResponseStatus ResponseStatus {get;}

Transaction Transaction { get;}

}

public enum PaymentServiceResponseStatus

{

OK,

CardDeclined,

ServiceLimitReached,

}

OK, I really am going away now :)

Thanks Phil and Ben - I like the points raised...

>>>public interface ITransactionRequestResponse<<<

Barf :).

Part of me wonders whether I really care about the gateway's response. Do I care if their limit's reached? I doubt they'll tell me anyway - it usually just says "Declined".

So I have 3 conditions:

1) Accepted

2) Service/Network Error

3) Declined

Phil I like your idea about passing in a Transaction; I need to think on this some more because I really dislike the eBay/PayPal "RequestResponseTypeResponseTypeRequest" stuff. Such amazingly generic terms and really folks, all I want here is to know if the PaymentService is going to authorize.

I hate that type of naming too. Since 2) you are going to throw (at least I think you are, dont know where it all stands now), couldn't you just have a Authorized bool on the transaction, or an enum {Awaiting, Accepted, Declined}?

I tend to favor an exception so that you can halt processing; you inherently eliminate having to check for a null value at your calling function.

Some might argue: "well how hard is it to check for a null value"

My argument to that is that as a human, I'm more liable to "forget" to trap a null value then I am to trap an exception. What about other people who call my function as well? They need to remember to trap the null as well.

Cheers.

@Kevin - As per my post above, without knowing more about the Transaction object, I have assumed we only get a Transaction if the call to the remote service succeeds (i.e. you reserve or take the money) but not when it fails (i.e. limit reached, fraud blocked etc). If we always get a transaction then your suggestion seems sound to me.

@Dave Savage - The issue is that we may get an error and not want to halt processing. Plus, unless I read the docs I wont know what exceptions I am getting. A return object with an intention revealing interface should be easier for a dev.

@Rob - I kinda hear what you are saying but I don't think the terms are generic. You are making a request for a Transaction (TransactionRequest). The remote service provides you a response to your TransactionRequest which is a TransactionRequestResponse or TransactionRequestReply (whatever). This may or may not contain successful transaction information along with other status information.

If you want to keep it super simple now. Then I think Mike Scott made a good suggestion above with his TryXxxx pattern (e.g. TryAuthorize(...)). The thing I prefer about the response object is that it is minimal code and it gives you an easy extension point for the future. If you start adding support for further payment services that do give you extra data (fraud score for the card, address verification, extended failure information etc) then you can add it to the response object without breaking the method signature and without it polluting the transaction object.

Plus, if you think two of the biggest transaction players in the market use this pattern (eBay/PayPal) surely that should suggest there might be good reasons for it (unless you think they are just architecture astrounauting, which is possible).

"Part of me wonders whether I really care about the gateway's response. Do I care if their limit's reached? I doubt they'll tell me anyway - it usually just says "Declined".

In my experience you usually get a lot more than just declined. If we look at the PayPal Direct Payment API https://www.paypal.com/en_US/ebook/PP_APIReference/dcc.html. This looks closest to what you are building. With this API you call DoDirectPaymentRequest to do an Authorise or Sale (Capture). If all goes well with DoDirectPaymentRequest www.paypal.com/.../dcc.html you get a DoDirectPaymentResponse https://www.paypal.com/en_US/ebook/PP_APIReference/dcc.html#1394852. If it does not go well, you get an Error Response which could include one or more of these errors https://www.paypal.com/en_US/ebook/PP_APIReference/Appx-ErrorCodes_and_Messages.html#2259084. There are ~114 different possible errors. Some of them would indicate bugs in your code (these we should throw), some are customer issues (not enough money) so show them a message, some are temporal (gateway not available) so maybe retry... etc

Anyway, I get the feeling I'm not going to be able to convince you. It is not easy to do in comment ping pong (and of course I could be wrong!). Also, as I don't know the internals of the Transaction object I may be off base. It would be fun to discuss with you. I have implemented 30+ large scale ecommerce sites in the UK, and I quite enjoy ecommerce talk (I can't believe I just wrote that). Ping me an email if you would like to discuss on the phone!

@Ben - was replying thinking that we were passing in a Transaction object to Authorize now.

@Kevin - apols did not catch that.

@Ben - no worries, you are right that if a Transaction only represents a successful, money changing hands then it wouldn't be the place for a response type/status of authorization. But then again if that is what a Transaction represents, and Authorization only lets you know 'if' the payment would process (this is my very minimal knowledge of the terms, to me Authorization is basically a yes/no with some explanation and Capture is when you actually have money changing hands) then the result of the Authorization wouldnt even be a Transaction. I could be way off on this, you definately have more experience than me and I could be stumbling on semantics and lack of knowing the particulars of these types of processes (I take this as a WTF is wrong with me for not knowing and will spend some time looking at this hah).

@Kevin - I think there is a good chance that I am way off too! As we don't have Rob's code, I have made a bunch of assumptions about Transaction (because of it's name specifically). However, I did not assume Transaction is just for money changing hands. As I say above, it is also being used to "reserve" funds (Authorise) as you rightly point out.

We are all engaged in a discussion of the payment processing domain. Ultimately, it would be hard to get to the right answer in the comments of a blog (especially without the code and a clear idea of the requirements). However, it is still fun trying ;) Perhaps we should all get on a conference call!

I really hate the "is that exceptional" test for exceptions. I prefer to think of an exception as being a way for code to say "the caller or the system didn't fulfil the contract". Confusing? Let me break it down:

- The contract can be implicit or explicit, but basically the contract is your guaranteed behavior when you write that code. For example, when I call Payment.Authorize(cardno) I guarantee that the cardno is not null, and is fully numeric. If that (implicit) contract is broken, then Authorize should throw.

- Let's say that the (implicit) contract to Payment.Authorize guarantees it can access the network. I'm saying "assume I've checked network availability". In this case, Authorize should throw when that contract is broken.

- I (personally) do not guarantee that the cardno is valid. In fact, that's what I'm asking - "is this valid?" So do NOT throw if it's not valid.

As you see, in my world there's no hard and fast way to look at a snippet in isolation and say "this should throw" or the opposite. It's up to the system designer to say "the concern of checking network availability is over here, so everyone else throw when that contract is broken". However, when you look at it this way, you start to see some guidance.

- What's the primary responsibility or concern of the method? If the situation is outside the primary concern, it should probably throw.

- if you can't tell if this is the primary concern, perhaps the method or this part of the system should be refactored.

- if it is the primary concern, it should probably not throw, and the calling code should know what to do with the results.

Now, we no longer have "exceptional" conditions... we have return values within the responsibility of the method, and we have exceptions when we can not give a meaningful response within that single responsibility.

That's my preference, anyway.

@Ben - I hear what you are saying. But, is the safer approach to "continue if the exception is unknown" or "continue if the exception is known".

For example:

try

'--- Operations

catch ex As DataException

'--- Let's continue processing

catch ex as Exception

'--- Don't continue process, this could be any number of unknown errors.

...

end try

In layman's terms it is better expressed in how Phillip presented the question:

Integer:

Client: "Broker, I need $20.00 please."

Broker: NULL (no response)

Client then needs to determine what "0" represents.

Exception:

Client: "Broker, I need $20.00 please."

Broker: "Your balance is too low."

In which case it is 6 to one, 1/2 a dozen to another.

@Dave - I'm not totally getting what you are saying (think we may be crossing wires ;) However, what I am suggesting and what I think you are describing are not the same thing. I am suggesting this (in lay terms):

Client: "Broker, I need $20.00 please." (Request)

Broker: "Sorry, you cannot have that as there is no money left in the account" (Response)

Client: "Thanks for letting me know. I will tell my customer." (Client Code Action)

I am not suggesting Null is returned, which is what I think you are saying I am saying ;)

In respect to exceptions, I agree with Philip's last comments. In my opinion these types of payment calls will have totally valid failures that do not warrant throwing an exception. They warrant returning some results that we can act on (or not) in the client code. Ultimately, how the payment results are translated into Rob's domain is up to him. I am just suggesting one way (i.e. we have a response object that gets returned regardless of whether the operation was a "success").

Let me know if I have missed your point.

Sorry I didn't put my full name up earlier - don't want to seem as though I'm trying to be Phil Haack.

I agree with Ben on the last (especially since he agreed with me) - return something that can represent all of the possible values this code should return.

Imagine if Regex.Match threw an exception if the no match were found? Note how over the .net iterations, TryParse was added to lots of types because Parse throwing was not acceptable in many situations?

Not that I'm 100% I'm right here - but using my own guideline, it seems as though Authorize is expected to either Authorize or Not based on a valid card number. While I'd prefer to break that out into two or three separate operations (Is card valid? Is card authorized for x amount? Charge x amount.) I realize that many payment operations can't or shouldn't be broken out like that (Authorize probably one of them). If it does have to be one, I'd rather see those possible results as valid return values to be handled upstream.

@Ben and others - again thanks for the comments. This is really interesting! It seems to resolve down to this question:

Do I care WHY the authorization failed? To answer this one needs to consider what types of failure are possible...

1) Network failure

2) Service failure

3) User failure (no money, over limit)

The first one will throw no matter what - so that seems out of our hands and being good developers we'll let that bubble up the stack.

#2 happens a lot too. Your auth.net account might be inactive, you may use a wrong password, perhaps you're over your quota for the month let's say. If you use PayPal, this type of error could, literally, be anything and everything in between.

#3 is usually in the nature of "Declined" - rarely will they tell you the problem.

TO @Phillip's point I already have a validator on CreditCard that will check the card number and other things...

From this I can say that YES it's very important to know what happened - especially if it's a merchant error! How in the world would you ever divine this if you threw the whole time an Auth didn't work.

So - you've convinced me and THANK YOU for this exercise!

Talk about raining on our "should I throw an exception or return an object" parade... You can't just go and agree with me and some of the other people (but not all of them). That means no more commenting for us!

Look forward reading future updates and seeing the application (can I get on the TAP? ;).

Wait...what changed your mind?

"How in the world would you ever divine this if you threw the whole time an Auth didn't work"

By the kind of exception thrown...?