|
I try to follow the best practices of DRY (don't repeat yourself), "a method does one thing only" (whatever the acronym for that is), and good naming conventions. Sometimes, they conflict. For example, I have a method for a web service that verifies an authentication token something like this:
VerifyToken(IContext context, Guid token)
{
bool ok = token == session.AuthenticationToken;
if (!ok)
{
RespondWithError(context);
}
return ok;
}
So, this method actually does two things -- it verifies the token and calls the error response if authentication fails. Oops -- fails the "do one thing only" test, and not the best name. But why? Because in my web service handlers, of which I have several, I have things like:
Handler(IContext context, RequestData data)
{
if (VerifyToken(context, data.Token) {...happy response...}
}
The idea being, I didn't want to repeat myself with every handler, which would then look like this:
Handler(IContext context, RequestData data)
{
if (VerifyToken(context, data.Token)
{...happy response...}
else
{...not so happy response...}
}
ok, so one best practice suffers to meet more of the goals of the other best practice. And perhaps I'm taking DRY too far.
And of course, this bit me yesterday, because I couldn't figure out why I was getting all sorts of bizarre errors, like connection already closed, content type already set, bytes being sent differs from content length, etc. It was really quite random. Turns out, on a new service call I was adding, I had written the second form (because I forgot that VerifyToken handled a bad token response, and on a bad token, both VerifyToken and my service handler were trying to write to the response stream, and depending on the state, it was already closed, etc., being that the response is handled on a separate thread.
So to help me not make that mistake again, I renamed the method to VerifyAndRespondIfTokenBad. 
Then again, you might argue that the router should have handled the token validation for these particular service calls, and indeed, that would truly resolve the DRY issue, as the handler itself wouldn't even need to call VerifyToken as it should be guaranteed to be a valid request by the time it gets to the handler. And I agree, but that'll take more refactoring. 
None-the-less, I found it an interesting case study, and in particular, how when thinking through the best practices, while one can come to the ideal solution, various compromises are often made.
And if you are going to complain about how bad Guid authentication is, don't -- these calls are being made between two servers over HTTPS which are running my code, and the likelihood of someone guessing the Guid is pretty darn small, unless there's something I don't know, which is definitely possible!
Marc
Latest Article - Create a Dockerized Python Fiddle Web App
Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny
Artificial intelligence is the only remedy for natural stupidity. - CDP1802
|
|
|
|
|
You might find AOP (Aspect Oriented Programming) using something like PostSharp useful in cases like these.
modified 18-Jul-17 10:00am.
|
|
|
|
|
Jacquers wrote: ou might find AOP
True, but as a rule I avoid pre/post processors like PostSharp.
Marc
Latest Article - Create a Dockerized Python Fiddle Web App
Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny
Artificial intelligence is the only remedy for natural stupidity. - CDP1802
|
|
|
|
|
Any specific reason? Because it 'hides' logic?
|
|
|
|
|
Jacquers wrote: Any specific reason? Because it 'hides' logic?
In part, yes. I did a relatively deep dive into AOP, gads, 14 years ago, and basically came to the conclusion that I had to no good use cases for it. The vast majority of examples are related to logging which becomes irrelevant when you use a good messaging architecture which can centralize the logging. Other use cases were arcane, often dealing with post-production code changes, which I found to be an unappetizing solution (maybe it sounded better in 2003). Basically, in every case, I found that AOP is a bandaid where in reality, a better architecture would solve the problem, well, better.
So it surprises me when I read on their site that 10% of Fortune 500 companies use PostSharp -- none of their sample topics sways me with a "oh, this solves a real problem" reaction. Then again, if you have a good use case, I'd definitely love to hear about it!
Marc
Latest Article - Create a Dockerized Python Fiddle Web App
Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny
Artificial intelligence is the only remedy for natural stupidity. - CDP1802
|
|
|
|
|
It's been years since I used it, but it was for error logging 
|
|
|
|
|
Jacquers wrote: but it was for error logging
Hah!
Marc
Latest Article - Create a Dockerized Python Fiddle Web App
Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny
Artificial intelligence is the only remedy for natural stupidity. - CDP1802
|
|
|
|
|
Marc Clifton wrote: The vast majority of examples are related to logging which becomes irrelevant when you use a good messaging architecture which can centralize the logging
Perhaps we have a different definition of "logging" but what happens when the messaging service itself fails?
|
|
|
|
|
It is clear that your VerifyToken should only do that job. I would write the if and else block, because that is clean code. DRY has to step back.
Another solution is to overload the the VerifyToken function, but it can get weired. 
Press F1 for help or google it.
Greetings from Germany
|
|
|
|
|
KarstenK wrote: It is clear that your VerifyToken should only do that job. I would write the if and else block, because that is clean code. DRY has to step back.
On that I agree as well, though I much rather like the idea that all these requests go to a specific pre-route handler that does the validation. 
Marc
Latest Article - Create a Dockerized Python Fiddle Web App
Learning to code with python is like learning to swim with those little arm floaties. It gives you undeserved confidence and will eventually drown you. - DangerBunny
Artificial intelligence is the only remedy for natural stupidity. - CDP1802
|
|
|
|
|
Name it VerifyTokenX and you are out of Problem
Bruno
modified 19-Jan-21 21:04pm.
|
|
|
|
|
I'm all for the SRP (Single Responsibility Principle, now you know the acronym ), but you can take it too far.
Like my former "technical director" who heard about it from me and then told me how to implement it.
The result was a whole lot of single method classes that, I'll give him that, did only one thing
At some point in your code you just can't help but doing multiple things.
The only thing you can do is to minimize it and when you do multiple things at least make it things that belong together, like checking authentication and returning a response.
It's like "purely functional" languages, like Haskell. At some point they have to have side-effects, but they wrap it in a single function and keep the rest of the application "pure".
|
|
|
|
|
Hmm. For my money, I'd prioritize DRY (Don't Repeat Yourself) and SR (single responsibility) based on which option resulted in less code.
If you've got 100 discrete calls to VerifyToken() that you'd have to add else { error-handling-stuff; } to, I'd leave the error handling where it is.
On the other hand, if you've only got a couple calls to VerifyToken(), but the error handling is different for each case, then obviously you don't want it inside the function.
Software Zen: delete this;
|
|
|
|
|
It's understandable that you wrote VerifyToken that way. Seems logical and "Clean" - you're not repeating yourself. But at the cost of introducing a dependency on 'IContext' in VerifyToken that has nothing to do with it's job. So, in my way of thinking, that's not "Clean". Consequently you can repeat yourself or find another way, maybe more correct, to report the error.
Another option is to refactor the code a bit. Have one function VerifyToken() and have another VerfyTokenAndRespondIfBad(). The second would call the first.
#SupportHeForShe
Government can give you nothing but what it takes from somebody else. A government big enough to give you everything you want is big enough to take everything you've got, including your freedom.-Ezra Taft Benson
You must accept 1 of 2 basic premises: Either we are alone in the universe or we are not alone. Either way, the implications are staggering!-Wernher von Braun
|
|
|
|
|
Just throw an exception if the token validation fails, catch it and send your error response in the catch block.
simples 
Your VerifyToken() method at the very least should be renamed RespondWithErrorOnInvalidToken() - which will avoid the mistake you made...
PooperPig - Coming Soon
|
|
|
|
|
Call Accenture in for a consultation.
Aren't they the experts in Best Practices that management relies on?  
|
|
|
|
|
Marc Clifton wrote: "a method does one thing only" (whatever the acronym for that is)
There's no Acronym for it I know of, it's simply the "Single Responsibility" principle and thus the S in the SOLID principles of object oriented programming. It goes for the whole class though not just for the method itself. It's essentailly the UNIX philosphy[^] either way.
In your special case you really might want to look into the "OData and Authentication" Articles [^] to give you a slightly different approach into authentication of some sorts or rather: a different place to hook up your authentication (assuming that's what you want to use the token for).
But coming back to the topic, there's many priciples like DRY, KISS, YAGNI, SOLID and they all make sense and are perfectly clear when it comes to a clean and maintainable program that's rather self-documenting. And it is definitely worth it to stay true to these priciples for as long as you can. They are however still principles and not rules and you might not always, or rather can't always follow them as closely as you might want to. Especially if you factor in time constraints you usually have if you develop an application in a business environment.
In the end it is a real shame, since they exist for a reason. 
|
|
|
|
|
|
I call time travel shenanigans
|
|
|
|
|
There meant to be.
If you do it, you love it. If you love it, put a pride with it (DON!).
|
|
|
|
|
Met my doppleganger. Too bad for him only one Most Handsome Man in the World can exist.   
|
|
|
|
|
|
Don't worry - the UK still hasn't fully converted either.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Bottles hold ml, beer comes in pints.
Food is sold by the Kg, but people are weighed in stones and pounds.
We buy cloth by the m, but drive by miles.
That's pretty much metric, right there - Human stuff in human units, manufactured stuff in metric (except some of the beer, obviously).
And when will the world adopt the metric DateTime?
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
AntiTwitter: @DalekDave is now a follower!
|
|
|
|
|
OriginalGriff wrote: Food is sold by the Kg,
But everyone over 35 still complains about that, and has to mentally convert it to pounds and ounces.
OriginalGriff wrote: metric DateTime?
Obligatory xkcd[^].
But it still takes nine paragraphs to explain the difference between "12:00 AM" and "12:00 PM":
12-hour clock - Confusion at noon and midnight - Wikipedia[^]
Maybe we should all switch to decimal time[^]?
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
