Refactoring a Command Line Shell | Code Roast Part 1

few weeks ago I realized I hadn't done a code review video for quite a while I miss it you know just bashing other people's code it's my happy place so today I'm going to get my fix because it's time for another where's the intro maybe I should do this a bit more enthusiastically because it's time for another play the intro already [Music] today I'm going to review and refactor a command line shell application in which you can hash encode and decode strings the only thing that's missing is sudo thank you moody for submitting your code for roast if you want to get your code reviewed like this not sure why you would want that but if you do then join my Discord server at discord.iron workouts in this channel where you can submit your code for roast even if you don't want to submit your code for Rose this is still a really chill community that I'd like you to join lots of people helping each other and we recently reached over 10 000 members which is absolutely amazing so I hope you join us and come say hi now let's start dashing some code what I'm going to do is I'm first going to do an analysis of the code and then I'm going to do a refactoring but before I do that let's first run the script and see what this actually does so as you can see this shows a shell-like environment and it allows you to end go to decode and hash strings for example I could do something like this so this creates a hash of the string iron codes and if I simply type hash then it's going to give me an overview of the syntax now currently this code is not really stable because if I type for example hash ankles and I forget the algorithm then actually the application crashes let me start that up again and encoding and decoding Works in a very similar way so here's a base 64 string of iron codes and then if I take this string and I do a decode operation then we'll get back the original string so this is basically what it is next to these three actions you can also display the help simply by typing help or you can write exit to exit D shell and that's basically all there is to it so let's take a closer look at the code so I'm going to start at the left by looking at the overall structure so one thing you see that we have Source folder that contains a main file that basically Imports everything and then runs the app and then everything else is in something called util package that's already one thing you should definitely avoid doing don't call your package util package because it gives zero information about what the package actually does there's a couple of files in here some of these are actually not needed for example this is simply an empty file so already I'm going to delete it just to keep things a bit cleaner then we have a file algorithm.pi that Imports all kinds of different hashing encoding and decoding algorithms and that stores them in variables so we can access them later that also shows a couple of problems one is that the naming is very inconsistent for example here this one is uppercase which is the proper naming standard for constants in Python this isn't this is some sort of weird mix of camel case and snake case so if you use variables that are not constants always use snake case in Python don't use camera case and certainly don't use camel snake case that doesn't even exist another thing is problematic here is that the encoding constant actually doesn't only contain encoding functions it also contains decoding functions and that makes things complicated later on I'll show you in a minute what I mean by that another thing is that these consoles do not only contain references to these actual functions they also contain the documentation but you might think okay that's maybe nice that this is together but on the other hand it's also kind of cumbersome because that means that currently every entry in this encoding dictionary means something else it's either a list of functions or it's a list of documentation and what it is depends on the key and that's not really the proper way to do it it also makes it hard to type these things because normally a dictionary is a key value mapping and currently if you look at what vs code thinks this is well it thinks this is a dictionary which has a string too and then it's like this extremely complicated type because it simply doesn't know what that is now part of that also is because of these functions obviously that are all a bit different so the type that is inferred by vs code by pylons it's very complex but by also adding the documentation of further complicating things also sometimes these algorithms are lowercase sometimes they're uppercase which is also quite confusing and then same here we have the hashing algorithms which is a list of algorithms and then you have the actual hashing functions again this is uppercase that's lowercase I wouldn't make that distinction it's Error prom also here you see that the documentation is part of the dictionary of hash function so I wouldn't do that another reason by the way for not including documentation here is that if you want to create in a later stage a multi-language version of the same application then you might want to read the documentation from a Json file and not have it in the code at all so then if you'd have to sift through all the algorithms and constants that are like in your util package then it's going to be hard to update the documentation if you want to change things another thing is that actually you could wonder whether it's really needed to have a list of algorithms and a dictionary because a dictionary already has a list of keys so why do we need to store that twice it just means double the amount of work because now if you want to add another algorithm we'd have to add it here but we also need to add it here and that's just annoying I guess that has something to do with the fact that documentation is part of the dictionary so here we don't know whether it's something as an algorithm or whether something is a documentation string so that's something we're going to fix later now here we have the encoding API and this is let me make this a bit larger this looks like a extremely complicated piece of code there is like an operation which is an integer and you have these constants where you indicate whether something is encoding or decoding it passes a function that is a non-standard type hint of callable then you have some asserts then there is an if else statement where you define a function that checks if a string is a string or if it's actually bytes and if it's a string then we're going to encode it so that we can actually call decode on that particular function uh it's like extremely hard to understand what this is actually doing and it doesn't really solve any problem it doesn't make it easier in the main file for example if you look at how this is used in the main file then let me scroll down a bit then we see that we have here the encode method for example again snake case not camel case and then you see we need to call the function like this but why do we need to have an encoding manager for that why don't we just call the function and it also doesn't solve duplication issues because encoding so you see here and decoding look almost the same so it complicates things but it doesn't actually solve the duplication that we see here if you look at the hashing function that's actually quite similar to the encoding function so it also optionally transforms a string into bytes because that's needed for the particular hashing operation lots of duplication and because this is a separate function that gets another function and that then returns also itself again a function it's not very clear to me why why this is set up in that way okay the shell so we have here this is basically the model of the shell the way to interact with the program so we have a class command that contains a command that's the the first part of the thing that you type and then it contains a list of arguments also there's here A bunch of helper methods that are a not really used anywhere else in the code but also not really necessary since data classes already return a representation of the object so I wouldn't even do this and also the question is whether we really need a class for this because well a command is actually pretty simple it's a sequence a list of strings basically you can just Supply the command or you could basically parse it and then you'd have a list of strings in the first one is to come out so I mean we could leave this as a data class you could also turn it into Tuple if you wanted to something simpler I'm going to try something else in a minute there's also a shell class so that has shell input again Watch The Camel case versus snake case there's other typing issues here as well like we can supply a tool which is has an uppercase character it's not standard it's of type string but it's assigned non which is not loud because that doesn't match the type shell input itself returns a command or a bull which also is complicated I typically really try to avoid methods functions that return things that can have different types depending on different situations because that means that whenever you call that method whenever you use the method then you also have to build in checks to make sure that what you're getting is actually what you expect so in this case every time you use shell input you have to verify that what you got back is actually a command object and not a bull or vice versa and same for parse command that actually does the parsing operation so this actually Returns the command and its arguments but if there are no commands then it's going to return false and that's basically the problem because then this thing also returns false if there is no command that could be parsed and they have to check for that and everywhere else in the code another thing is why is this a class it's not really needed there's no instance variables that we need to keep track of these things can just as well lb functions and it's going to be way simpler then we have a tool file which is currently not being used so I'm simply already going to delete that and we also have a util function that's of course similar to the YouTube package not a name you should use for your module don't use util funks because we have no idea what it actually means so let's take a look at what this actually does so actually when you look at this it basically loads a configuration file but actually when you look at the main file this is not being used at all at the moment so I'm also for the purpose of this review going to particularly this also seems to be some sort of testing there I don't I don't know what that is exactly so I'm simply going to delete that the config file that we have here I guess you got started with being able to load these algorithms or Define these algorithms in a Json file which makes a lot of sense but it's also currently not being used so I'll also delete this for completeness one final thing that I did is that the original code had a requirements.txt which contains all the dependencies so I change this to a Pi Project file because then I could also easily create a virtual environment I've also added a poetry.tomo file so I can Define that the virtual environment should be defined in the project so the VM folder is created inside this folder so I can easily delete it later finally let's take a quick look at the main file so we see there are some imports that are not being used furthermore you see that we have a documentation string which is totally fine but there's also a class that's called interface which is very generic again it's not really clear what that actually does but there's also kind of confusing setup of default commands and normal commands but actually the only thing that the default commands are doing is that they display the documentation which is again confusing because the decoding documentation is accessed by going to encoding documentation and then passing the decode operation so encoding is as a dictionary constant and decode is actually an integer and I find it very confusing so then for actual encoding you see that hard coded in the method is that we print documentation basically or we give or there's no sort of error and same for decoding so here actually there are some print methods that's also hard-coded in the method and then it uses the encoding manager to to actually decode or encode the string hashing is the same thing so again we have hard-coded prints in the methods I would typically avoid that I would really avoid that then what else do we have we have exits which is currently a sleep operation so that's fine it's just for testing that's no problem at all there is a set text method which I'm not really sure why that is needed and then we have a help which also will print a documentation and we have execute which runs a command so then it first goes through the default command ounce and then depending on the length of the number of arguments it's going to call the default command so I think that's also confusing I would assume that a command is let's say a definition of the syntax of a command so help could be a command without any arguments decode string algorithm is another example of a command and the way this is set up is that the syntax of each of the commands is sort of directly linked with this code because currently we can't have command that has zero arguments because then it's going to be considered a default command whatever that means so to me that's very confusing I would say that a command should specify the arguments that it has okay so then we have a run function so that basically mimics the shelf so there is a while loop it gets the commands it reads the command from the input and then it's going to execute the command and in main we then create an insta sense of the interface class and then called the run method so also here the interface class is not really needed in my opinion it doesn't really contain any useful information and definitely if the shell becomes something that's no longer a class because that's also not needed then this code becomes way simpler to manage final thing I'd like to mention is that it's a good thing to use an auto format so if I save this you see that it changes all the formatting well because I'm using black and that's really helpful to me because it makes my code more compliant more standardized and I don't have to worry about formatting so always use an auto formatter so that's my review of this code by the way if you like to become better at doing these sorts of reviews yourself I have a free workshop on co-diagnosis you can get access for free by going to ireland.com diagnosis it's about half an hour teaches you lots of different things that you can do in order to improve the way that you review code and that's going to help you tremendously getting better at identifying problems much faster also in your own code so I unlock code slash diagnosis if you want to learn more of a hospital Link at the top now let's get started by improving this code the first thing I'm going to do is I'm going to make a couple of improvements here in the Shell file so the first thing that I'd like to do is simply get rid of this whole command data class because I really don't think it's needed there done actually what I'm going to do is I'm going to define a command type which is simply going to be a tuple string and a list of strings so that's basically all a command is command and the argument then the shell class I'm going to delete because I don't think we need really a class for this if you look at the code here so 2 is actually not being used anywhere in the code so for Simplicity I'm just going to delete all of that and I'm also going to return only a command as a result so that it's clear exactly what we're going to expect and then you see there's another thing here which is a sort of a header so what I'm going to do is I'm going to take this and Define that at the top like so and now shell input can actually be really simplified a lot because one is that we need to get the user input and we're simply going to call input with the shell header so that's basically what we're doing here and then we need to parse the command so I'm going to come back to this in a minute parsley command is basically this function so self I'm going to remove that command or blue is going to become command and I'm also going to change the name and what I'm going to do is call this parse command string because we know the command here is a string you can see the code is actually sort of complicated it does a splits and then several other splits so what I'm actually going to do is I'm simply going to rewrite this function to make it a bit simpler so what I'm going to do first is I want to get the parts so that's command dot split and this is not really needed so that gives me the part of the commands and then what I'm going to do is extract the arguments so first the command itself is going to be command parts zero dot lower dot strip so what I'm doing here is making sure the command is lowercase and doesn't contain any extra characters that are not needed and for the argument I'm going to do the same thing but here I'm going to say part dot strip for each of the part in the command Parts counting from one so that gives me the commands and the arguments and then I'm going to return command and arguments like so and then all of this is not needed so I'm simply going to delete this so shell input is then actually also really simple because we're simply going to return parse command string of the user input and we can delete all of this and let's also rename this so that it uses snake case and there we go and data classes is not needed anymore so while we're at it let's also do some renaming so actually the package we're going to call hatch help so that's a hash and code decode and I'm going to use lowercase here and shell I'm going to use core as a name because that's actually the core of the actual shell and now let's clean up the code a bit here in the main file so we don't have util package anymore of course we have shell like so and same thing here so now it's importing all kinds of things that's not totally correct another thing that I typically like to do is not have my init files contain any extra Imports unless that's really necessary so here basically I could also import the necessary functions for the shell from headshell.core so I prefer to do that so also I don't like the uh from algorithms Imports Asterix because that's going to import everything we don't really want that in all cases so I don't really like that approach so now what I'm going to do is I'm going to add here a line from head shell dot core import so what we're going to need is the shell input and for now that's basically it and these ones I'm going to clean up in a minute so how does that then work well if you go to the actual class we see that we have a run and an execute method run is basically an infinite while loop we don't need a separate variable for this we can simply write while true and also it's something that I commonly see when people write classes that they have a tendency to store everything as instance variables if you use a class that's not really necessary Only Store instance variables that are used by multiple methods and here that's clear not the case because if you call execute you're actually passing the command as an argument so you don't need to store it as an instance variable so instead of doing this I'm going to replace this line by saying by writing that we have the commands and the arguments which is the Tuple equals shell input and now also we don't need to do any check whether there's really a command or not but we can simply call execute and pass it the command and the arguments and you could also use the command type if you wanted to here so here we can remove this if statement it's not needed and we can also remove this and then we're going to call here self.execute on the command and the arguments like so I'm going to clean up these print statements in a minute so now there's no problem obviously we have to deal with these commands and default commands which honestly I think is a bit confusing but for now let's first clean this up a bit so here this is going to be if the command is in the self.default commands list then we're going to do this and if it's in the command list that we're going to do that I'm going to change this later on and then here we can simply check that the arguments are higher than zero and here same thing like so so now we've sort of cleaned this up a bit I still need to remove all of these things because that's no longer there like so and here the same you also see that here there's actually quite a bit of duplication that will deal with that in a minute I'm going to revise this completely anyway so actually the way that I like to set this up is to really treat each command as a separate function so there's a couple of these functions that we can do in a pretty simple way for example the help method and what I'm going to do because I want to get rid also of this interface class that we have here is that I'm going to move these things to separate functions so let's start with the simple help command so I'm going to put that here at the top so we have the documentation here so we have the starting screen documentation so here I'm going to write the help method and I'm going to rename that to help shell because that's going to display the shell help also this is not going to have any return value but I simply want it to print the documentation now instead of doing that hard coded here what I'm going to do is similar to this documentation I'm going to supply a health talk which is this and now I'm going to print the help dock like so so that's the only thing that the help shell is going to do what I'm also going to do for each of the command functions is that I'm going to standardize the way that the arguments are supplied and I'm simply going to assume that the arguments are going to be a list of strings and here we're not using those arguments because help doesn't need them but still I want to supply them and that is also going to help us solve the problem of what happens if you don't supply enough arguments or you supply an unexpected number of arguments so that's help and then what I'll do is I Define a dictionary of commands here where we have help and that's going to be help shell and now what I'll do is that I'll step by step move things to this new command structure and execute new command is now also really simple we simply check that if command in commands then we're simply going to call what am I doing wrong here oh of course I need to call this commands that's better also makes more sense so let me go back to the execute method of the gets the commands and argument so if there is a command then it's going to call that particular command with the arguments that's exactly what we want and I'm going to delete all of this because I'm going to handle each command and each function separately another thing I'm going to do is turn these things also into functions because there are no isn't there's no need for them to be methods in a class so now I can simply have execute and run like so or perhaps let's actually follow this a wrong shell so that that's a bit clearer what it does and then we're going to do run shell like so and as a preparation for what I'm going to do in a few minutes I'm already going to move these two print statements here I'm going to move them to the main function now running the shell executing commands is done by these two very simple functions so now step by step let's move the other commands to separate functions so we have exit so that one is also pretty easy to do so I'm going to uh so I'm going here and then turn that into a simple exit shell function we don't need self and go to the indent this and also I'm going to standardize the list of arguments like so and then I'm going to add the exit command here it's a type obviously like so so next these are actually not being used at all in the code so I'm simply going to remove them and now we have if all is going well we have three functions left we have hashing we have decoding and we have encoding and there's the documentation which I'm going to combine with the other things these two I can also delete and while I was editing this I realized this video is becoming way too long so I've decided to cut this into two two parts next week I'm going to finish the refactoring by looking at the hashing decoding and encoding algorithms and clean that up a bit and then finishing up the project I'll put a link to the final part of the series right here as soon as it's out until then thanks for watching and take care

Channel: ArjanCodes

Views: 23,961

Keywords: refactoring, python code review, code roast, code roast arjan, code roast python, refactoring code, python code roast, refactoring code python, refactoring python code, refactoring python, python programming tutorial, python refactoring, python refactor code, python programming, python tutorial, code refactoring, refactoring code explained, refactoring code best practices, code refactoring python, clean code python, refactoring python book, code roast 2023, code review 2023

Id: TDgouhWA13A

Length: 26min 21sec (1581 seconds)

Published: Fri Jun 02 2023