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

Key Takeaways

okay guys I found this code on one of Facebook groups called as.net 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 the 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 are improving it first we are declaring a quantity setting it to zero next we are declaring a SQL 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 SQL 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 in32 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 see this code is poor naming convention look at this button one here what does one mean how is it different from button two or button three I can't 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 that's more meaningful now I click this button here to show the events and see it automatically rename that method which is oops didn't 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 aqt here now it's kind of obvious that it's quantity but aqt is not a very beautiful and meaningful name for a variable why a quantity why not just quantity and instead of QT writing the actual word quantity it would be much cleaner so I'm going to rename that but see how I rename this 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 controlr and R you're going to get a dialog box which actually is not appearing here because I've got a plugin installed called resharper but um even if you don't have resharper you can still use Visual Studio to rename variables basically when you use that dialog box you rename a variable and visual studio automatically renames 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 suggesting in32 which is not really a good variable name so I'm going to go ahead and just rename that and enter c it automatically updated the quantity variable here so remember control r and r I believe another shortcut in Visual Studio is F2 so try it and you'll love it similarly this CN CN 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 control R and R and see this time resharper realize that this is for a SQL connection and the variable names is suggesting is connection or a SQL 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 dr2 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 so let's just rename this to query and instead of cmd1 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 three 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 2 or if there is a Tex TT Box 2 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 name 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 ASX 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 resharper to rename that for me now let's save everything if I go to that Designer file here you see that resharper automatically rename 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 text box I select the properties here and here here it's not renamed I don't know why resharper cannot rename um objects on a web form like this so I have to manually change that to txt book name all right 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 are telling me that okay we have quantity we have connection we have a reader and then you kind of forget this uh move on and go somewhere 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 SQL reader we declare it and use it straight away and see the court is also one line shorter all right another thing we can do here is using the VAR keyword in C with VAR you can uh write cleaner and shorter code let me show you how it works so here instead of using SQL connection I can just use the keyword VAR and the benefit of that is see with this I'm saying SQL connection connection is new SQL 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 SQL command and again new SQL command so one is enough to have it here and we can use bar 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. close 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're 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 um 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 that 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 Udi 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 exception is thrown here no matter what this finally block will always be executed okay now we have an error here reader because reader is defined in this Tri block but we are using it in the finally the problem is um when we Define it here in the triy the scope is is applicable only here so it has no meaning outside that block which means we have to take this line out of the tri block so I'm going to put it here all right let's look at the next problem in this code look at this bit here we are trying to construct a dynamic SQL query based on what the user put in the text box this is very dangerous because there is a security attack called SQL 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 P what the haer 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 um 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 qu 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 the 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 insert into members or users you know with some values blah blah blah now they it also requires a little bit of um intelligence and guessing the schema of your database uh I'm not a hacking expert but I know there are ways for them to even extract the schema of your database no exactly what what tables you have what columns you have and based on that they can construct a query that will take that would give them the complete control of your uh application and potentially your server depending on the permissions so the lesson here is never ever ever trust um 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 parameterized queries which means instead of concatenating strings here we're going to change our query to something more um kind of static which would be like this so this is a parameter that is prefixed with the AD Sign so what the user puts in the 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 would go here and when SQL Server runs that query um it doesn't see this t takes as yet another query so this is the actual query that will be executed now in order to use parameterized 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. parameters. add now the parameters to the ad 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 SQL parameter object so here I'm going to create a SQL 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. text right so with this we can get rid of our SQL injection attack and our code will be more secure let me get rid of this commands comments here okay let's see another way to improve this code see this method here between check availability is doing too many things it's trying to access the database here and read the quantity and then it's trying to show a message to the user 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 um first of all I'm going to move all of this code into a separate method just to show you a stepbystep 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 in the 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. right all it needs to know is how to get the data from the database right so let me go ahead here going to declare it as private and I wanted to return the quantity which which would be an integer and I'm going to call this method get product quantity from stock as the sorry stock and as the parameter I'm going to define a string which is book name and see um creating um 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 write code that is Meaningful like a uh 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 reads 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 txt bookname do 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 can get rid of everything else here so let me clean up this code 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 uh my personal taste and I will show you shortly how we can even make this a little 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 variable so I'll just put that here and get rid of this extra variable here all right other things we can do to improve this code is this hardcoded connection string here it's not a good thing why because if the connectional 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 it 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 these connection strings so I can define a connection string here use the ad give it a name let's call it I don't know book book store for example bookstore 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 it doesn't require a Rec compilation and when we deploy this application to each environment whether it's Dev test or production we would 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. configuration so I can just hit enter and resharper automatically adds that uh name space 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 um menu on the top here that is called resolve namespace something like that I Str recommend you to install resharper because it makes your coding a lot easier all right let's go ahead so configuration manager is a class I'm going to go to app sorry to 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 SQL connection I would argue that it would be also cleaner to instead of writing this raw query here you use a store procedure for example you would have a store 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 um got my attention is this book name we declared it but 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 web page 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 the stock or updating the stock from the database now there are various ways to model that and create layers for your application but um it's going to beyond the scope of this video because this code is I think for beginner level level.net developer and I don't want to confuse you with too much complexity so what I would suggest is at a minimum um go to your project here right click add a new folder call it maybe services so here I'm going to create a a new class at 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 uh go and use the mouse to select this and cut and paste or there is a quicker way so anywhere here I can press control and M twice the method gets collapsed now I can press control X and then I can press control and 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 come here and then contrl V to paste the code right it's much easier when you use your keyboard again in my visual studio course on udem I talk 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 um this SQL connection here is um in the system. data. SQL 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 S similarly configuration manager is in system. configuration namespace which does not exist here again I rely on resharper to automatically fix this for me so the shortcut is alt and enter there you go which means now I can go back to my code behind file again I'm going to use control and 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 should always be prefixed with an underscore and C resharp 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. getet product quantity from stock it's still complaining okay because that method was originally declared as private and now resharper is telling cannot sorry 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 um 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 um so it would 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 contrl R and R again so contrl 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 this name spaces here the first two are gray which means they're 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 VSS commands which automatically cleans up your code whenever you save your code file so in this case if I press controll and S to save this code file see it automatically removes that I don't even have to touch my Mouse um okay I think this code is almost okay let's get back to our code behind file see the intention of this code is very clear now instead of having button one or textbox 3 or seeing all the details of connection and command I can immediately see this button when clicks it tries to check availability it goes to stock service which gets the product quantity from the database uh 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 B10 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 uh 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 [Music] [Music] watching [Music] [Music] oh