On Writing Software (well?) #1: Pilot Episode

hey everyone welcome to on writing software and writing suffer well I don't know I don't know what this is gonna be called yet what did you know is I wanted to have an opportunity on software in writing it specifically looking at code while we do it not just as an abstract form of discussion not really in a way of trying to show you a specific library or whatever just the considerations that I encounter as I'm writing code predominantly for base camp sometimes for rails we'll see where it goes I just wanted to have basically a chat with the camera as though I was sitting and having a conversation with another programmer and we were discussing how can we make this piece of code better how can we make it clearer easier to understand all these good things that we're considering when we're writing software so the first thing I want to open up with and talk about today is the idea of code comments code comments I have a sort of mixed relationship with that sometimes it's really great that you can have a chance to explain something that does not seem obvious from the code but more often than not I tend to look at code comments as a bit of a smell why are you explaining something that isn't apparent or clear from the code itself if you're writing in a programming language right Ruby you should be able to encapsulate whatever it is you want to explain in the code itself such that this does not need additional explanation in prose now here's a piece of code we have in Basecamp 3 this is an encapsulation of the access concept people have access to what we call buckets which is this abstraction that covers teams and projects and circles and and stuff like that and this axis object basically is what we check for when we check whether a person has access to to view a piece of content now D axis itself when it's just up and running is this basically has too many relationship buckets can have many people accessing them an individual person can have access to multiple pockets but when the access is revoked that's when something interesting actually happens we need to do two specific things in Basecamp we need to reconnect the user from the action table server that's the thing we use to manage the WebSockets connection that we're sending updates over basically you shouldn't get updates from channels that relate to buckets that you no longer have access to so what we do is we check whether the person first of all is a user we have a few different ways of dealing with persons in Basecamp one of them is that someone is a user where they can log into the system another version is with their client or they can't log into the system and a third is actually that they're a tombstone that they've been deleted and then of course they can't log into the system either but that's one thing we need to do but what I really want to focus on today is this remove inaccessible records method because as you can see it is the only method in this class that has code comments and I was just reading through the Basecamp code to started doing a new branch yesterday that I called thh refactoring 2018 as I frequently do I've read through the entire code base at Basecamp 3 and try to make things better than I don't think are good enough or basically revisit decisions that we've made earlier that I think now I have a better idea of how to do and the first file I opened was this access document as you can see it's literally the first model in our directory so I thought when I stood there and the first thing I really just noticed was this comment right when we're removing inaccessible records we give you basically 30 seconds of grace period to undo that when we remove Aldean's inaccessible records it's kind of a job that takes a while to run that's why we run it in a job in the first place and it's also irreversible all these records that need to be removed when they're removed it's kind of pain in the ass too at the back in let's actually just take a really quick look at that in the accessible records job so it removes a bunch of things we have these things called readings which is a recording of whether someone had read a piece of continent system we have book markings we have a bunch of things it doesn't really matter so much what matters is that these things need to disappear when someone loses access to to a pocket but nice thing we came up with here was that sometimes you accidentally end up removing someone and if you add a Mac back in 30 seconds we won't actually have destroyed all these records and and it's kind of just a way of offering an undo in any case I didn't like this comment here I felt like why do we need a comment to explain this concept of forgiveness when we could explain it code and the first thing I thought about here was what does this actually explaining it's not restraining this conditional it's actually explaining this somewhat magical integer here in Ruby we have this well actually in rails I suppose it's an active support addition that you can say 30 dot seconds to explain what this 30 actually means it actually just returns 30 right but we could have written 30 minutes and then it would have returned 30 times 60 well we have our 30 seconds here but it's still a bit of a magic variable because what are these 30 seconds well that's what we explained up here right it's 37 seconds of forgiveness couldn't we just explain that some other way well the standard ways of course to extract an explaining variable so we could just do it in line we could do grace period for removing inaccessible records right 30 seconds and then replace the concrete you sits down here with this explaining variable great alright this is actually a step forward already we could just delete this and that looks better that's that's not bad I tend to see these inline declarations of variables as another sort of smell right like this is not actually something that needs to happen every single time we run this method because it never changes it's a constant in fact right okay great constant let's extract that constant and see what we can do that we could take the constant here and we could what we normally do what I know we'll do is that just shove it right up here and then I think textmate has this way of chuckling case there we go all right great we have a constant up here we can use the same constant down here we're referencing it that's an improvement - right this is getting better there's something else though I find when I extract these constants is how far away are they from their usage and is this a general constant that someone should consider as a Pollak public constant or is it more of a private thing in this case I think it's actually more of a private thing we're not going to need to refer to this constant directly somewhere else so we don't actually need to have it declare it publicly so we could remove this put it down on the private okay even better still it's closer but it's not really fully close enough - to the method that we're using so let's swap the order of these two methods and now it's pretty good I'd say you have the method that references this constant right next to where the constant is declared so you've read them sort of together that introduces sort of a separate problem here is I love to have my private methods in basically you order of contents table of contents is what I call it and the table of constants is declared by when they're called and we're declaring the morning we're called through a couple of callbacks here we have after destroyed that does the reconnection of the user and then we have the asset destroyed commit that once the access destroyed and that transaction is committed then we've removed the records from it so they're not really in the right order here reconnect is called first search should be first but if we just took basically this block and moved it down here that's sort of works but then we have this constant floating here in midair which doesn't really seem that aesthetically pleasing to me either I like it when it's just then to just straight on to the the scope the private scope and this is sort of what I'm talking about like these are the considerations were you have a general principle that you want your private methods to appear in the order of which that they're called the table of contents and then that conflicts with another aesthetic consideration that you want your constant to declare be declared first and you want to them to be clear declared as close to the usage of that constant as possible so when you have these two considerations that are somewhat in conflict you have to choose which one is more important to you and in this particular case I think that it's actually more important to have this look good and then violate the idea of the table of contents especially since we just have these two methods here it's good example two of where if you just have this automatic style checker it can't really encapsulate and deal with this and what we're trying to do and what I'm trying to do basically it's just make this easier for me to read more aesthetically pleasing for me to read easier to come back to yeah so that's basically the considerations that went into just extracting one simple constant because we didn't like the or I didn't like the use of a code comment and that's it the second thing I wanted to talk about today is basically how I go about extracting things into rails a lot of that comes from reading through the code base as I said this is this mission I'm on right now reading through the entire base camp three code base and finding things that would be well-suited to go into Ruby and rails and the thing I've stumbled upon here is is this block the grant person administrator ship so administrator ships is basically a join model again between an account and a person and who has the right to administer that account who is the right to grant other people access to things and remove people and add people to the account and all the stuff that you normally allow admins to do well we've encapsulated that in the idea of an administrator ship rather than just having an admin boolean or something else like this I I really like encapsulating these domain objects and being able to reason about them and giving them a place to live and giving them a whole concept rather than just giving them boolean and in any case here we have an administrator ship association it has many Association but we want to do something a little nicer on the DSL side of things when you want to give people an administrator ship or revoke it and in the grant method here you see is we're basically doing is we're adding methods onto the onto the Association so it'll be I account dot administrator ships dot grant and then you pass it a person now an administrator ship you only needed one so if a person already has one we shouldn't allow them to create another record and database they shouldn't confuse things so we first of all we have a unique index on this pair between the account and the person in the migration that we set up for the administrator ship but we also want to basically deal with that if it pops up at the code if somehow someone triggers or tries to grant an administrator ship that already has been granted and we catch that well you see it's it's a little odd here right so we're actually we're using exceptions as a control flow and what this block of code really wants to be is is something much shorter than this right it shouldn't be all these lines it shouldn't have this code cominis we just talked about in the session before this code comments are a bit of a smell the fact that we need to explain what's going on means that it's not quite obvious what's going on well what's going on is that Brielle's has a default method basically called fine or create by some pair of attributes this will be person person but rails does this in a sort of way that encourages people to get stale REITs if they have a really busy application because finder create will start a transaction then it will try to do a where call and the association for a person see if if there's already an administrator ship between the account work early on and the person we're passing in and if so just return that otherwise create create something well those two separate steps of course are separate steps and those you're liable to to end up with with a stale read and on an application like Basecamp that has a fair amount of usage this certainly can happen and there's just a better way of doing it and the better way of doing it is to basically try to create this administrator ship association up front and if it does not work well that means that it already happened right so we're basically just doing one insert and relying on the unique index in the database to throw an exception if this person already exists and then just return the that record we're looking for we didn't have that or we don't have that right now so it needs all of this right what we want really is we want something like what we have in rails right now we just want it flipped we want create or find by person person person and then that we can encapsulate this all pattern of doing the to create catching the exception and just returning the the person that's offering the database of that happens and then if we have that we can reduce this whole thing to basically an alias grant becomes an alias just to create or find by person makes the BSL just slightly a bit nicer and there we go so this is one of those things I'm looking to extract just a small extension to else put it into rails and then we'll have this pattern go forward in fact perhaps this pattern should be the default and we should consider whether we want to deprecate the find or create by signature instead just one other thing I'm noticing here is I don't know how we ended up with this but we're actually can just deal with records directly it doesn't need to deal with just IDs so let's fix that at same time and there we go that's pretty much it

In which I talk about a dislike of code comments, replacing them with explaining constants or extracting a concept into Rails itself.