Code Review & Refactoring to a better design

it's code review and design time I'm Derek Martin from codepopinion.com and recently I reviewed a video that was talking about clean architecture cqrs and what I thought was a lot of unneeded indirection but also in that video there are some API design that I didn't really love around nullables and throwing exceptions and neither did you based on the comments so here are my thoughts around that design some refactoring what I think it should look like but more importantly why I want to thank event store for sponsoring this video event store DB is a new category of operational database built for event sourcing cqrs and event driven microservices for more on eventstoryb check out the link in the description so here's the original design from that code review I did I'm going to point some things out and dive a little bit deeper into them and provide some refactorings of what I think it should look like so we have this mediator query where we're injecting this I order read Service the first thing we're doing in the Handler is calling that I order read Service we're calling get order by IDE async so let's take a look at it first thing to notice here is that it's returning a nullable so we could possibly return null when we call this if we look at the implementation we're just using our DB context we're fetching our data out but the important part here is we're calling first or default async so if there are more than one record we're just going to get the first one that we run into otherwise if there are none your returning default which is going to be null so if I jump back over to this code because we're possibly returning null we have our null check we're throwing an order not found exception if it is null now as you'd expect we probably are using this query in mediator let's say from something top level and asp.net core is the example and if that's the case we then now need to catch this order not found exception so that we can return something more meaningful in HTTP back to our client possibly like a 400 or a 404. now the first thing I want to touch on are really expectations specifically with that data access it was calling first or default async now first or default let's touch on the first part of it meaning that we have some expectation that maybe there's more than one order ID but we probably know that can exist in our database so that really potentially could be masking a problem if there are more than one record Now the default part is meaning that oh maybe that order ID that's been passed doesn't even exist and that's where it comes from expectations now I'm not talking about this was called directly from a public API this was kind of buried within layers of indirection in my opinion but the point is is that we're using null as a way of specifying okay this record doesn't exist but I still go back to well why would the record not exist at what point if our app is the caller of that method of that service why would we be passing an ID that does not exist so can we kind of remove all this concern about nullables about more than one record why wouldn't we just just call single so if the expectation is that the caller is going to pass an order ID that actually exists and then there's only going to be one of them because we know that based on our schema we can just call single async now I can get rid of the nullable here and keep going up here remove it from our interface now I need to backtrack through all this interaction which we know that in our Handler this is never going to be null so that way we can just remove this entirely I could actually even get rid of this and just return this and we're going to backtrack a little bit more and actually go into the controller because we can see where we're actually calling detox detail here and sending our requests with mediator this is also not possible anymore because we're not throwing this exception so therefore this all can get removed now the thing is is that if you do pass an order ID that does not exist that singular a single async is going to throw an invalid operation exception and that would bubble up all the way and if you have a custom air handler you'd probably get some type of 500 that you'd return now you may say okay well that's not good but really the only way that's going to happen because this is MVC and being rendered server side is that the way to get to that is via a link and we're the ones producing the link in our actual page so really the only way this is possible is if somebody were actually changed the URI change the ID in our route to actually produce that exception which then would result in a 500. now a lot of this comes down to what type of app are you building is this something just internally in your company where somebody was changing the URI manually and ultimately that throw an invalid operation exception and then a 500 is that really a big deal you know that the input how you're directing people is going to be valid they have to be going out of their way to produce the 500 but maybe it is an issue and you do want to return a 404 and we can do this without throwing or returning nulls and one option we have for this is an option an option type so I've added one here to our interface and we're saying okay now instead of returning a nullable response or just a response it's been going to be an option or a response I've changed our definition here and what I've converted now is I'm saying it's single or default async because I'm going to want internally this to possibly return null now what we're doing is we're checking that we're saying okay if it is null then I'm just returning an option of none that's kind of the representation of it otherwise if there is a value we're saying it's sum if I go back through all this wonderful not really wonderful interaction you'll see that now what I'm doing is I've changed the signature of this query Handler to do the same thing I may be returning a response so I'm representing that as an option so I can just send that back the entire way now if I go back up to our controller now we can handle this so that we can pass it to our view or return a not found so the way that looks is we're getting our response here our view model it's an option we can call match on this API we're saying hey we're returning an action I action result it's either going to be if there is a result if there is some let's pass it to our view if there's nothing if it's none then we're going to return are not found so I've done this all by just using an option type I'm not dealing with null and I'm not throwing exceptions now whether you choose to create an API that's returning nullables or an option type like this I think generally your thought is that you're being defensive because you don't want bad inputs and if you're going to be defensive likely you end up with doing this everywhere and you have to handle if it's null manually you'll have to deal with it otherwise with an option type it's kind of forced upon you but you have to do this through all your different layers or abstractions all this interaction you potentially have to handle it now again context matters are you dealing with a public API what kind of response do you need to return a 400 or 404 could be very valid for a public API where somebody's consuming that isn't used some third party that'd be really helpful so likely you'd want to do it but again if it's some type of private internal API and you're expecting valid values you can go through kind of save yourself a lot of headache and not have that expectation that somebody's going to pass in random data if you were to call single and it were to throw an invalid operation exception and bubble all the way up to the top layer and let's say this was some HP API and that returned to 500 and you logged it you'd probably like to want to know that was happening because that's really unusual what would you possibly return back to the client to say hey that wasn't useful you should do something with it if you return them back a 400 at that point or a 404 they're like great okay what do I do with this you'd want to know that something's gone bad it's the exact same thing as the illustration at the very beginning where it was calling first or default at no point would I ever want to do that because it's really masking a problem that there's duplicate records what I'm not expecting it so a lot of this comes down to your contacts and expectations now as for throwing exceptions in the original example I removed it's throwing order not found exception I think the original idea there was to be specific about what was actually occurring the problem is it's not explicit at all anytime you're throwing an exception you have no idea from a caller's point of view that that actually might happen so from our controller we are calling mediator and that's where that exception was being thrown but we need to know that which we would then have to look at the implementation of the Handler to see that and then we're using that try catch as really as control flow so that we can return a not found the exception is not explicit the return type is if you found topics like this interesting and you have your own thoughts or opinions you can join my channel and get access to a private Discord server where you can chat with other software developers about topics like this check the link in the description on how to join if you found this video helpful please give it a thumbs up if you have any other thoughts or questions make sure to leave a comment and please subscribe for more videos on software architecture and design thanks

Channel: CodeOpinion

Views: 10,812

Keywords: code review, software architecture, software design, design patterns, software architect, programming, microservice architecture, code review best practices, code review example, code reveiw best practices, code reveiw in sofware engeneering, code review exemple, code review interview google, code review process, code review in sofware engineering, code review in software engeneering, code reveiw interview questions, code reveiw process, code reveiw proccess

Id: ka0Ag98_o_g

Length: 9min 42sec (582 seconds)

Published: Thu Jun 22 2023