DEV Community

Krste Šižgorić
Krste Šižgorić

Posted on • Originally published at betterprogramming.pub on

A Journey Through Anti-Patterns and Code Smells

To understand today’s best practices, we should understand the journey that led to them. And why they could change in the future.


Photo by Dino Reichmuth on Unsplash

I must admit that I was really lucky to be able to work on a variety of projects that gave me somewhat unique insights into the software development process. From doing greenfield projects, to maintaining and expanding two decades old ones. Doing short 3-month projects or working on the same project multiple years. Working backend, frontend, and desktop development. Sometimes I worked on products, and sometimes it was tools or libraries.

And I did not use only one technology. My language of choice is C# in which I did(do) most of my work, but I did use some other languages like JavaScript, PHP, Ruby, or Java. Back then the changes within the languages themselves and frameworks were not so drastic, so it was easier to be good in multiple ones. Today this is much more difficult because languages are changing rapidly, including what is considered best practices. But some fundamental knowledge stays the same. And I would like to share some of my conclusions that could be placed in that category.

The Beginning

Most web-based projects are based on MVC pattern. In this context everything related to data is planned to be part of the Model (M), representation part of the View (V), and business logic part of the Controller (C). Or at least this was the way it was implemented in many projects that I worked on. I followed this pattern and loved it.

If new action was needed on the system, I would create a new endpoint and implement it. If functionality was big, the controller method was also big. Each method would be one unit that represented everything that action was doing. If that action needed to be changed, well… everything was in one place.

It did not take me long to realize that this is not the best way to do things. Most actions in the system are not independent. One action would do the same thing as the other, just a little bit differently. If something needed to be changed in business logic, it needed to be changed in multiple places.

The problem would go even further than that. Sometimes I would implement already existing logic because I did not know it existed in some other method. This led to the fact that one thing, which should be consistent system wide, would be implemented and used in several different ways. This was not good.

I started to extract that duplicated logic into separate methods. Logically, inside the same controller. This way I could reuse existing logic inside different method. But only inside that controller. Eventually, I did find myself in a situation where I needed to use one controller inside another controller. Solution to this was to push the method further down, to the base controller class. Now every controller had access to all that business logic, and I could choose what to use and where.

The base controller started to get big and it was doing a lot of things. Naming methods became difficult because the names would collide with existing ones. This was no longer sustainable. The final straw was a situation where I put the wrong access modifier on a method. Instead of making the method protected I set it to public. The consequence of this was that each controller got an additional endpoint, and the Access Control List used on the system generated rights for each of those endpoints. It made me think if the base controller is the best place for those methods. It was time for a change.

I Need Help(er)

So how to fix this problem? I started to create a type of classes that helped me reuse the same logic in multiple places — helpers. It was a great solution, because I could reuse logic without fear that some helper would grow big. Splitting a helper class was easy. If something needs to be done in multiple places… put it into a helper. So if you are working with dates… use DateHelper. Done.

There was a lot of logic that was delegated to the helpers. Controller methods now looked much leaner and more comprehensible. Changes were easier and the system was more stable and consistent.

Now everything looked like it could be placed in helper classes. If something was used twice, I would move it there. There was not only DateHelper kind of stuff. There was AccountHelper, MatchHelper or BetHelper, depending on what I was working on. And that’s where the problems started again.

I was not working alone. If someone else was working on a new feature, he or she might not know that some logic is already implemented and could be found inside the helper. Sometimes I was not aware that I already implemented some logic, ending up re-implementing that same small feature yet again inside the controller.

And all the same problems as with the controller start resurfacing again. I needed a method that was already implemented, but only a little bit different. I needed one helper inside another. And then some more. In an attempt to reuse logic I ended up using an existing method that directly called resources (database, disc, or external API). This resulted in bad performance, because external resources in some cases were called in for/foreach loop. Those calls could have been done much more efficiently if the same methods had not been reused.

I ended up dividing the helper into two categories: true helpers and business logic helpers. True helpers were kind of helpers that were composed of what today is called pure functions. Method that receives input, does some logic, and returns the result. Those helpers were OK, and I never had a problem with them. I kept them in the same form as they were. But business helpers needed to change. I once again started looking for a different place to put that business logic.

Modeling solution

Business logic placed inside business helpers was always related to M part of MVC — the model. And models are accessible within the whole system. Then why use helpers (that you need to know about) when you can deliver data and business logic together, within the model? I started to push logic from business helpers to models. Although there was some improvement, all the same problems started appearing once again. As well as logical solutions for them.

If you are not careful, you could still change the model directly instead using methods that have some common logic for that part. This was fixed with encapsulation. If you prevent model from being changed from outside, you can force developers to use intended methods.

Another thing was that I still had problems with performance. By pushing dependencies inside the model, I could still end up calling external resources inside the for/foreach loop. But if I call an external resource inside the controller, and pass the result to the model I have much more flexibility in utilizing external resources. Calling external resources multiple times is expensive compared to calling it once and doing the work inside memory.

Without dependencies, the model was now containing only logic for changing itself and interacting with other models. This fixed the problem with needing the same, but slightly different method. Business logic was placed inside the controller where it belongs, and behavior for instance of the model was placed inside the model itself.

I could have still pushed dependency inside the model, as a parameter of a method, when some complex behavior needed to be executed on that instance. But I choose not to do that. There was no strict reason for this, but I just had a feeling that eventually this will lead toward pushing inappropriate logic within the model.

A Step Further

But there were still some things that could be improved. When some action is executed, there is often a set of additional commands that need to be executed as well. Doing this manually after every action, inevitably leads to long methods and hard-to-maintain code. This could be replaced with an event that will trigger the parallel execution of those commands. This way if additional behavior needs to be added it can simply be done by adding another event listener/handler.

This could be difficult to imagine without an example, so let’s try with this one. One of the features of some system I was working on was that users can be part of the group. That was not the main functionality, but it was important. Each user could have been only in one group, and each group could have one, and only one, leader. Based on this, user could have access to some resources and permissions to do some actions.

When a user was added to a group, this logic should have been updated to reflect those rules. When the leader was changed inside the group, again, this logic should have been updated to reflect these rules. The same was for the case that the user is deleted/deactivated.

But that was not the only thing that needed to be done. Each group had some expenses and was treated as a cost center. Each user had an additional cost. For each user there should be calculated direct (user cost) and indirect cost (group cost divided by number of users). So, if a user was added, demoted, promoted, or removed from the group it should have been recorded.

Those two features were not directly related, but the cost center was dependent on group administration in one simple way. It needed information that something happened, an event, so it can do its own thing. There is no need for the administration of the group to know anything about costs of the group, or even of their existence.

With time there was a need to add history of changes for group administration. Again, the administration of the group should not care about this. New event handler was added, and an audit was created.

And last, but not least, new feature was a list of notifications on user’s dashboard. Again, in case of group changes all users inside the group should get their notification about this event. Once again, there was no need that process of administrating group needed to know about this. New event handler was processing new business logic that was independent. It did not touch (or potentially break) group administration, cost centers logic, nor audit.

All of this made this amount of business logic more comprehensive and easier to maintain and extend. To ensure that all of this is done atomically (everything succeeded or nothing succeeded) I wrapped the whole process in a transaction. This ensures that data saved in the database is valid and consistent.

Conclusion

A good part of what I know about programming I learned the hard way. That helped me understand why some things are done the way they are done. Over the years I started to learn from other people’s mistakes, but this transition from Anemic Model toward Rich Domain Model is not one of them.

There are always exceptions to the rule, but these are my ideas for developing software. I picked them up over time, and I use them as a guidance:

  • Write code as you are writing an email, descriptive and straightforward. Do not mix multiple topics inside one “action”. Separate reads from writes to simplify things. Eventually you will read your own code and will need to understand it quickly. Do yourself a favor and write it that way.
  • Never call the same type of classes from that type (controller inside controller, service inside service, handler inside handler, events inside events). Business logic that needs to be shared between services should be pushed into entities, placed in manager/processor/helper classes (to mark the difference from services), or delegated to in-process/out-of-the-process handlers. This prevents circular dependency and keeps logic lean.
  • Keep business logic as lean as possible. If you need to dig deep to find out what one “action” on the system is doing then something went horribly wrong. You will end up with bad performance and unwanted behavior/side effects.
  • Be consistent. If you do things a certain way, do them that way everywhere. Sooner or later, someone will need to make changes to the system, and it will be much easier if they (or you) don’t have to learn how things are done for each feature.
  • Minimize who can do what. Encapsulate as much as possible and have as little as possible access points. This minimized breaking points. And if something is not working as expected it is easier to test and locate it this way.
  • Split your system into subsystems if possible. Smaller parts are much easier to maintain than one big whole.
  • If there is an exception to the rule, always comment on it well and make it as visible and as obvious as possible. This will prevent a lot of confusion later.
  • Think for yourself, question authority. It does not matter who said something. If for a particular situation it doesn’t pass a sanity check or test of time, it should not be used.

P.S. To prevent this article from becoming a small book I threw out some (a lot of) parts. Some of them are moving from MVC toward Layers architecture, getting circular dependencies with services, and ending up with some version of Layered architecture that is similar to Onion architecture. Point of this article is to present how I was moving the placement of logic in an attempt to increase maintainability of software I was working on. And all the most important points that led me to today’s way of thinking about software development are listed within this article.


Top comments (0)