{"id":13,"date":"2018-07-31T10:00:00","date_gmt":"2018-07-31T10:00:00","guid":{"rendered":""},"modified":"2023-11-17T16:40:03","modified_gmt":"2023-11-17T16:40:03","slug":"lesson-learned-code-regression","status":"publish","type":"post","link":"https:\/\/ahm.basfinans.com\/index.php\/2018\/07\/31\/lesson-learned-code-regression\/","title":{"rendered":"Lesson Learned: Code Regression"},"content":{"rendered":"
\n

<\/p>\n

\nGiving advice is not very effective most of the time. Sharing real experiences and trouble and how it is handled is more effective. The best is whoever learns from others mistakes. So I am sharing here a story where I and my team introduced a bug and it was leaked to the production environment. I know most of the advice here when I was consulting people in Agile but when the situation changed, I did that same mistake. The shame was painful and hence<\/span><\/span> the learning was intense. Experience is where you really learn, not advice from ivory towers.<\/div>\n
\n
<\/span><\/span><\/div>\n
\nI am sharing here hoping that it will be useful for myself and for others.<\/span><\/span><\/div>\n
\n
<\/span><\/span><\/div>\n
\nLook at this code fragment that is a part of timezone based message broadcast.<\/span><\/span><\/div>\n
\n<\/span><\/span><\/div>\n
\n    <\/span>private String timezoneMetadataId = “23467fe6523310”;<\/span><\/span><\/div>\n
\n       <\/span><\/span><\/div>\n
\n       …<\/span><\/span><\/div>\n
\n
<\/span><\/div>\n
\n     <\/span>\/\/Check Metadata for Timezone<\/span><\/span><\/div>\n
\n    <\/span>List subTimezoneValue = sub.getSubscriberMetaData().get(timezoneMetadataId);<\/metadatavalue><\/span><\/span><\/div>\n
\n    <\/span>for (String timezone : timezones.split(“[;,]”)) {<\/span><\/span><\/div>\n
\n        <\/span>for (MetadataValue mv : subTimezoneValue) {<\/span><\/span><\/div>\n
\n            <\/span>\/\/Do some processing here<\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\n        <\/span>}<\/span><\/span><\/div>\n
\n    <\/span>}<\/span><\/span><\/div>\n
\n<\/div>\n
\n<\/span>
<\/span><\/div>\n
\nThe question here: Is timezoneMetadataId acceptable to be null? In our case, it must not be null. <\/span><\/span>The initialization above ensures it is not null.<\/span><\/div>\n
\n<\/div>\n
\nUntil now, everything is ok, but the hardcoded timezoneMetadataId is not recommended especially as it is repeated in many places in the source code.<\/span><\/span><\/div>\n
\n<\/div>\n
\nWe wanted to change this hardcoded id and place it inside the configuration file. As we use Guice dependency injection, we changed the above code to be:<\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\n<\/div>\n
\n    <\/span><\/span>@Named(value = “mongo.wmprod.subscriber.metadata.timezoneId”)<\/span><\/span><\/span><\/div>\n
\n    <\/span>private String timezoneMetadataId;<\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\n<\/div>\n
\nAnd here we made the mistake of forgetting the <\/span>@Inject <\/span>annotations which meant the @Named annotation has no effect and the variable, timezoneMetadataId is initiated to null. Sadly the above for loop code will not raise any errors as getting the value of key null is an empty list and looping over the empty list is ok.<\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\nThis is a code regression. The changes that intend to make the code better ended injected a critical bug.<\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\nWe have a Code Review process, the reviewer did not notice the mistake. Maybe because it was buried into a broad set of changed lines. <\/div>\n
\n<\/div>\n
\nQA also did their own testing, they did not run the timezone based broadcast test case. <\/div>\n
\n<\/div>\n
\nHere I am sharing lessons learned to avoid such regressions in the future:<\/div>\n
\n<\/div>\n
\n<\/div>\n
\nLessons Learned for Programming:<\/span><\/div>\n
\n<\/div>\n
\nYou are changing the code. Do not be fooled by the simplicity of your changes or assume safety. Follow proven programming practices all the time.<\/div>\n
\n<\/div>\n
\n1) Verify value is not null<\/b><\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\nAll configuration items must be verified. See below:<\/span><\/div>\n
\n
<\/span><\/div>\n
\nimport com.google.common.base.Preconditions;<\/span><\/span><\/div>\n
\nPreconditions.checkNotNull(timezoneMetadataId);<\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\nYou can also verify the size of the key or the validity of the key. <\/span>Fail Fast<\/a> is your best friend.<\/span><\/div>\n
\n<\/div>\n
\n2) Add log statement that shows the id.<\/b><\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\nlogger.info(“Sending broadcast with timezone metadata id: {}”, timezoneMetadataId);<\/span><\/span><\/div>\n
\n<\/div>\n
\nIf the value is invalid in some way, you can check it in the log files.<\/div>\n
\n
<\/span><\/div>\n
\n
<\/span><\/span><\/div>\n
\n3) Write a unit test that ensures failure in case of null or invalid timezone metadata.<\/b><\/span><\/span><\/div>\n
\n<\/div>\n
\nActually, this will ensure that your preconditions are really working. It is indeed easy to make mistakes. Ensuring your preconditions and verifications are working is essential. If the project has no unit tests at all, create a task to create your first unit test immediately. <\/div>\n
\n<\/div>\n
\n<\/span>
<\/span><\/div>\n
\n4) Write integration tests. <\/b><\/span><\/span><\/div>\n
\n<\/span>
<\/span><\/div>\n
\nIn this project codebase, there was no integration testing framework in effect. If you join such team, create a task to create your first integration test in your first employment week.<\/span><\/div>\n
\n
<\/span>
\n5) Use Constructor Injection<\/b><\/span>
\nSee https:\/\/github.com\/google\/guice\/wiki\/Injections<\/span><\/div>\n
\n<\/div>\n
\nLessons Learned for Code Review:<\/span><\/div>\n
\n<\/div>\n
\n<\/span>
<\/span><\/div>\n
\nWe did not have a written checklist for code review. This is a mistake. Even if you join a team that does not have a checklist, do not wait until you have team experiences to create your own list. Conduct a meeting and start with a simple checklist immediately. With each Story or Bug Code Review, the checklist should be filled and attached to the Story or Bug. The checklist template should be updated frequently.<\/span><\/div>\n
\n
<\/span><\/div>\n
\nWe took Code Review seriously but with no structure. The above ideas are to make it serious process with explicit checks and responsibility. <\/span><\/div>\n
\n
<\/span><\/div>\n
\nLessons Learned for QA:<\/span><\/div>\n
\n
<\/span><\/div>\n
\nWe kept assuring the safety of changes to Project Leader and QA. This is really a mistake. Fears should be reduced by adding more test cases not by getting developers assurance about the safety of the changes.<\/span><\/div>\n
\n
<\/span><\/div>\n
\nI find it necessary to list all changed functionality to QA. It is even better for QA to have access to changes themselves. Once QA read source code nouns such as variables names, functions names, and class names, they can associate it to UI functionality and create the corresponding test cases.<\/span><\/div>\n
\n<\/div>\n
\n<\/div>\n
\nIn the end, I hope my mistakes and lessons learned will be useful.<\/span><\/span><\/div>\n
\n<\/span><\/div>\n
\n<\/div>\n<\/div>\n
From ahm507.blogspot.com<\/div>\n","protected":false},"excerpt":{"rendered":"

Giving advice is not very effective most of the time. Sharing real experiences and trouble and how it is handled is more effective. The best is whoever learns from others mistakes. So I am sharing here a story where I and my team introduced a bug and it was leaked to the production environment. I […]<\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[1],"tags":[],"_links":{"self":[{"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/posts\/13"}],"collection":[{"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/comments?post=13"}],"version-history":[{"count":1,"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/posts\/13\/revisions"}],"predecessor-version":[{"id":251,"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/posts\/13\/revisions\/251"}],"wp:attachment":[{"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/media?parent=13"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/categories?post=13"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/ahm.basfinans.com\/index.php\/wp-json\/wp\/v2\/tags?post=13"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}