So You Want To Be A Code Reviewer ? #7

[@hansrudolf5849]

Already looking forward to your next Video Shiv! I am a big fan of this particular series .. :)

[@Matlus]

Thank you Hans and glad to hear. Sometime I don't know what folks would like to see. This subject is very near and dear to my heart and I spend a lot of time reviewing code and coaching my team.

[@robertslazda8581]

Couple hours ago, was wondering if Shiv is going to post, and alas - the video is here on schedule. Sweet. Now my monkey brain is going to crave for another next sunday. Keep it up, Shiv, your awesome!

[@Matlus]

Thank you Roberts! At least someone is watching!!

[@saurabhchauhan232]

I had simillar feeling when I saw, I was so happy but as you said will crave for another sunday 😁😁

[@Matlus]

@@saurabhchauhan232 Thank you Saurabh! I guess the pressure is on :).

[@hansrudolf5849]

@@Matlus I would also gladly donate to make you keep producing those sweet vidoes .. ;)

[@Matlus]

@@hansrudolf5849 Thank you for your support! I've been thinking about this since many folks have suggested and asked for it.

[@VinuP2023]

Thank you Shiv. Thank you so much for posting vidoes regularly 🙏☺️

[@Matlus]

Thank you Vinay! Thank you for your support and inspiration.

[@MrJonnis13]

Thank you Shiv for this great series. Can you please justify a little bit more about passing an inner exception, giving a real world scenario ? Apart from the fact that you don't want to lose the initial reason of failing code and logging that reason. I mean more from the user perspective that uses the system

[@Matlus]

Thank you for you comment Johnny and your question. The reason is exactly what you said. You never want to loose the original exception. That is, the exception type, the message the stack trace and any inner exceptions it has. This way in case you need to you can always go all the way to the source of the origination of the problem. This is extremely important for any TCP socket related exceptions (Http, GRPC, Database, Message Brokers etc.) because they invariably have inner exceptions and the very first exception message is the most relevant (and sometime the most meaningful) one.

[@mustafaelshobaky6881]

Thanks Shiv , very informative. Maybe, you can try to add to the video title an informative name about the content to help others reach it.

[@Matlus]

Do you essentially change the title? or instead of #1, #2, #3 etc. to have text that includes the content? I can see that the title itself is not good enough (or interesting enough) to entice folks to watch. I feel like I need someone dedicated to coming up with good names and tags for my videos!! :)

[@mustafaelshobaky6881]

@@Matlus you have so much information about different things in the same videos which makes it hard to include all the content in the title. However, i guess you can add main topics like "handling exceptions" in the title and just paste the comments about the fixes, that you should in the video , in the description. I actually like that you're opening alot of areas in the same video. It feels like a private chat with a professional person like you.

[@Matlus]

@@mustafaelshobaky6881 Thank you for your well thought out comment Mustafa, appreciate you taking the time to give me feedback on ways I can improve as well as your appreciation for the effort I put in. I'll start to include the "highlights" in the title moving forward. then go back and change the titles of the earlier ones.

[@saurabhchauhan232]

Would be nice , if you share that file on git by single file hosting don't know how exactly but seen people doing it. Thanks for the video and all your efforts. Looking forward for next Sunday 😊

[@Matlus]

I've updated the description of the video to contain links to the original code and the fixed code

[@saurabhchauhan232]

@@Matlus thanks

[@martonbalassa8128]

Can you elaborate more on why nested try-catch-finally blocks or a try-catch inside a using block is bad? I agree that the using should be only wrapping the line where the stream is actually used, but otherwise I'd find it's expressing power more important than the small penalty of nesting.

[@Matlus]

Primarily the nesting itself it the issue :). Why nest when it's not required. Also from a code point of view, there is a next too. A try-catch nested inside a using statement. In C# 8 we now have using expressions (no need for a using statement block) and luckily the lowering of that code does the correct thing (that is if there is a try-catch) then the lowered code will translate to a try-catch-finally rather than a try-catch nested inside a try-finally. I think nested code is hard to read. So at what point would you say that nesting is not ok at this point? my proposal works in all cases. That is as soon as there is nesting, avoid it because otherwise you'd have to say something like: ok 2 nested using statements is bad or 2 try-catches inside a using is bad or a limit some sort account for all permutations. I'm not against the using statement. I'm not against using multiple using statements one on top of the other with just one block (open, close curly). But the moment you go to a nesting, I'd say convert that to a try-finally or a try-catch-finally as the case maybe because the next coder that needs to add another using and a try-catch will simply follow what's there without thought (most times) and I've seen code rot in this fashion. Plus if you do this everywhere in your system and the codebase is large you will start to pay a performance penalty.

[@martonbalassa8128]

@@Matlus thanks for that, I haven't really considered a using statement instead of the block. I think what bothered me is the idea of manually calling Dispose() - something I rarely ever do because I'm used to the using blocks.

[@Matlus]

@@martonbalassa8128 cool! There is nothing wrong with calling Dispose manually and in fact one needs to do it more often than not because it's not always correct use, "using"). Many times, you create an instance of a disposable thing but you should keep it alive (as a member variable rather than a local variable) because that's the correct way to use it. For example, HttpClient, Database connections, most of Azure SDK "things", Message Broker connections, etc.

[@martonbalassa8128]

Well yes, long living and costly objects are better 'owned' (for those who used Delphi ;) ), but file streams and temporary/pooled objects just feel natural to be used. Maybe C# should have an abuse keyword as well :D

[@Matlus]

@@martonbalassa8128 so you're a fellow Delphian? How cool!. Yes, Streams are one of the few disposables that get "used". But eve there, most times I return stream from methods, so the call site is really the one "using" them. I think Delphi/C++ programmers have a better gasp of disposing (when, when not to, how etc.). Anytime I've working with a new type, the first thing I do is see if its disposable.

[@obiwanjacobi]

Solution: I would suggest not waiting for an exception to determine if the passed-in file exists or not. There is a perfectly fine method for that on the File class. Don't use exceptions as flow control. [2c]

[@Matlus]

Marc, I'm not sure what part you're referring to exactly. But on your comment of "Exception for flow control". I beg to differ. This is not a case of using exceptions for flow control. I'm aware of how to use exceptions correctly :) and I don't ever use them for flow control. Some folks think I do but I don't think they know what that is. Just using exceptions (throwing your own) translates to using exceptions for flow control in their minds. Of course I don't remember the code in this video and I don't really know what part your comment on so I could be wrong.

[@obiwanjacobi]

@@Matlus @27:26 there is a catch with 3 types of exceptions. The first is an IOException and reports a message about the file not being found. My point was that you could prevent that exception from occurring by using the File.Exists method. In the mean time I've seen your video on exceptions and understand your reasoning - and I do agree with that for a large part (using exceptions cuts down on the need for conditionals). I think throwing an exception when File.Exists returns false, reads (and behaves) better than waiting for an IOException - which could also mean other things (perhaps the file was locked?). There is a FileNotFoundException which would at least not 'change' the meaning...

[@Matlus]

@@obiwanjacobi Ok, thanks for the time coed and context. Ok, let me explain how I design/implement my systems. Validations are done at the entry point. From that point forward, there is no invalid data. This class in question is so deep in the system (so far removed from where the validations are done) that it has no need to worry about bad data. And if there is bad data, its not the class' problem nor its responsibility. Or are you going to return a bool or some other way of indicating success/failure? I just don't program like that. Action methods do things, but return nothing (1 anomaly exists) . Query method must return what they claim or not return at all. I talk about all of this in my "Programming With Intent - Method design" video. If I have a try-catch anywhere in the system (and not at the entry-point) it is to translate exceptions to business related exceptions. Exceptions are just that, exceptional. They are not expected to occur. This code here is also one of those. That is, exceptions are not expected to occur, but if they do, throw and let that exception fly all the up the stack. Hope this makes sense?

[@obiwanjacobi]

@@Matlus Yes, I understand your reasoning, but you also don't want to mislead the dev when it does go off. Currently you could throw a domain/business exception with the message the file was not found, with an inner IOException that the file was locked. That is misleading IMO - there is no worse info than wrong info. Either change the message to a more generic statement (look at the inner for more details) or the complexity of exception translation goes up - something you'd want to avoid I imagine... I am not saying I have the answer, just trying to point out a problem.

[@Matlus]

@@obiwanjacobi I get your point. However the inner exception is there for exactly that reason. The message in the domain exception is indicative of the reason for why the exception could occur (that is, the file may not actually be there in the real world, in our scenario). So it's either there or not there). We don't account for all possible scenarios. Now if in production if at any time we find our exception messages to not be clear or don't provide information that could have been provided to better direct us in the right direction, the first thing we'll do is fix the messaging. I'm a big stickler for really good exception messages.