DEV Community: Nahuel Garbezza

Clean Code Cleanups

Nahuel Garbezza — Sun, 28 Jun 2020 03:21:13 +0000

Intro

Once in a while, we face the task of cleaning up a piece of software. There could be several reasons for deleting code: users don't actually use it (or it is causing a bad experience), we made an experiment and we already got results, there was a temporary business need that is no longer relevant, or maybe it's better to remove it because we can't afford the maintenance cost; in other words, it is adding more problems than business value.

Having features partially cleaned up is one of the most common problems we can see on any piece of software. It increases the level of uncertainty in the project, and a feeling of "so many things to fix that I don't know where to start". I'll walk you through different stages of cleanup, and things to have in mind to make it in an efficient way, based on my experience.

Prep for cleanup

Most of the time you know that a piece of code is going to disappear. You can smell it (I heard about a skill of senior developers that is hard to explain which is an ability to smell things like complexity). If that's the case, you can start marking the code to easily find it later on, when you finally decide to make the actual cleanup.

I remember in one of the teams I worked on, we had to implement the V2 of a feature (which was a rewrite of a successful experimental feature). When we made the project kickoff, in addition to the V2 work, we added two additional tasks to our backlog:

Mark the V1 code for deprecation (this could be done right away, independently of the progress on the V2 implementation)
Remove V1 code and data models associated (this might need more than 1 PR)

I still remember my first attempts to remove code: they were not so well organized, and they always took more time to complete than I initially expected. I clearly underestimated the task. Lesson learned!, Nowadays, I always try to leave marks in the code when appropriate.

Another cool thing on one of our client projects is a simple deprecation mechanism, where you can mark your code using comments with a specific format:

# DEPRECATE 2020-12-01; ticket number; brief description

The cool thing about this snippet is that it has an expiration date. A script on the pre-commit goes through all the deprecation marks and looks for expired ones. If it finds any, a warning is raised and you have to make a decision: bumping the date or addressing the deprecation. It's a simple attempt to avoid leaving legacy code forever (thus becoming more legacy as time passes by).

Another flagging mechanism I use a lot is in Cuis Smalltalk. There's a very simple built-in feature: you can call the #flag: method on any object (because the method is defined in Object) and then you can easily find it across your program. It's a live deprecation notification because it is code. You can play with metaprogramming and make cool things out of it: like measuring the technical debt. #flag: receives an argument that you can use as a way to categorize your mark.
Here's how it looks when we find all the places where #flag: is used plus a concrete example:

Top-down or bottom-up?

Code cleanup goes smoothly if we have a strategy. If you start deleting code randomly, it's very likely to leave things out. So I don't recommend that "strategy" (Or, actually, lack of it :-)).

In the same way you can build a piece of software, you can clean it up: it could be top-down or bottom-up.

Top-down means starting from the UI, looking at all the code it's being referenced there and start deleting: if it is a web application, the starting point is the HTML, then JS logic, CSS rules, assets, translation keys, and so on. Then proceeding with communication with the backend (routes/controllers), then services or other objects involved, and lastly the models/data models.

Bottom-up is exactly the opposite: for instance, removing models and see where it breaks: that is the next thing to address, until you get to the outermost part, that could be the UI.

I feel more comfortable with the top-down approach because you first remove usages and then definitions (if they are not used somewhere else). Code breaks progressively, while in the opposite approach it is broken in the first touch and it remains broken until you finish. To find unused code, in addition to finders in your IDE, you can rely on linters or tools like unused, which is technology agnostic, and it's easy to run on any project. I used it on Ruby on Rails and it caught a lot of code I missed on my initial searches. I strongly recommend it.

Things to have in mind

Linting rules: sometimes, we can build our custom logic to detect unused code. One that I've seen in one of the projects I worked on is a script to detect unused i18n keys (one of the most common pieces of code that get forgotten in a cleanup).

Tests coverage: before start removing anything, it is a good idea to measure how covered by tests is the code affected by the cleanup, or how confident we are with the existing tests (harder to measure because it's subjective). Tests should give us feedback on the cleanup process: if we have few tests, we could be under a false feeling of success. And if you don't have any test, it is a good time to write more, or you will spend a bunch of time on manual testing.

Manual regression testing: not everything can be caught by automated tests, so we need to know how to test manually, focusing on the most critical areas and parts that could be affected by the change.

Metaprogramming: cleaning code that makes use of metaprogramming is challenging. And on dynamic languages, it is even harder. We can't just do a text-based search to decide if we should remove a piece of code or not. What I do on these cases is to find where metaprogramming is used (in Ruby, that could be when someone is using send or method_missing, or eval -HOPEFULLY NOT!-) and check if that code could affect or be affected by the cleanup I'm doing.

Conventions (could be seen as a particular case of metaprogramming): sometimes, we can't find exact matches of terms in the code, but there is a functionality that a framework like Rails could be providing. Rails can make, among other conventions, plural/singular conversions automatically for you, so when searching code, you need to have that in mind. For instance, in things like routing, store#index means to call an index method on a StoresController.

Not just business logic: sometimes removing pieces of code imply removing libraries, configuration files, test fixtures, translation strings, assets, CI setup tasks, and more auxiliary things.

Commenting code instead of removing is not an option: DO NOT LEAVE COMMENTED CODE. Commented code is dead and confusing.

Version control history is your best friend: If you look at the original commit/PR that added the code you want to remove, you might find related groups of code that can be probably deleted together. Also if you have the chance to talk with the person or team that added this feature, probably you could gather additional relevant information.

Be prepared for the worst: I usually make a revert PR in case something breaks, and let my team (or the person on-call if there are any) know when it is going to be merged/deployed. And if it is possible to make it in small PRs, go for that!

For the future: if you are adding new code to your system, think about how you can clean it up. Is there anything that I can help today as the creator of this feature, in case someone (that could be also me) wants to clean it up in the future? Can some comments be helpful to explain WHY a piece of code was introduced? If I have to use metaprogramming (pro-tip: this should be your last resource), how can I make it "not obscure"?

Conclusions

I'd just say do not underestimate code deletion tasks. There are a lot of things to have in mind. Make it part of your project backlogs, and think about strategies (like the top-down I recommend) to move towards cleaner and stronger codebases.

Ruby's Array: a Swiss Army Knife?

Nahuel Garbezza — Sun, 15 Dec 2019 03:39:01 +0000

Intro

In one of the lectures about OOP I usually give at university we go through design patterns and antipatterns. When we first studied the Swiss Army Knife antipattern, the students asked me for an example and I didn't hesitate for a second. I immediately thought of the Array class in Ruby!

Problem

On a very questionable design decision, Ruby has an Array class that can be used as any type of collection. Want a fixed-size collection? use Array. Want a queue or a stack? use Array! Want a simple ordered collection? Use Array!

Next there's a list of all the methods instances of Array understand:

> Array.instance_methods(false)
=> [:transpose, :fill, :assoc, :rassoc, :uniq, :uniq!, :compact, :compact!, :to_h, :flatten, :flatten!, :shuffle!, :include?, :permut
ation, :combination, :sample, :repeated_combination, :shuffle, :product, :bsearch, :bsearch_index, :repeated_permutation, :map!, :&,
:*, :+, :-, :sort, :count, :find_index, :select, :reject, :collect, :map, :pack, :first, :any?, :reverse_each, :zip, :take, :take_whi
le, :drop, :drop_while, :cycle, :insert, :|, :index, :rindex, :replace, :clear, :<=>, :<<, :==, :[], :[]=, :reverse, :empty?, :eql?,
:concat, :reverse!, :inspect, :delete, :length, :size, :each, :slice, :slice!, :to_ary, :to_a, :to_s, :dig, :hash, :at, :fetch, :last
, :push, :pop, :shift, :frozen?, :unshift, :each_index, :join, :rotate, :rotate!, :sort_by!, :collect!, :sort!, :select!, :keep_if, :
values_at, :delete_at, :delete_if, :reject!]

And I'm being a nice fella and didn't include the extension methods added in ActiveSupport (remember #forty_two?) ;-)

Array's multipurpose use goes against several object-oriented principles. There are no single responsibilities and there are groups of messages only valid for certain contexts. The class loses its essence just to become an object that tries to make everyone happy. The different collection types are not reified at all. Furthermore, if I want to use a stack in my program, I would expect a Stack class with an essential protocol of push, pop, top, empty? and nothing else. I'm definitely sure I wouldn't want to retrieve the third element of the stack, for instance. Or to add an element to the bottom.

Collection types

The idea of specific collection types is not new: Smalltalk-80 specification includes (as of today) the best, in my opinion, collection hierarchy with clearly defined collection types: Array, Set, Bag, OrderedCollection, SortedCollection and so on. Even though this collection hierarchy has some design problems, it allows the developers to choose a collection type that fits the domain they are working with. You can use those collections in pretty much any Smalltalk dialect, such as Squeak, Cuis or Pharo.

Possible Solutions

Don't use Array for everything... have you heard about the Set class?

Ruby has a Set class! Unfortunately, I rarely see it on Ruby programs, maybe because it's easier to create an Array. Just type require 'set' and you'll be set (pun intended) to go. Now that you know about it, it's time to use it!

Metaprogramming to make Arrays more specific

Let's say I want to enforce a list to be used as a fixed size collection. What I can do, given I don't want to implement a brand new collection class, is to "select" just the methods we want, depending on the type of collection we need.

So I want to change default Array’s behavior (and the way I want to use it) from this:

> my_kind_of_array = [1,2,3]
=> [1, 2, 3]
> my_kind_of_array << 4
=> [1, 2, 3, 4]

to this:

> my_true_array = [1,2,3].as_array
=> [1, 2, 3]
> my_true_array << 4   
NoMethodError: undefined method `<<' for [1, 2, 3]:Array

This would indicate that I want to use my list as a fixed size collection (that's the purpose of the as_array message) and therefore, every method to add/remove elements should not be understood on that object. That way I can have (in a dynamic way and still using native Array methods) the exact essential protocol I need.

Here’s a way to implement this:

class Array
  def as_array
    instance_eval do
      undef :<< # and do the same for other methods that add/remove elements
    end

    self
  end
end

By using instance_eval I'm able to change that particular instance I'm using, and undef makes sure that the method is completely removed, and for the end-user, it seems the message never existed at all.

I can even change the semantics of some of the messages. For instance, << on a queue could mean "enqueue", and on a stack could mean "push". Everything you want could happen inside an instance_eval block: removing methods, adding new ones, and even changing some semantics. Here’s an example of a change in the semantics of << to have an array working as a set.

class Array
  def as_set
    instance_eval do
      undef :[], :[]=, :at, :first # and more

      def <<(object)
        super(object) unless include?(object)
      end
    end

    self
  end
end

This is just an experiment, the implementation is not complete but you might get an idea of the final goal.

Conclusions

We still have some antipatterns living in our daily languages and tools. But sometimes, as in Ruby, we have the power to modify the language and/or extend it to suit our needs. This is an option we don't usually consider (maybe for historical reasons, and most people say it's "bad" without strong arguments) but it's perfectly valid.
Metaprogramming is very helpful in this case, not to add new behavior but to "turn" the swiss army knife in a single, one-purpose tool.
If collection classes only have essential behavior, developers get more "educated" and they are "forced" to think which kind of collection they want and use it consistently. This would prevent some unexpected hacks to happen.

6 tips for a powerful TDD session

Nahuel Garbezza — Wed, 29 May 2019 23:32:11 +0000

(Original article at 10Pines blog here)

TDD is ~95% practice. The theory about it is really simple and you may already know the Red-Green-Refactor cycle and what to do in each step. Practice is harder, and it's always beneficial to have some guidance as you practice it. Here's a quick list of six things that work for me, and maybe can work for you.

Analysis Paralysis not allowed: Sometimes we think too much, and thinking is good but it's also time-consuming and it's better to spend the time building your program, rather than thinking how it should be done. Questions like "how do I call this test?", "is it better to create a separate object or not?" appear all the time. Those kind of questions are ones that you need to ask yourself after your test is green, not before. And maybe you haven't built enough to have an answer, so don't lose much time on them, TDD will guide you sooner or later to the answers.
Do not forget step 3: Sometimes we make a test pass and we go immediately to the next one. But step 3 (refactor) is an important step, because you can fix the tech debt you just left. It'll be harder to fix later. Check namings as well! And don't forget to refactor the tests. Think about the interest you can accumulate by leaving tech debt. Don't hurt your future self ;)
Assertions first: in the Act-Arrange-Assert pattern, I like to go from bottom to top. This will help you focus on getting what you want, and then writing all the context you need to make that happen. At the end, you will probably have the minimum context you need for the test. Simplicity is a big win here. Nobody wants to maintain tests with lots of set-up lines of code.
Break anxiety: Sometimes we have an idea of what our final solution will be, and we cannot resist to code it and then we break the TDD cycle (I confess! It happens to me every day). We lose the flow. We might be introducing a bug (we always tend to think that our solutions are unbreakable). Do not think that because of TDD, you are going slow. Think about the long-term benefits. Starting again the cycle after you broke it, it's also time-consuming. So it's better to go at a constant velocity, without taking "shortcuts". If you do a lot of cycles, you may probably notice that you have already accelerated it.
Leave edge cases to the end: Edge cases are usually hard to test, compared with the happy path. And they are not too valuable if you don't have the happy path already covered. Think about the future users of your system. What will be the first thing they will try to do? This should be your priority. Unless you are really sure about what you don't want to happen in your system.
Feel free to take another path if you are stuck: Let's say you are stuck in step 2 or even in step 1 trying to get a green test. I suggest that if you are more than 10/15 minutes stuck, you need to do something else to move on. Diverge (leaving the test unfinished and trying another) or rollback (deleting the current work and starting with another test) are both valid options. In TDD we are always following a path, and sometimes it is wise to take a step back and change the path.

In summary:

Do not let anything block you.
Do not miss opportunities to improve.
Start small and unperfect, and do small steps.

And you will rock!