Clean Code: Learn to write clean, maintainable and robust code

okay guys I found this code on one of Facebook groups called a speedo pet and it immediately got my attention because I can see quite a few number of problems here so in this video I'm going to show you how to fix these problems and how to write cleaner more maintainable more robust and testable code I'm guessing this code is for a web form that might potentially look something like this don't worry about this stuff happening here I'm guessing on that web form we should have a text box where the user types the name of a book and a button which I'm guessing when it's clicked it will display a message saying the book is in the stock or not I actually didn't see the web form all I saw was this code behind file here so let's read this code line by line and see exactly what's happening here before we can start improving it first we are declaring a quantity setting it to zero next we are declaring a secret connection and then a data reader finally a query which is trying to select the quantity of a book from book detail table and it's filtering that based on the book name which is entered into the text box finally we are instantiating a sequel command opening the connection to the database executing the command and reading the result in the reader we access the first column which should be an in 32 or an integer which represents the quantity if the quantity is zero we display a message saying book is not in the stock otherwise we display a message saying book is issued which I'm not quite sure what it means and finally we close the data reader and the connection so the first problem I say in this code is poor naming convention look at this bottom one here what does one mean how is it different from button two or button three I can tell without looking at all the code and figuring out what does one represent so I'm guessing if that's our button one that's trying to check the quantity or availability of a product in the stock it would be much better if we name this button something like instead of button one BTN check quantity it's more meaningful now I click this button here to show the events and see it automatically renamed that method which is oops going to work I don't know let's double click that okay so I'm going to move all of this code here into this method and get rid of this so now instead of saying button one click we have BTN check availability click which is very meaningful same for this a QT y here now it's kind of obvious that it's quantity but a QT Y is not a very beautiful and meaningful name for variable Y a quantity Y not just quantity and instead of QT y writing the actual word quantity it would be much cleaner so I'm going to rename that but see how I rename these I'm not going to go and rename it like this why because see these are references to that variable and if I manually rename it here I have to go here copy this and then paste it here and who knows how many references you have here this is very slow and really not how a good coder renames variables a better way is you use the refactor rename command of visual studio so in visual studio if you press ctrl R and R you're going to get a dialog box which actually is not appearing here because I've got a plug-in installed called resharper but even if you don't have resharpen you can still use visual studio to rename variables basically when you use that dialog box you rename a variable and visual studio automatically renamed all the references to that variable so you don't have to manually rename each one now here with resharper you see this little box here and resharper tries to find a good name but in this case it's not intelligent enough to suggest the name quantity it's interesting in 32 which is not really a good variable name so I'm going to go ahead and just rename that and inter see it automatically updated the quantity variable here so remember ctrl R and R I believe another shortcut in visual studio is f2 so try it and you'll love it similarly this cien cien is not a good variable name try to avoid acronyms and use full meaningful names for your variables so I'm going to rename this again ctrl R and R and see this time resharper realize that this is for a single connection and the variable names is suggesting is connection or a sequel connection my preference is a shorter variable name so I'm just going to select the first one and resharper automatically updated all the references to that variable here same here what does dr1 represent how is it different from dr - i don't know so let's just rename that to reader okay and here s again s is not a good variable name this is a query right so let's just rename this to query and instead of CMD 1 I'm going to use command there you go and that's it I think yeah the code is kind of more readable now there's just one more issue here and that's textbox 3 again as a reader of this code when I look at this I don't know what's the difference between textbox 3 and textbox to or if there is a textbox to on the form at all so I have to look at the whole code to figure out that textbox 3 is where the user types in the name of a book so it would be much better if we this to txd book name instead of textbox 3 so what I'm going to do is I'm going to again use resharper and rename that to txt book name press Enter resharper automatically realize that that object is used somewhere else in this file default.aspx designer which is the code that Visual Studio automatically generates for each of your web forms so I've selected that is selected by default and I'm going to let be sharp red to rename that for me now let's save everything if I go to that designer file here you see that resharper automatically renamed this for me that's why I say always use Visual Studio or resharper for renaming your variables let's go to default and this is the textbox I select the properties here and here here it's not renamed I don't know why resharper cannot rename the objects on a webform like this so I have to manually change that to txt book name alright all good get back to our code behind next problem I see is this reader is declared here but then it's used here it's best to declare a variable closer or closest to where it's being used this reduces the noise in your code see as a developer reading this code I see you're telling me that okay we have quantity we have connection we have a reader and then you kind of forget this to move on and go somewhere else then you talk about query and then command and connection open so what happened to the reader see it kind of creates a mental disconnect it would be much better if I move this line of code here just where we need the sequel reader but declare it and use it straight away and see the court is also one line shorter alright another thing we can do here is using the VAR keyword in C sharp with var you can write cleaner and shorter code let me show you how it works so here instead of using C code connection I can just use the keyword var and the benefit of that is C with this I'm saying signal connection connection is new cycle connection so I'm repeating this word twice here all right we can just use var to write less code and it's cleaner and less noisy same here we are saying cycle command and again new cycle command so one is enough to have it here and we can use var here and same for reader see the code is getting cleaner with every step okay another problem I see is this last line here connection that closed even though the writer of this code had good intention and they wanted to make sure that the connection to the database would be closed at the end but unfortunately this code is not very robust why well see we open the connection here and then here we are trying to read something from the database now if something goes wrong in between for any reason for example maybe there was a bug in your query or maybe the value that was returned it wasn't an integer you know anything can go wrong in that case if something is wrong an exception will be thrown which means anything after this line will not be executed so there is no way to guarantee that the connection will be closed how we can resolve this we use try finally block let me show you how it works so what I'm going to do is once the connection is open I can define a try finally bad means type in try F this is a code snippet so you write a little bit of code and then let Visual Studio generate more code for you so try F and then press tab see it automatically creates a try finally block for you I have a course on udemy called visual studio tips and tricks or double your coding speed where I cover a lot of tips on how to write code faster and if you're interested you can look at more code snippets and other ways to write code faster so let's move on I'm going to move all of this stuff here into the try block and then I want to make sure that the reader and connection are closed no matter what and that's the intention of the finally so if an exemption is thrown here no matter what this finally block will always be executed all right now we have an error here reader because reader is defined in this try block but we are using it in the finally the problem is when we define it here in the try the scope is applicable only here so it has no meaning and also in that block which means we have to take this line out of the try block so I'm going to put it here alright let's look at the next problem in this code look at this bit here we are trying to construct a dynamic sequel query based on what the user put in the text box this is very dangerous because there is a security attack called sequel injection which basically means a hacker can go and type in some weird text in your text boxes and that would translate to some query that will be executed on your database let me show you what I mean by that imagine if I go into text box and type in something like this so ignore this comment bit here just look at this bit I'm writing so if I type exactly this into the text box see what is going to happen well this code is trying to concatenate this string with what comes from the text box right so let's just simulate that here I'm going to copy this bit here and then I'm going to copy what the hacker puts in the text box and finally um this bit here so another actually I can don't worry about it I don't want to confuse you here but just for demonstration see with that input in the text box the query will end up looking something like this so we're selecting quantity from book detail table where book name is empty string or one is one well one is one is always true which means this query will be somehow executed don't worry about what it's returned but here is a tricky part with semicolon we can batch queries which means this would be also executed on your database which means if the hacker puts that string in the text box they can easily drop one of your tables using a similar technique they can go ahead and write a query that would create an administrator account on your database something like this inserting two members or users you know with some values blah blah blah now they it also requires a little bit of intelligence and guessing the scheme of your database I'm not a hacking expert but I know there are ways for them to even extract the scheme of your database know exactly what you what tables you have what columns you have and based on that they can construct a query that will take that will give them the complete control of your application and potentially your server depending on the permissions so the lesson here is never ever ever trust what the user puts in a text box and based on that just go ahead and create a query the way to resolve this is to use parameterised queries which means instead of concatenating strings here we're going to change our query to something more kind of static which would be like this so this is a parameter that is prefixed with the @ sign so what the user puts in them text box will end up being as a parameter here and this is the actual query so nothing else will be executed on the database so if I go and put some weird string here that will go here and when sequel server runs that query it doesn't see this text as yet another query so this is an actual query that will be executed now in order to use parameter S queries the first thing you need to do is define a parameter in your query and then you need to add that parameter in your command object so here after I define my command I'm going to go ahead and say command the parameters that add now the parameters to the add method you can define a parameter name the DB type and size and column or I would say this is the best overload so you pass a sequel parameter object so here I'm going to create a sequel parameter object and give it a name the name should be exactly what I put here book name and a value which would be what we put into the text box so that was txd book name the text right so with this we can get rid of our sequel injection attack and our code will be more secure let me get rid of this command comments here okay let's see another way to improve this code see this method here between check availability is doing too many things is trying to access the database here and read the quantity and then it's trying to show M message to the user this bit accessing the database should not be the responsibility of a method in your code behind file your code behind file belongs to your web form this web form and the web form is in the presentation layer of your software which is what the user sees this bit here is about data access so data access code should not be in the presentation layer so what I'm going to do is first of all I'm going to move all of this code into a separate method just to show you a step by step refactoring technique so basically what we need is a method that gets as a parameter or argument the name of a book and then go into database and return the quantity right that code should not know nothing about what is going to be displayed in the user interface like this response to write all it needs to know is how to get the data from the database and so let me go ahead here when I declare it as private and I wanted to return the quantity which been which would be an integer and I'm going to call this method get product quantity from stock as it sorry stock and as the parameter I'm going to define a string which is book name and C I'm creating methods that have meaningful names and meaningful parameters anyone looking at this code can immediately tell what this method is doing without going and reading every line of this method and that's the key you should read code that is meaningful like a like a story like a newspaper like a book you don't have to a developer does not have to read every line of your code to know what's happening so what I'm going to do is I'm going to kind of well copy everything here into this method now this method as I said is about data access so it should not know anything about writing something on the web form so basically once we read the quantity it did we don't need to know anything else finally we close the reader and the connection and just return the quantity right so let's review again this method creates a connection query command opens the connection risk the data from the database and return it it has one single responsibility it's not doing too many things then I can go ahead and refactor this code and instead of going to the database and doing everything here I can ask this method to get the data for me and then if the quantity is zero I can just display this message otherwise this message so let's do that I'm going to define a var quantity and ask this method get product quantity from stock and then just pass the ticks t book name the text and at this point we've got the quantity it's either zero or not if it's zero we're going to display this message otherwise this message and we can get rid of everything else here so let me clean up is cold see now this method is much cleaner it has only a few lines of code and its responsibility is purely to display a proper message to the user and this code has also few lines of code well it's a little bit more than my personal tastes and I will show you shortly how we can even make this a little bit shorter but it has only one single responsibility and that is to get the data from the database okay let's look at this code again this query we can just grab this and put that here where it's used because this is the only place we have used that query and we don't necessarily need to store it in a separate very so just put that here and get rid of this extra variable here all right I don't things we can do to improve this code is this hard-coded connection string here it's not a good thing why because if the connection string changes we have to go here and change it here and recompile the application now you might wonder when the connection string changes well it's quite possible that in real-world scenarios you have a development server a test server and a production server and each of these servers require a different connection string you cannot recompile your application every time you want to deploy to one of these servers so a better way is to use a configuration file where you can store the connection strings for each environment and then your source code would just read the data from the configuration file in the case of this web forms application we're going to store the connection string in web config so I'm going to open up web config here under here at the beginning just after configuration and config sections you see this connection strings so I can define a connection string here use the add give you the name let's call it I don't know book store for example store and the value sorry the connection string would be exactly what we put here so I'm going to cut that and paste that here now this is just an XML doesn't require compilation and when we deployed this application to each environment but it's dev test or production we will have a separate web config for that environment and the application code will be exactly the same now how can we read the data from connection string we use the configuration manager class like this so I type in configuration manager now I'm using resharper and resharper automatically knows that this class is in a namespace called system that configuration so I can just hit enter and resharper automatically at that namespace here for me if you are not using resharper you probably have to type it here and Visual Studio doesn't recognize that then you right-click and there will be some menu on the top here that is called resolve namespace something like that I strongly recommend you to install resharper because it makes your coding a lot easier alright let's go ahead so configuration manager is a class I'm going to go to app sorry - connection strings and this has an indexer I can specify the name of my connection string which is in this case bookstore and that returns a connection string object which is equivalent to this object here so that object has a property called connection string which is the actual string here so in order to get that we come back here we get the connection string object and then we access the connection string property so this is what we'll put here in the sequel connection I would argue that it would be also cleaner to instead of writing this raw query here you use a stored procedure for example you would have a stored procedure called get quantity which gets an input which is the name of the book and returns the quantity I'm going to leave that exercise to you because I want to talk about more fundamental issues in this course so let's see what else we can do to improve this code one thing that just got my attention is this book name we declared it what we never used it how do I know well resharper is kind of graying out this for me look this is white and this is gray which makes me think this is never used so when I look here at the parameter I can see that it's still using the txt book name I don't want this method to be dependent on that text box because shortly I will show you that I'm going to move this somewhere else so let's just use book name here and see now it became white earlier I told you that the data access code should not be in the presentation layer which means this code does not belong to this code behind file for this webpage what we can do is to move this to a separate class let's say we're going to create a class called stock that would be responsible for getting this doc or updating the stock from the database now there are various ways to model that and create layers for your application but it's going to beyond the scope of this video because this code is I think for beginner level dotnet developer and I don't want to confuse you with too much complexity so what I would suggest is at a minimum go to your project here right click add a new folder call it maybe services so here I'm going to create a new class add so add a class I'm going to call this stock service and then I'm going to move this code here into that class let me also show you a quick shortcut here see I can go and use the mouse to select this and cut and paste or there is a quicker way so anywhere here I can press ctrl + M twice the method gets collapsed now I can press ctrl X and then I can press ctrl + tab and this shows all the open windows in my visual studio project so I'm going to go to stock service and then I'll come here and then control V to paste the code right it's much easier when you use your keyboard again in my visual studio course on udemy I talked about all these things so if you want to learn how to write code fast you can take my course and get a lot out of it now when I paste this code resharper is automatically detecting that mystical connection here is in the system that data the C code client namespace which is not added here so I'm just going to click this or press alt and enter as the resharper suggests so there you go and similarly configuration manager is in system that configuration namespace which does not exist here again I'd rely on resharper to automatically fix this for me so the shortcut is Alt + Enter there you go which means now I can go back to my code behind file again I'm going to use ctrl + tab go here see this class is much cleaner now now this method does not exist in this class anymore and it's in our stock service so we need a reference to the stock service so what I'm going to do is I'm going to create a private field here stock service give it a meaningful name now private fields will always be prefixed with an underscore and Siri sharp rate is automatically suggesting three names for me I'm thinking stock service is more meaningful in this case and then in the constructor of this class we can initialize this field and finally here we can use that reference stock service get product quantity from stock it's still complaining okay because that method was originally declared as private and now resharper is telling cannot start it's visual studio that is complaining it says cannot access private method get product quantity from stock here so I'm going to go back to my stock service and change that to public now control tab back here the problem is resolved also another thing that got my attention is this method here is a little bit too long for my taste I would like it to be a little bit shorter see we are using stock service so it's obvious that we're dealing with the stock so it would be more so it'll be cleaner if we just name this method get product quantity instead of get product quantity from stock right now how can we rename this I told you earlier control R and R again so control R and R and then I'm going to go ahead and get rid of this extra two words get product quantity and guess what resharper automatically renames it here as well as in the stock service where we define the method and another tip I show you is look at these namespaces here the first two are gray which means they are not used as well as the last two now with visual studio you can right-click this and go organize using and select remove and sort which cleans up your code or you can install a plug-in called vs commands which automatically cleans up your code whenever you save your code file so in this case if I press ctrl + S to save this code file see it automatically removes that I don't even have to touch my mouse okay I think this code is almost ok let's get back to our code behind file see the intention of this code is very clear now instead of having button 1 or text box free or seeing all the details of connection and comment I can immediately see this button when clicked it tries to check availability it goes to stock servers which gets the product quantity from the database and returns it here if it's zero this is the message we display otherwise this is the message I don't care how the stock service gets the product quantity from the database it's the implementation detail of that service if there is a bug in that code I can just go here click this and press f12 to jump to that method and then read the details of this method so the lesson is your code should be clean such that a reader looks at a method name like this and knowing what's happening without going and looking at every line of that method similar here see we tend check availability I don't really necessarily need to look at this detail I can just look at the method name and say okay this is checking the availability of the product okay that's it for this video I hope you enjoyed it and if you like me to review your code please contact me I'm more than happy to help you thank you for watching [Music] [Music] [Music]

Channel: Programming with Mosh

Views: 344,161

Keywords: visual studio, c#, .net, asp.net, webforms, asp.net mvc, clean code, refactoring, coding, code smell, quality code, programming with mosh, code with mosh

Id: 5koPpYVa020

Length: 34min 18sec (2058 seconds)

Published: Sat Nov 15 2014