When Worlds (or best practices) Collide
-
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
-
Jacquers wrote:
Any specific reason? Because it 'hides' logic?
In part, yes. I did a [relatively deep dive](https://www.codeproject.com/Articles/4039/Aspect-Oriented-Programming-Aspect-Oriented-Softwa) 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
-
Jacquers wrote:
but it was for error logging
Hah! :laugh: :laugh: 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
-
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
VerifyTokenhandled a bad token response, and on a bad token, bothVerifyTokenand 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 toVerifyAndRespondIfTokenBad. :sigh: 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 callVerifyTokenas 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 thr -
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
VerifyTokenhandled a bad token response, and on a bad token, bothVerifyTokenand 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 toVerifyAndRespondIfTokenBad. :sigh: 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 callVerifyTokenas 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 thrI'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 :sigh: 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".
Best, Sander arrgh.js - Bringing LINQ to JavaScript SQL Server for C# Developers Succinctly Object-Oriented Programming in C# Succinctly
-
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
VerifyTokenhandled a bad token response, and on a bad token, bothVerifyTokenand 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 toVerifyAndRespondIfTokenBad. :sigh: 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 callVerifyTokenas 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 thrHmm. 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 addelse { 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 toVerifyToken(), but the error handling is different for each case, then obviously you don't want it inside the function.Software Zen:
delete this; -
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
VerifyTokenhandled a bad token response, and on a bad token, bothVerifyTokenand 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 toVerifyAndRespondIfTokenBad. :sigh: 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 callVerifyTokenas 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 thrIt'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
-
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
VerifyTokenhandled a bad token response, and on a bad token, bothVerifyTokenand 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 toVerifyAndRespondIfTokenBad. :sigh: 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 callVerifyTokenas 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 thrJust 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
-
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
VerifyTokenhandled a bad token response, and on a bad token, bothVerifyTokenand 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 toVerifyAndRespondIfTokenBad. :sigh: 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 callVerifyTokenas 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 thrCall Accenture in for a consultation. Aren't they the experts in Best Practices that management relies on? :laugh: ;P
-
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
VerifyTokenhandled a bad token response, and on a bad token, bothVerifyTokenand 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 toVerifyAndRespondIfTokenBad. :sigh: 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 callVerifyTokenas 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 thrMarc 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. :doh:
-
Jacquers wrote:
Any specific reason? Because it 'hides' logic?
In part, yes. I did a [relatively deep dive](https://www.codeproject.com/Articles/4039/Aspect-Oriented-Programming-Aspect-Oriented-Softwa) 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
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?
