Two quick tips for writing legible code

We’ve all been there. Looking back over a piece of code we wrote many moons ago and wondering what the hell our intent was. A similar thing can happen when looking over code somebody else has written.

Whilst neither situations indicate ‘bad’ code, it does waste valuable time whilst you try and work out what the f@#k is actually going on.

What I want to share today, are two of my favorite tips for writing legible code.

Storybook Code

My first, and biggest tip, is to write your code like you would write a story. Imagine you are somebody who has never seen a single line of code in their lives, and you have to work out what a piece of code is doing.

Take the following example:

var deserializedObject = JsonConvert.DeserialzeObject<MyObjectType>(aLongString);

The vast majority of .NET developers have dealt with Newtonsoft.Json at some stage, so this makes perfect sense. But imagine you have never seen it before… You could probably figure it out, but isn’t this much nicer:

var myObject = MyObjectType.CreateFromJsonString(aJsonString);

The intent of the original developer is so much easier. They intended to create an object of type ‘MyObjectType’ from a JSON string.

A better example

That example is trivial, so let’s look at an example from a recent project I have been working on.

I’m going to write both variations of the code (pre and post refactoring) before I explain what it does. Let’s see:

namespace JEasthamDev
{
    public class Uploader
    {
        public void Upload()
        {
            var outstandingDocuments = await this._documentService.GetDocuments();
            var batchName = $"newbatch_{DateTime.Now}";

            foreach (var document in outstandingDocuments)
            {
                foreach (var file in document.Files.Where(p => p.HasSentToOcr == false){
                    var bytes = file.FileData;

                    using (var ocrResource = new OcrResource)
                    {
                        var existingBatch = ocrResource.GetBatch(batchName);

                        if (existingBatch == null)
                        {
                            existingBatch = ocrResource.CreateBatch(batchName);
                        }

                        existingBatch.AddFiles(document.FileName, bytes);

                        ocrResource.UpdateBatch(batch);
                    }
                }
            }
        }
    }
}

namespace JEasthamDev
{
    public class RefactoredUploader
    {
        public void UploadAllOutstandingDocumentsToOcrEngine()
        {
            var outstandingDocuments = await this._documentService.GetDocuments();
            var batchName = this.generateUniqueBatchName();

            foreach (var document in outstandingDocuments)
            {
                this.uploadSingleDocumentToOcrEngine(batchName, document);
            }
        }

        private void uploadSingleDocumentToOcrEngine(string batchName, IDocument document)
        {
            foreach (var file in document.GetFilesNotSentForOcr())
            {
                var ocrBatch = this.createNewOrRetrieveExistingBatchInOcrEngine(batchName);

                ocrBatch.AddFileFromBytes(file.FileName, file.GetByteRepresentation());

                ocrBatch.Update();
            }
        }

        private IBatch createBatchInOcrEngineIfNotExists(string batchNameToCreate)
        {
            using (var ocrResource = new OcrResource)
            {
                var existingBatch = ocrResource.GetBatch(batchName);

                if (existingBatch == null)
                {
                    existingBatch = ocrResource.CreateBatch(batchName);
                }

                existingBatch.AddFiles(document.FileName, bytes);

                ocrResource.UpdateBatch(batch);
            }
        }

        private string generateUniqueBatchName(){
            return $"NewBatch_{DateTime.Now.ToString("yyyyMMdd-HHmmss")}";
        }
    }
}

Which is preferable from a point of view of having never seen this code base before?

I fully expect that most .NET devs reading this can easily interpret both samples, but in my personal opinion, the second is so much easier to follow.

I try to follow this style of code for the following reasons:

1. Readability – If I look at the second sample again in 12 months I can quickly ascertain what’s going on at a very high level
2. Single and specific responsibilities – Each method has a very specific and singular concern.
   • UploadAllOutstandingDocumentsToOcrEngine – The only public API of the Uploader class. No comments required to understand what that does
   • uploadSingleDocumentToOcrEngine – I bet you can’t guess what this method does…
   • createBatchInOcrEngineIfNotExists- And again, who knows what this method does
   • generateUniqueBatchName – I would ordinarily make this a static method of the Batch object itself, but for clarity, it’s within the method
3. Small method length – A really limited number of lines in each method, making understanding of what the method does extremely easy

Which leads me on quite nicely to my second tip on writing readable code

Keep your methods small

This tactic has been talked about many times over, and there are so many rules of thumb out there.

• No more than 20 lines
• Should fit on a single piece of A4 paper
• No more lines than the number of pixels display on the screen multiple by the dimensions of the monitor currently being viewed on

All these are fantastic options and will set you on the right path (aside from the last one, nobody has time for that!).

However, my personal favorite is nice and simple.

All code in a method should be viewable in a single viewport window

The time saved by not spending time scrolling up and down within a single method is incredible. Variable declarations, logic, everything all in one place is fantastic for your develope productivity… and sanity!

Hopefully, these two tips provide some useful hacks for your productivity as a developer. I’m always fascinated to hear other peoples ‘hacks’ and tips for keeping their code legible though. Drop a note in the comments and let’s have a discussion!

Comments

[Zohar Peled]

IMHO, the method names in the last code example are way too long. Personally, I find such long names hard to read, even though I know that the capital letter means a new word.
Instead of these huge long method names, use XML documentation comments.
If it's a public API, compile it with the XML documentation file.

I would much rather read this code instead, even though I've made only small, seemingly insignificant changes to your example:

namespace JEasthamDev
{
    /// <summary>
    /// Responsible for uploading outstanding documents to ocr engine.
    /// </summary>
    public class Uploader 
    {
        /// <summary>
        /// Uploads all outstanding documents to ocr engine.
        /// </summary>
        public void UploadAll()
        {
            var outstandingDocuments = await this._documentService.GetDocuments();
            var batchName = this.GenerateBatchName();

            foreach (var document in outstandingDocuments)
            {
                this.UploadSingle(batchName, document);
            }
        }

        private void UploadSingle(string batchName, IDocument document)
        {
            foreach (var file in document.GetFilesNotSentForOcr())
            {
                var ocrBatch = this.GetOrCreateBatch(batchName);

                ocrBatch.AddFileFromBytes(file.FileName, file.GetByteRepresentation());

                ocrBatch.Update();
            }
        }

        private IBatch GetOrCreateBatch(string batchName)
        {
            using (var ocrResource = new OcrResource)
            {
                var existingBatch = ocrResource.GetBatch(batchName);

                if (existingBatch == null)
                {
                    existingBatch = ocrResource.CreateBatch(batchName);
                }

                existingBatch.AddFiles(document.FileName, bytes);

                ocrResource.UpdateBatch(batch);
            }
        }

        private string GenerateBatchName(){
            return $"NewBatch_{DateTime.Now.ToString("yyyyMMdd-HHmmss")}";
        }
    }
}

[James Eastham]

Yeah, I do like the casing changes. However, I like keeping private methods distinguishable in my code by keeping the first letter lowercased.

Also, agree with your point on the long method names, that's something I've mulled over for quite a long time and probably the one thing I can never decide on a good length for.

I find a nice descriptive method name makes a high level skim read of the code much easier. XML comments are nice, but it means you need to interrupt the reading flow by hovering the mouse over the method name.

I can cope with the length, if it gives enough detail to be useful. Obviously, something like

public void UploadDocumentsAndCreateBatchInOcrIfDoesNotExist(IDocument document){

}

is just crazy and something I would never condone.

That said, if I was writing a Nuget library or anything open source I would add XML comments on all public API's.

[Zohar Peled]

Well, in that case, I would rather call the class itself "OutstandingDocumentsToOcrEngine" and leave that part out of the upload methods names - but hey, that's just me.
I find it very hard to read these long names, and I don't think it's because I'm a non-native English speaker, (after all, I've been reading and writing PascalCase/camelCase code for over two decades now).

I mean, compare how easy it is to read

Upload documents and create batch in ocr if does not exist

vs

UploadDocumentsAndCreateBatchInOcrIfDoesNotExist

It's not just me, right?

[James Eastham]

Fantastic points, especially on making it a class instead of a set of methods. It's not you at all!

My example may be trivial, but the crux of what I was trying to get across is small methods with descriptive names are better than one long method which is difficult to follow.

I put this post out hoping to start a discussion and that's exactly what we've got. :)

[Zohar Peled]

You might also want to read Stop Writing Code Comments. I can't say I totally agree with this, since I really do believe that comments should be used in code (Especially xml documentation for public APIs, but not only). In fact, in a comment to another post a few days ago I was telling how good commenting habits from the old me saved the current me literally days of work - but yeah, as a rule of thumb, he does have a valid point: good code is a code that's easy to read even without comments.

[James Eastham]

I agree wholeheartedly with the zero comments within code. I think if you need to add a comment to describe a line of code, then the line of code isn't as clean as it could be.

var documentList = this._service.Get(); // Retrieve all outstanding documents from the database

foreach (var document in documentList){

}

vs

var outstandingDocuments = this._documentService.GetOutstandingDocuments();

foreach (var outstandingDocument in outstandingDocuments){

}

The second one is longer, but I'm completely ok with that for the gain in legibility.

Public API's are a whole different matter, and as far as I'm concerned should always be fully documented with XML comments.