code review
- Back in our UI, where we display the widget, we do not display the category.
- When we get a successful response for the request, we update the internal state of the app (we don’t use Redux). So we “save” the category.
- When we open the form the edit the widget, we fetch data from the API first and display that to edit.
- For the happy path test for a successful response and be done
- Add tests for the failure case of unsuccessful responses
When is the right time to break something?
Please excuse this sensational headline. I am referring to classes in your software and to their design. But also to tests, but see below…
Work
The past week I mainly tried to pair with my fellow developers to impart some knowledge onto them. We worked together on integration tests with Nightwatch.js which is something like Capybara from the Ruby world. A question that came up was “when do we stop testing?”
I have to share some details about our setup: We created a React application for the frontend and use the APIs provided by the backend developers. Those APIs are sometimes flaky. And generally it would be a bad idea to access them during the tests. This is because they would introduce a dependency in your tests that is outside of your control. Do you want to have your tests fail, because somebody changed something inside the APIs? And would you like your tests to succeed, regardless of whether the API was accessible or not?
The answer for both question is “yes”. The first type of test is a system test, where you do a complete blackbox-style test of your system for (mostly) functional requirements. If the API changes, you would want to know. And you would like to have your tests fail and notify you of that change. Well, you would want the backend dev to notify you beforehand, but still…
The second type of test I was describing is an integration test. You want to find out whether the code you wrote tries to access the API in the required way and whether your software behaves as specified for the API’s responses. But you don’t need the actual API for that. You should use a MOCK API that replaces the real API and behaves just the same. And this mocked API should be under your control.
So much for the details. Because we wrote integration tests against the mocked API. We sent a request to get data, let’s call it a widget and we specified it by its ID.
GET /widgets/123-567-890 => widget.json
We received JSON and displayed the widget in a form, so the user is able to edit it.
In this test, we edit a widget’s category only. We send a PUT request with the whole object, even though we only update the category (A PATCH would be enough, but this is what the API specified…). We send the request to our mocked API. Since this was the “happy path” testing, we received a response that indicated a success. 🎉
A bit more of background:
Now the question was: “Where do we stop testing?” Since we mock the successful response anyway, do we test that we update the internal state with the widget? But we don’t use or display that state (the updated category) anyway?!
I want to say, I had to think about this for a while. I wasn’t sure what the best way would be. One answer could’ve been to display the edited category somewhere accessible for the test (perhaps as a data-attribute). But where’s the business value in that?
A better answer turned out to be:
And the test and it’s questions revealed another thing: We do not need to store the updated category right now. The person who wrote that code did it with a possible future requirement in mind. But it wasn’t and isn’t necessary right now. But since they didn’t use TDD to create the code and its design, they didn’t notice.
So that’s one more history for “tests help increase the quality of code and design”.
This is already long and I wanted to share something on the size of classes and when to refactor them. I guess this has to wait for the next issue.
Talks/webinars
This topic from last week resonated with you. Thanks for the many responses. I will take you up on your offers and put you in a virtual crowd using Zoom, next time I am doing something like this.
Personal
Fixed gear bikes seem to be a favorite for many readers. I hadn’t anticipated that you would have experience riding these. I was glad I shared it.
On a completely unrelated note: In the future I will factor into my selection of clients whether they have a shower in their office or not. Having access to one helps me tremendously to combine my training plans into my working plans. Just yesterday I was able to do an intense interval running training on my way to my client because I knew I could shower afterwards. The alternative would have been to do it in the evening. But I’d rather spent the evening with my family instead. So this really increases my happiness.
Another personal endeavor of mine is to decrease my reliance on external/3rd party solutions for syncing data and storing it. This means stuff like iCloud and Dropbox but also other proprietary solutions inside apps that I use, e.g. DayOne.app
If I write my thoughts for years and years, I do not want to rely on some business to be able to access it. Same with my own business content. I mostly write code and articles, but also thoughts on strategy and lots of other things. I plan my business for the long run, which means I want to be able to access these things forever.
That’s why I begin to turn away from iCloud and friends. I don’t use Ulysses anymore for writing, but Joplin. I run my own cloud service (Nextcloud) and sync all my encrypted plain text files to the cloud and between my devices. That way I can tripple-back-up everything. Joplin is FOSS. I can save the source code in its current state (you bet I already did!) and will be able to access my notes for as long as I have a computer.
So I started migrating my data from the different apps I used to Joplin. This is an ongoing task and I also began to write an Importer to do this automatically. I have thousands of entries and notes in DayOne and other apps. It’s quite a challenge. But this makes sense to me.
External forces
I am occupied with learning these days. Learning on my own about visualizations of data among other topics. But also learning about learning. For that I read what other people think about learning. There are many things I have to learn about this whole topic. One thought I saw repeatedly, was about external forces, or limiting factors.
Let me elaborate what I mean by that: There are people that can motivate themselves more easily than others can. They reach their goals or at least try very hard. Others give up more easily when they face some resistance. As always, there are people in the middle between these extremes. You know best which group you belong to. 💪
What has this do with software quality? I am getting there… 😉
I am wondering how external forces could help improve quality. If you need to reach your goal and you don’t belong to the group of highly self-motivated people there are options like hiring a coach. Athletes do that all the time. I pay for a “virtual” coach that guides my running efforts.
How could you hire a “virtual” coach for your coding efforts, for reaching your targets on your software quality metrics? You could hire me or other “real” coaches, of course. But that doesn’t scale too well and might be too expensive.
Again, for some people it is easy enough to use static analysis or linting — a kind of coach in it’s own right — and follow their guidelines. Yet, still there are people that ignore the warnings or guidelines imposed upon them by the tools. Reasons may be a hard deadline or too much workload. How could we offer external forces, limiting factors that help them, guide them, towards doing the right thing?
One solution I can think of is to have a robot not accept your code when it is below standard or ignores guidelines. A robot could be anything that measures and grades your code and reports back to your team. Some tools already offer this, for example GitLab. If you want to merge code that decreases the overal quality metrics, you are not allowed to do so. So that would be one.
Another idea: If you try to commit or merge such code, you need to consult with another developer about the code. Once you worked on it together, the other dev has to enter her secret key, to remove the lock on the merge. This forces you to pair on code more often.
When it comes to teaching there is this saying of the “glass has to be empty (enough).” You cannot pour water into it, when it’s already filled. Said ideas 👆probably won’t work for a team that isn’t aiming for learning and improving.
I will continue to think.
Quick wins, part 4: YAGNI
We refactored some code yesterday, to move knowledge about an implementation of a function back to where it belonged: Into the function’s class and into the function itself.
Today I want to talk about another topic that often comes up when you are refactoring, or plainly “changing code.”
It happened the other day, during a workshop I was doing on software testing. The participant wanted to apply his new knowledge and write tests for a given JavaScript class. He had written the code a few weeks before we did the workshop. During the hands-on part of the workshop, he wanted to add tests, to make sure everything worked and to make sure that he understood what I had been talking about.
We were lucky in as so far that something happened that usually happens next: He noticed that his code was not easily testable. The design of his class made it harder for him than he would have liked. We talked about the problem, and he noticed the source of it. His class had too many responsibilities. He extracted the code in question into a new service and could then mock the new service when testing his original class. This was good. He was ecstatic. He made progress!
Having gained so much momentum, he went overboard: During his test-design, he wanted to be too clever and tried to write an elaborate test setup which was to be reused between different test runs. It was supposed to be a reusable, parameterizable do-it-all function that could setup the tests just right. With no duplication. In short, it was so much code with so much logic that it would’ve warranted its own tests. And the worst thing? It didn’t work and made trouble for writing his tests.
I was a bit thankful because that gave me the opportunity to tell him:
Premature optimization is the root of all evil.
Perhaps you’ve heard that saying already. Donald Knuth coined this phrase. There’s more to it, but that could be discussed in another email.
Back to my tester. After talking about the problems his function gave him and the difficulty in getting it right he settled for the simple solution: Write your tests. Accept duplication. Keep it simple, use copy & paste if it makes you faster and is more convenient. Write the tests you need and keep them green. And after all that, and only then, refactor your tests to remove duplication where applicable. Don’t try to write the perfect code from the start. Let the design evolve with the help of your tests. Don’t be afraid to make baby steps and don’t expect to have perfect code after the first try.
I hope you liked this series. Perhaps you could take something away from it. If you have questions, let me know.
Tomorrow I’ll be off; it’s a holiday where I live, and I’ll use it to spend time with my family. See you on Monday. 👋
Answering a comment about 'Delete all your tests'
Matthias Berth is a German expert on software delivery and software quality. He politely disagreed with me on the idea that you should delete all your tests.
I decided to call this day “Video Wednesday” and record an answer as a video. I just posted it on LinkedIn, and thought you might like to watch it there.
It even has subtitles 😉
Answering a comment about 'Delete all your tests'
Matthias Berth is a German expert on software delivery and software quality. He politely disagreed with me on the idea that you should delete all your tests.
I decided to call this day “Video Wednesday” and record an answer as a video. I just posted it on LinkedIn, and thought you might like to watch it there.
It even has subtitles 😉
2018 State of Code Review Study
81% of respondents who were satisfied with their code review process were also satisfied with the overall quality of their software. Respondents who were not satisfied with their code review process were half as likely to be satisfied in their overall software quality, with only 40% respectively.
This is from a research study done by SmartBear.
This image tells us that the majority things code review is the way to go to increase code quality. This might be true, or it might not. The quality of a review depends 100% on the knowledge, ability to communicate and the time a reviewer takes to dive into the code. Static analysis is way down in second to the last place. Unit testing is right behind code reviews.
I could argue either way. My problem with this study is the term code quality. Code quality to me means that you talk about things like coupling/cohesion, readability, and maintainability, adherence to standards, low bug count. Quality metrics your code exhibits. Code review is not the best tool to increase this metric. Robots and static analysis are. You need impartial tools that hold you to a strict standard. People don’t do this. They are lazy. If you talk about software quality, on the other hand, that’s where you need people. Thinking about and discussing software architecture and design, debating about usability. Fine-tuning the visual design of a product. This is where reviews are the go-to tool for the job.
I guess this distinction was unclear to most participants. That’s a shame.
You can read the study here. I don’t link to their signup form for the study. They ask details like your phone number before you can download it. That’s bad practice, hence the direct link. I’d be happy to hear your thoughts on this. Do you make a distinction between code and software?
2018 State of Code Review Study
81% of respondents who were satisfied with their code review process were also satisfied with the overall quality of their software. Respondents who were not satisfied with their code review process were half as likely to be satisfied in their overall software quality, with only 40% respectively.
This is from a research study done by SmartBear.
This image tells us that the majority things code review is the way to go to increase code quality. This might be true, or it might not. The quality of a review depends 100% on the knowledge, ability to communicate and the time a reviewer takes to dive into the code. Static analysis is way down in second to the last place. Unit testing is right behind code reviews.
I could argue either way. My problem with this study is the term code quality. Code quality to me means that you talk about things like coupling/cohesion, readability, and maintainability, adherence to standards, low bug count. Quality metrics your code exhibits. Code review is not the best tool to increase this metric. Robots and static analysis are. You need impartial tools that hold you to a strict standard. People don’t do this. They are lazy. If you talk about software quality, on the other hand, that’s where you need people. Thinking about and discussing software architecture and design, debating about usability. Fine-tuning the visual design of a product. This is where reviews are the go-to tool for the job.
I guess this distinction was unclear to most participants. That’s a shame.
You can read the study here. I don’t link to their signup form for the study. They ask details like your phone number before you can download it. That’s bad practice, hence the direct link. I’d be happy to hear your thoughts on this. Do you make a distinction between code and software?
A story about a feature gone bad
Chad clicked the button and created his pull request. He had worked on his feature really long — it must have been three weeks. Finally, it was time to integrate his changes into the master branch so they could go live with the new version of the app. This was the first huge feature that was his responsibility. His team lead, Janet, gave him the ticket for the task and he set out to write his code. “When it finally goes live,” he thought, “the churn of our users should drastically go down!”.
Primarily he had found many places in the app, where users committed some actions. Up until now, these actions weren’t registered anywhere, so nothing and nobody tracked them. There was no record of what the user did or didn’t do inside the app. Marketing alerted management that too many users did not renew their accounts, or outright canceled. In turn, management asked the developers to do something about it. Together the team decided to record all user actions and put them into a log of all activities for this user. This way they could make calculations which users were not active in the app and reach out to them to prevent them from abandoning the application, or so they hoped. The development of the feature enabled Chad to take a thorough look at the whole application. After all, he had to integrate his code into all kinds of places. And so he did. Today he was finally ready to publish the code and begin the merging. To integrate it into the master branch so that it could get deployed, he had to create a pull request. “Since my code is well written and worked when I tested it, it shouldn’t take too long for this merge to complete.” was his conviction. He assigned the pull request to his team lead and another backend developer that he had talked to during lunch a few days earlier. Chad used the lunch to tell her about his progress on the feature, and she seemed interested. So another set of eyes shouldn’t hurt.