A month of clean code
Jason McCreary

Jason McCreary @gonedark

About: I build things with my hands. The human behind Shift - https://laravelshift.com, master of Git - https://gettinggit.com, and author of "BaseCode" - https://basecodefieldguide.com

Joined:
Jan 11, 2017

A month of clean code

Publish Date: Nov 10 '17
520 44

Each week over the past month, I have posted before and after code samples calling out ways to clean up code. The response has been amazing, many receiving hundred of likes. While I will continue to share these weekly clean code samples, I wanted to collect the previous ones into a single post.

Breaking up long methods

In this first tweet a developer submitted a long Laravel controller action they wanted to clean up. At a high level the controller action handled a course enrollment request. At a low level the code had two paths for enrolling the user.

Before and after code cleanup of a long method

The main question to ask when evaluating a long method is who is responsible for this section of code? The answer to this question will help determine if the code can be moved elsewhere. In this case, the creation of the user could be moved to the model. We could also streamline the two paths by creating a private, conditional method. This method performs the branched logic of creating the user when necessary.

The result is a much smaller method. Some may argue we've simply moved the code. Nevertheless, the intent of our original method is more concise and its component parts are easy to follow. Both improve communicates - which is the measurement of clean code.

As pointed out on Twitter, the after code did have some mistakes, such as missing parameters to createUserIfUnauthenticated. However, these are trivial mistakes which do not change the clean up.


Leveraging objects

The second tweet was a Laravel controller action submitted by a developer. At a high level the controller action handled storing image uploads. At a low level the controller did everything itself - validation, image manipulation, storage.

Before and after code cleanup introducing objects

To clean this code, the strategy is to leverage other objects to handle all the things. A Laravel Form Request object could be used for the validation. We could adopt the Repository Pattern to introduce an object to coordinate the storage of an image.

Similar to before, the result is a smaller method. What's different in this case is leveraging existing objects available in the framework. We also applied a pattern that fit our code. Too often developers do this in reverse - fitting the code to a pattern. Letting a pattern emerge from the code is a far cleaner approach.


Cleaning conditional code

The third tweet focused on cleaning up the conditional code that forms most of our codebase. This has been the most popular cleanup so far. Likely because the fundamental code allows it to apply to many languages.

Before and after code cleanup of conditional code

Most of these cleanups fall under the third Writing Clean Code practice - avoid nested code. Anytime you want to clean up conditional code as yourself two questions:

  1. Can I return the condition directly?
  2. Does inverting the conditional logic improve communication?


Spotting primitive obsession

The fourth tweet focuses on spotting Primitive Obsession. Our obsession with using primitive data types often results in duplication of related code. It's not the exact same code which makes it harder to spot. Over time this code leaks through the system and becomes harder and harder to cleanup.

Before and after code cleanup of primitive obsession

The tips do not draw out some additional characteristics of these value objects. Almost as important as the encapsulation there provide is the fact that they are immutable. This limits state, making them relatively lightweight to add and even easier to maintain. For more detail and examples on Range, read RangeObject by Martin Fowler.

And yes, I would absolutely refactor limit() to utilize array filtering. This would be the next clean up:

function limit($items, Range $range)
{
    return array_filter($items, function ($item) use ($range) {
        return $range->includes($item);
    });
}
Enter fullscreen mode Exit fullscreen mode


Want more cleanup tips? Follow me on Twitter to get weekly tips or sign up for my upcoming field guide.

Comments 44 total

  • Gustavo Castillo
    Gustavo CastilloNov 10, 2017

    Awesome article and thanks for sharing it.

  • Yann Rabiller
    Yann RabillerNov 10, 2017

    Thanks for your article. I agree with most of your refactoring except the first one. I really disagree with the idea of a method User::createFromCourseEnrollmentRequest.

    1/ It's just 4 values that are usually used to create a User (first name, last name, email, password). We can assume there is nothing special related to the context of the Course Enrollment.

    2/ It binds your model to a specific Request (so I can imagine to the Laravel Framework) which is IMO a very bad idea. Here it's just moving some code to hide it in the Model, it's not refactoring. Actually, in my opinion, it will lower the reusability and maintenance of your code.
    What if you want to use something else than Laravel tomorrow?

    For these reasons I would prefer the Before version. But I like the idea of a method to return a User (created or fetched) :).

    • Jason McCreary
      Jason McCrearyNov 10, 2017

      To be clear, there is no actual Model/Request coupling. It's just coincidence that we pass the same object. To your point, we could easily use $request->only() to pluck these values out to pass down to the model.

    • Rahimie Ahmad
      Rahimie AhmadNov 23, 2017

      as for me this static method doesn't introduce a new behaviour to the user model. design pattern wise, it basically uses static factory method (i actually like it), although i preferred to have some sort of EnrollmentRegistrationService for this.

  • Adrian B.G.
    Adrian B.G.Nov 10, 2017

    I was actually looking for Open Source code to write some refactoring articles, but I couldn't found one :/
    I will share your work with my cadets.

    • Jason McCreary
      Jason McCrearyNov 10, 2017

      Awesome. Be sure to share back once you write something up!

  • Rafal Pienkowski
    Rafal PienkowskiNov 11, 2017

    Nice article. Good that you're pasting examples. You clean controllers code but in my opinion for the big picture you should add code snippet for places where the code is moved to. I think that could be very useful otherwise it looks like you're moving code in other places.
    Examples with methods refactoring suit me.
    Don't give up with refactoring. Fingers crossed 😁

  • Pavel Razgovorov
    Pavel RazgovorovNov 11, 2017

    The screenshots are a little bit hard to see (too small) :(

    Will try later from my phone xD

  • Paweł Ruciński
    Paweł RucińskiNov 13, 2017

    For me it looks like a great introduction into usage of Boy Scout Rule.

    That's not mine, but read more maybe here

  • Baptiste Lafontaine
    Baptiste LafontaineNov 13, 2017

    Too bad the screenshots are way to small, they seem really well edited.

  • joseph emmanuel kayode (iemarjay)
    joseph emmanuel kayode (iemarjay)Nov 13, 2017

    Great Article
    IMO clean code is not just about architectures... a code is also cleaner when it can communicate it's intention without the use of comments. like example one

  • Axel Mhar
    Axel MharNov 23, 2017

    Nice Article

  • Nicolae Casîr
    Nicolae CasîrNov 29, 2017

    Awesome!!!

  • GBKSOFT
    GBKSOFTDec 1, 2017

    Thanks for your article, Jason.

  • Jochem Stoel
    Jochem StoelDec 16, 2017

    I hate to sound stupid but these lines with comment blocks, I can only assume you are generating these? Using what? Or did you only manually do them for the sake of example?

  • Pierre-Adrien Buisson
    Pierre-Adrien BuissonDec 21, 2017

    Very interesting, I'll keep on reading your posts, thanks! This is actually one of the area I want to improve these days so your posts come in handy. Just one detail: you should make your screenshot bigger, because they're really hard to read like this, even full-size.

    • Jason McCreary
      Jason McCrearyDec 21, 2017

      Thanks!

      Unfortunately, as commented, this is a limitation of dev.to and their image upload functionality. View the original tweets for a better resolution.

      • Pierre-Adrien Buisson
        Pierre-Adrien BuissonDec 21, 2017

        Oh I'm new to dev.to and didn't know that, sorrry for the useless comment. I'll follow up on twitter as well, thanks for the heads up!

  • Hamdi Rizal
    Hamdi RizalDec 26, 2017

    Really, the only complaint I have in this post is the screenshots are too small. I barely able to read them.

  • Daniel Gutierrez C.
    Daniel Gutierrez C.Dec 29, 2017

    Excellent article, problem with screencaptures is the (automated?) generated cloudinary image of 880px max width; a lightbox would be great (the zoom mouse cursor tricks us to think that the image file will open in a modal window) or a link to original image would be highly appreciated instead of manual editing the link. What I did was to convert res.cloudinary.com/practicaldev/im... into thepracticaldev.s3.amazonaws.com/i... and similarly in every image in order to watch your nice programming standards.

    Who can we contact to express these drawbacks of dev.to?

  • datashaman
    datashamanJan 3, 2018

    IMO, it's better to mirror the use of the collection method inside Set::contains() instead of using in_array, since items seems to be a Collection object. You get more functionality that way and it's more Laravel-ish.

    return $this->items->containsStrict($item);
    
    • Jason McCreary
      Jason McCrearyJan 3, 2018

      I see your suggestion. However, it's important to remember an array is a collection. No need to introduce an entire framework just to use one method.

      • datashaman
        datashamanJan 3, 2018

        Ah, I see that the other calls are to the Set class size and contains. I misread - I thought it was calling those on items. My bad. You are correct.

  • Asif Iqbal
    Asif IqbalJan 30, 2018

    Ah, great article.

    Nice tips for cleaning your code and it is readable with comment on each line.

    Thanks.

  • Johhnyalmi91
    Johhnyalmi91Mar 14, 2018

    What's the code editor on screenshots? Vim or atom?

  • Florian Dutey
    Florian DuteyJun 22, 2018

    1) To do clean code, you must stop using php
    2) Models should not implement every possible way of instanciating them
    3) Transfer from one account to another without transaction? This should be in a service.
    4) The basic minimum for clean code is to have tests for them. I don't see any
    5) No comments?

    • Jason McCreary
      Jason McCrearyJun 25, 2018

      I stopped reading your reply after your first point… Clean code is not exclusive to any one language.

    • Boris Jamot ✊ /
      Boris Jamot ✊ /Oct 26, 2018

      Either your first point is a troll, either it discredits you as a good developer.

  • Saviobosco
    Saviobosco Jul 7, 2018

    Thanks for the wonderful tips.

  • Aditta Chakraborty
    Aditta ChakrabortyJul 9, 2018

    This is a really awesome blog. I love this blog. I wanna translate it in Bangla & post in my organization website. Would you like to give me permission to translate & repost it?

    • Jason McCreary
      Jason McCrearyJul 9, 2018

      Sure. Send me the link when you're done.

      • Aditta Chakraborty
        Aditta ChakrabortyJul 9, 2018

        Thank you very much. I will create an account for you. Then I will send you everything. Give me your email address.

      • Aditta Chakraborty
        Aditta ChakrabortyJul 11, 2018

        Can you give me your email address for opening an account of you? I will give the post Author you.

  • Manjuanand
    ManjuanandSep 4, 2018

    Thanks Jason. These tips really helped me.

  • Boris Jamot ✊ /
    Boris Jamot ✊ /Oct 26, 2018

    Hi, great article on such a simple concept!

    It seems that the $request argument is missing on line 7.

    Thanks!

    • Jason McCreary
      Jason McCrearyOct 26, 2018

      You are absolutely correct. Unfortunately it's an old screenshot and I'm too lazy to redo it. :)

Add comment