(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
We can all agree that dead code should be removed. What we sometimes fail to see is how much it could cost to leave dead code with our product. Today’s article is about an example of a “dead code” and how it hit us back 1.5 years after we stopped using it. It took us 3 hours to debug and find the root cause. The conclusion is that we should have done ‘git rm’ an year ago. We did not and the cost was 3 hours.
This is a real life example article from our code base and it is design to share experience with our team, but I hope the whole community could benefit.
What happened
1. Jenkins build failed
First time we saw it we identified that it is not something serious and it kept failing for a few days until we had time to address it in the next Sprint.
2. Teaspoon, Jasmine, Bundler, Rails Engines
In the project there was the production code in Rails.root along with a dummy test code for starting JavaScript specs in Rails.root/test/dummy/. We are using Teaspoon with Jasmine in a Rails engines. Rails engines tend to create a test/dummy folder in which you could put the code for a test app in which you want your JavaScript specs to be executed. The product code is loaded in the test app and specs are executed.
About 1.5 years ago I did some experiments with this test app and made a few configurations. The goal was to have the production code along with some test app code both loaded in the test environment. In this way Teaspoon and Jasmine would see both the production and the test app code as coming from “production”. I literally remembered what my idea was and what I was trying to achieve. At the end I decided not to use the test app code, but to have only the production code located in Rails.root be loaded in the test environment.
But I forgot to remove the test app code from test/dummy/app/assets/javascripts/
3. Teaspoon Error
The following error occurred. Here “IS” is a JavaScript namespace part of the production code.
jenkins@vpszap6s:~/jobs/is-core Build and Release/workspace/test/dummy$ xvfb-run -a bundle exec rake teaspoon --verbose
Starting the Teaspoon server...
Teaspoon running default suite at http://127.0.0.1:36051/teaspoon/default
ReferenceError: IS is not defined
4. The error and debugging process
Not sure what the error was exactly I tried to identify the difference between my machine and the server machine where the tests was failing. This server machine is called “elvis”.
Bundler version – my version was 2.2.11 and the elvis version was 2.2.14. Tried with upgrade, but it did not resolve the issue.
Chrome driver – since the tests were executed on chrome I saw the chrome driver versions were different. Mine was 86. elvis was with 89. Synced them, the error was still occurring.
Rake version – rake versions were the same
Ruby version – the same
Teaspoon and Jasmine versions – the same
The OS is the same
I could not find any difference between my env and the elvis env.
Turns out the problem was in the loading order. elvis was loading test app code and then production code. My machine was loading production code and then test app code. I could not figure out why. The whole test app code was:
I was curious to continue debugging to see why the two machines were loading the code in a different order, but at the end decided against it. Just removed the dead code in test/dummy/app/assets/javascripts/ext/dummy/dummy.js which I only thought as dead, but it turned out it was affecting our project.
6. Builds passing
Finally the builds were passing.
Conclusion
Dead code my not be that dead after all. It might be loaded even if not used and loading order could differ. Better be safe and remove dead code at all. If we need it that much, we have it in the GIT repo history.
In today’s article I am sharing with our team an example I found while reading parts of the code in one of our platforms where we’ve created a form of ‘cyclic’ references with cancancan merges in Rails Controllers. I wondered for a while how we’ve managed to create it, how to avoid creating them in the future and I share this as an example with our team, but I hope the article could be useful for the whole community.
CourseSectionsAbilities is merged with ContentsAbility which is merged with ContentRefsAbilities which is then merged with CourseSectionAbilities. This means that CourseSectionsAbilities is merged twice and the second merge overrides the first merge.
class CourseSectionsController < CommonController
def current_ability
# This is the first merge
# CoruseSectionsAbility is merged with ContentsAbility.
@current_ability ||= Abilities::CourseSectionsAbility.new(current_user)
.merge(Abilities::ContentsAbility.new(current_user))
end
end
module Abilities
class ContentsAbility
include CanCan::Ability
def initialize user
...
# This is the second merge
# ContentsAbility is merged with ContentRefs ability
merge ContentRefsAbility.new user
end
end
end
module Abilities
class ContentRefsAbility
include CanCan::Ability
def initialize user
# This is the third merge
# ContentRefsAbility is merged with CourseSectionsAbilily.
# This overrides the first merge.
merge CourseSectionsAbility.new(user)
...
end
end
end
How did it happen?
It think it is the classic grow of the code where you solve the problem in the easiest way possible. Initially we had:
class CourseSectionsController < CommonController
def current_ability
# This is the first merge
# CoruseSectionsAbility is merged with ContentsAbility.
@current_ability ||= Abilities::CourseSectionsAbility.new(current_user)
.merge(Abilities::ContentsAbility.new(current_user))
end
end
But then a new requirement about an year after that has come and there is a commit that adds the second merge:
module Abilities
class ContentRefsAbility
include CanCan::Ability
def initialize user
# This is the third merge
# ContentRefsAbility is merged with CourseSectionsAbilily.
# This overrides the first merge.
merge CourseSectionsAbility.new(user)
...
end
end
end
What is the problem?
The problem is that there is a class called ContentRefsAbility that can dependent on everything it wants. It can merge anything inside it with any consideration on what was already merged. It can set it’s own dependencies. This couples the ContentRefsAbility with the places it is used. Because we must take into consideration every place where ContentRefsAbility is used before changing it’s implementation.
How Dependency Injection solves this?
We pass the ability in the constructor
module Abilities
class ContentRefsAbility
include CanCan::Ability
def initialize user, outside_ability
# This is the third merge
# ContentRefsAbility is merged with CourseSectionsAbilily.
# This overrides the first merge.
outside_ability.method1 # we call the method of the outside_ability
...
end
end
end
Instead of creating the ability in the class we pass the dependency from the outside. In this way we can control the dependency and choose different dependencies in different conditions.
The ContentRefsAbility no longer depends on the specific implementation of outside ability, but it depends on the behavior we inject from the outside.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
Today we made a code review on a feature. Generally the review process in our team is – someone drops a message in the slack channel and says – “hey, I have these changes. Can you please take a look at them?”. We do not require a merge from an “authority” to get something to production. We also have one rule we are trying to follow:
Your commit should make the product better than it previously was.
The commit below did not make the code better and this is the article about it. I am sure it would be useful for all of our team members and I hope it will be useful for the community as a whole.
How “if statements” are a sign of “code smell”
This is the commit. Do you see the problem with it?
The logic in the onStepsTreeLoaded method has significantly changed. It was a simple initialization of a private variable. Now it is an if statement with an else that sets the variable used in the if to a false value.
Wow. This is even difficult to explain.
Why was the change introduced and how the “code smell” helped us improve?
The thing is that our colleague had to do this change to keep the behavior of the code based on a commit from 7 months ago. But now we see this smelly code and we thought:
Why is this even needed in the first place? Why do we call this onStepsTreeLoaded method and what is it doing for us?
Turns out that we can just move the initialization from onStepsTreeLoaded method to another method called at a different place and we can delete this method. We would keep the same behavior. There will be no regressions. The framework has changed in the last few months to the point that there is now a better place for this initialization to happen.
My point is: “if statement”==”code smell”
Adding an if statement to a working code is probably a code smell. Wrapping an existing code in an if statement based on unrelated state with an else that sets this same state is probably the precise definition of what “code smell” is.
Conclusion
Revise your assumptions. Don’t add the if statements. Think again if this is really needed, why it is needed and where is its place in the architecture of the platform.
The concerned emotion
Can’t think of a better way to show you the emotion of code smell than to show you this Gorilla.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
Today on a review I saw an RSpec Matcher that was more complex than it should be. We’ve discussed it internally. I think this article could be useful to our team and to the community as a whole.
An RSpec Matcher should be simple. It should return just ‘true’ or ‘false’
IRL example of a complex RSpec Matcher
This is the real live example of an RSpec Matcher from todays review
RSpec::Matchers.define :have_content_picture_type_set_to do |picture_ref, content_picture_type, content_object|
match do |page|
content_object_undersore = content_object.model_name.to_s.underscore
content_object_undersore = "tutorial" if content_object_undersore == "episode"
content_object_undersore = "courses/#{content_object.course.to_param}/section" if content_object_undersore == "course_section"
picture_ref_row = page.find(:xpath, "//tr[.//img[@src='#{picture_ref.content_picture.image_url}']]")
js_agnostic_click_table_action picture_ref.to_param, "/author/#{content_object_undersore}s/#{content_object.to_param}/pictures/#{picture_ref.to_param}/edit", "Edit Picture"
page.select content_picture_type, from: "content_picture_ref[content_picture_type]"
click_on_and_expect "Update Picture", "Picture successfully updated"
expect(page).to have_picture_ref_with_content_picture_type picture_ref, content_picture_type
end
failure_message do |page|
"expected page to have content_picture with the specified type, but was #{page.body}"
end
end
There is a lot to unpack here.
We do some calculations of paths with the whole content_object_underscore logic
Then we find something on the page with the page.find method
Then we call ‘js_agnostic_click_table_action‘ which is a complex method
Then we select something from an HTML select
Then we click a button with the click_on_and_expect
At the end we expect something
We should have only step 6. That’s the purpose of an RSpec Matcher. Return true or false if the page contains something or not.
Why has this happened?
Because RSpec.define :matcher is a function like any other ruby function. And as a ruby function you can do whatever you want with it. You can call anything you want in this function.
Why should an RSpec Matcher be simple?
That’s the convention for RSpec Matchers. Imagine the following spec:
visit_pictures material
expect(page).to have_picture_ref_with_content_picture_type material.content_picture_refs.first, "Thumbnail"
With this spec you would expect visit a page and check if there is something on the page. But now have_pictuer_ref_with_content_picture_type makes calls, changes the db, modifies the current page and there is no way for you to understand it from reading the spec. Let alone reuse the Matcher in another situation.
What feature is this about?
The feature above is about showing a picture on the animated 3D models and building instructions that we show at BuildIn3D. Authors could choose which picture is the Thumbnail – example is this FabBrix Hen :).
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
I think calling a method thrее times is redundant. But then again, you have to balance. Today’s article is about a code review that at the end took like a few hours in total of different discussions and I believe it is important. This kind of things take time. Failed builds, difficult specs.
The IRL example – Where is the redundancy? Is this DRY?
This piece of code (along with a few other changes in the commit) were the root of a 2 hours discussion in the team. A spec failed because some things were int while we were expecting them to be float.
Calling ‘float’ three times like this is redundant
The reason I think like this is that if you have to change and would like to have a double value or an int value you would have to change the code in three places.
Probably e better solution would be:
+ if line_args[2] == "END":
+ rostep_params = {
+ "rot_x": LdrawLine.default_x,
+ "rot_y": LdrawLine.default_y,
+ "rot_z": LdrawLine.default_z,
+ "type": "ABS"
+ }
+ else:
+ rostep_params = {
+ "rot_x": line_args[2],
+ "rot_y": line_args[3],
+ "rot_z": line_args[4]
+ }
# We are adding a loop to call the float
+ for key in ["rot_x", "rot_y", "rot_z"]:
+ rotstep_params[key] = float(rotstep_params[key])
We call float only once at a single place. Now we have to deal with the fact that we have “rot_x” as a variable in two places, yes, that is true, but this could easily be extracted and we can iterate over the rostep_params values. But now we have consistency.
Is it harder to read? Probably it is a little harder. Instead of simple statements you now have a loop. So you lose something, but you gain something. A float function that is called at a single place.
What are we doing with these rotsteps?
ROTSTEP is a command in the LDR format. We support LDR for 3D building instructions. Here is one example with a FabBrix Monster that uses the LDR ROTSTEP as a command:
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
An ugly, sometimes over neglected truth of the industry is that the software that we work on does not become unmaintainable and tedious and difficult to work with overnight. It becomes like this with every change we introduce. While each individual change might not be “that bad” when they pile up we no longer have a well decoupled, maintainable system. We end up with a mess, which we than re-write with a new framework, a new team, new concepts and architecture just trying to do better. But the problem most of the time is in the single change that we do today – is it making the software system better or worse?
This article is about a practical example of today and how we got to stop it.
To better or worse
When developing software incrementally we introduce changes. There are basically two options – the new changes would make the system better or they will make it worse.
The logic is not important. We basically get the video for a material. What’s important is the code is practically the same in all 3 places. 4 calls in three files and they are all the same.
In the past I was leading a class in Software Development. One thing I tried to teach each student was:
When you copy and paste you introduce a bug. That’s a fact of the industry.
The reason is that once you copy/paste you would have to support the same logic in more than one place and you would simply forget about the second place the next time you would like to change the logic. It might not be you, it might be colleagues working years from now on the same code, but they will forget to change both places. That’s the problem with redundancy.
Remove the redundancy, and you remove most of the bugs
How did we stop it?
Simple review. Just ask a second person to check your commit. This simple review process protects you from a large portion of the other bugs that are still left after you’ve cleared all the redundancies (of course).
Enjoy
The code in question is part of the logic delivering these instructions. See how the video is displayed. That’s what the feature was about. Enjoy.
(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)
We’ve designed, developed from scratch and are running an event-driven plugin based browser framework that we call Instructions Steps (IS). It helps us visualize 3D models and building instructions on the BuildIn3D and the FLLCasts platforms. Currently it consists of hundreds of extensions separated in about ~50 repos with 587 releases. We’ve figured out a way to keep the quality of the whole framework at a really good level with almost no bugs and errors.
This article is about how we are doing. The main purpose is to give an overview for newcomers to our team, but I hope the developers community could benefit from it as a whole.
The IS architecture (for context)
I will enter into the details about the architecture of IS in another article. For this article it is enough to say that IS consists of a really small core – 804 lines of code and a lot of extensions.
There are many extension that are extending the framework. Most of them are under 200 lines. The framework is highly decoupled and “everything is an extension”. Look at the 3D model and building instruction below. The “previous” button is an extension. The “next” button is an extension. You could have “parts list”, “bill of materials”, play animations, fit and rotate the camera. These are all extensions. I took this idea for the way we were building plugins for Eclipse (many years ago).
What is the problem with quality and how do we keep delivering a quality product?
Event-driven plugin based architectures have many advantages – like decoupling the plugins which makes them more maintainable. It forces you to have clear API boundaries which also makes them more maintainable. But there are a lot of questions and drawbacks compared to a nice Monolith app. What should we test? Should we test a specific extension, or the repo or the extension as it is working with all of its direct dependencies. What kind of specs should we develop? Should we have small unit spec that tests the extension in an isolation or we should put all the hundreds of extensions and test them all together. How do we make make these decisions?
Here are the few simple rules that we try to follow.
Compilation and type checking with Google Closure Compiler in ADVANCE mode
We use vanilla JavaScript. No TypeScript. There are reasons for this. Nothing against TypeScript actually. We use Google Closure Compiler to compile each and every extension.
Here is an example of a declaration of an “interface method”
/**
* Loads the given url and returns a Promise that when resolved will provide the caller with a {@link IS.StepsTree.StepData}.
*
* @export
* @param {string|File} file - url to the file or a DOM File object to be loaded
* @return {Promise} Promise that when resolved will provide a {@link IS.StepsTree.StepData} which is the root step
*/
IS.StepsTree.IProvider.prototype.getStepsTree = function(file) {};
Looking at the code we have the jsdoc annotations like “@param”, “@return”, “@export”. These are annotations that GCC understands and will check. It will check if the param is of the given type, it will check if the returned value is of the given type. It will check if the classes that implement this interface actually implement it.
Google Closure Compiler (GCC) will check if we are trying to access properties and methods that are not available.
As a general rule of thumb – compilers are strict. If they understand your code, and compile it, then your code fulfills a bare minimum of requirements.
Each extension is tested in isolation only with its direct dependencies available
The navigation extensions are located in the repo “is-navigation”. When we test the functionality of the “Next” button we don’t expect to also have the “Fullscreen” or the “Animations” extensions available.
Each extension is tested automatically in isolation, because each extension should work on its own given that it is the only extension that is installed (and the direct dependencies of course). Which makes sense. We are building a framework, a platform. When we have a framework, platform or even OS we should be able to install one extension, app, or program and they should be able to work on their own.
All extensions are tested together in the ‘release_pack’
What teams building platforms and frameworks quickly find out is that all the extensions and apps can work separately, but there are a lot of cases where if you put them all together and install them, things start to get more difficult. An example are all the different problems different OS have. One program is affecting another program in an unpredictable way.
So we’ve build the is-release_pack. What it does is to put all the extensions together and to run a few basic tests on all of them.
It contains 1-2 specs that check that each extension is working in the general case and probably one or two very specific cases. All the other specific cases are tested in the extensions, not in the “release_pack”. We push everything we can to the specs of the specific extension, but we have a few “integration” specs that are in the is-release_pack. And it is beautiful.
The downside of integration specs
There is one major downside with integration specs:
All of us, developers, are lazy when it comes to really building it right. Once we build the feature and we see that it is working after a day of work there is little motivation in us to spend the next 3 days on building it right. It just feels so go to have it working that you commit and move on to the next thing.
When there is a problem and an integration spec is failing most of the time it is easier to go and “ease” the integration spec. Change the expects a bit. Modify them. Even remove them.
Other times when we have to develop a specific spec for the specific extension it feels easier to develop an integration spec instead of 20 specific specs in the repo for the extension.
Sooner or later you end up in one of these situations:
There are no integration specs or they contain expects and assertions that can not validate that your product is working correctly.
There are a ton of integration specs that are constantly and randomly failing from time to time. The “integration” specs suite also takes forever to pass as there are now so many “integration specs”.
Both of this situations are highly undesirable.
Resolving the downside of integration specs
One thing I learn writing business plans when applying for different VC funding is RACI.
There are people Responsible for the job, people Accountable for the job, people that could be Consulted and people that should be kept Informed.
So who is Accountable for the delivery of the IS framework and for the framework working correctly with all extensions in the user browser?
Ideally it should be one person. In our case – it is Me.
We are all responsible for the implementation. But in a team one should be held Accountable if something is not working and not right. One is Accountable for not checking. This person could change of course, but at any given moment there is someone “starting the engine of the car” as it exists the factory. You should start the engine and make sure the car works. You are accountable for checking it. You might not be responsible if it does not start, but you are accountable for checking.
With the is-release_pack we resolved this for us.
Only the Accountable (Me in our case) has access to the is-release_pack and its specs. Nobody else. You can not add integration specs, you can not remove, you can not even change on your own. The person that is accountable should do it. We keep the number of specs to a mininum – one basic scenario for each extension and when appropriate 1-2 (but no more) very specific scenarios for each extension. In the release_pack we prefer to has scenarios that involve more than one extension. In fact if there is a scenario that involves all the extensions we would probably use it.
Integration specs are coupling the extensions?
Yes. They are. When one extension fails the integration spec for all the extensions fail. That is true. With hundreds of extensions if every day a different extension “fails” then you will not have a successful run of the integration suite in years.
But the customer “does not care”. The integration spec is the closes spec to the customer experience. The users never interacts with a single extension. They interact with all extensions.
In the same time if an extension has reached the release_pack and is failing the release pack , we will go and add a new spec, but not in the release_pack. We add it into the repo for the specific extension. This protects us from regressions.
Conclusion
By having a small subset of integration specs in a project to which only I have access and that is the final step in the release pipeline we’ve managed to stop hundreds of releases that would break existing clients, would lose a feature or introduce a bug.
587 official releases already and it takes 5 to 10 minutes to release the whole framework. Integration specs are present in the release_pack, but we keep them to a minimum, each testing many extensions at once and making sure that a real life client scenario is working.
Reply
You must be logged in to post a comment.