DEV Community: Žiga Miklič

Why, Oh Why Was This Added?

Žiga Miklič — Thu, 26 May 2022 16:32:02 +0000

To read this post you will need to use a bit of your imagination.

Suppose you are checking a component called DatePicker that is built on top of a 3rd party date picker component. You are tasked to find if the component is causing problems now that you have updated another project dependency.

It is a larger update, so there are some breaking changes. From the migration guide, you even know exactly what can be the cause of the issues: event.stopPropagation(). That is exactly why you have DatePicker opened - it's using event.stopPropagation() in one of its event handlers:

function DatePicker() {
  function handleDateChange(event) {
    event.stopPropagation();
  }

  return (
    <ThirdPartyDatePicker
      onChange={handleDateChange}
    />
  );
}

A simplified version of the component

But, why was that event.stopPropagation() added? To know if the migration broke something, you first have to find the reason why it exists in the first place. Taking a sip of your coffee, you embark on your debugging journey.

Debugging Journey

When you remove event.stopPropagation(), you don't see any difference. All the tests are passing. You check the 3rd party date picker project/documentation if there is a specific reason but return empty-handed. You then revert all the migration changes and remove event.stopPropagation() in your current app version. You also don't see a noticeable difference. You start asking yourself if it serves a purpose at all.

You then check the Git history and see event.stopPropagation() is there since the initial version and there is no info about it in the commit message. Another dead end. Finally, you check who implemented it. Maybe they know some secret you don't. Maybe it's something super obvious that you are missing. Finally, they will solve this mystery and you will be able to go to bed peacefully tonight! Well, it turns out the original author no longer works at the company.

But you are still hesitant to remove it. Surely it was not added for no reason? You don't want to make assumptions and repeat your past mistakes. So you search your app where the component is used and try to spot any special use cases. And after a while you do. In one use case (out of 100+) the DatePicker component is placed inside another component in which preventing event bubbling is required. To make sure you don't forget, you quickly add a comment explaining why event.stopPropagation() is there. You are hopeful that it will help save time for someone in the future. That future someone might even be you.

After that, you are finally able to test if the migration is causing an issue and fix it if necessary. And so you come at the end of your debugging journey. Well, sort of. You come to the end for this particular event.stopPropagation(). There are still quite a few more in the codebase.

You move on to the next event.stopPropagation() and see that this one also does not include a comment. You read the surrounding code but again don't know why someone added it.

And just like that, we are again at step 1 of our debugging journey.

Backstory

Now to give you more context.

This week I was migrating our app to React 17. I suspect you might be thinking: "React 17? You do know that React 18 is already out, right?" I am solely to blame here. I had concerns with React 17 breaking another dependency and postponed the update several times.

Nevertheless, one of the things that React 17 changes is how event delegation works. More specifically, React no longer attaches event handlers to the document object but instead attaches them to the root DOM container.

My plan for migrating to React 17 was to search our codebase for event.stopPropagation() and then test the UI to make sure everything was working correctly. But while I was going through each use case, I realized just how little information event.stopPropagation() provides. We usually call it at the top of event handlers without any extra info. When checking the code you know what it is doing (preventing event bubbling) but you don't know why.

Some use cases were obvious by looking at the surrounding code but for a lot of them, it took me a while to figure it out. These ambiguous use cases fell into four categories:

They prevent triggering events in components nested many levels higher.
They solve very specific edge cases.
They prevent triggering events in 3rd party libraries.
They exist for no reason.

Please note that I am not writing this post to point fingers at my coworkers. I have written this post primarily for myself as I have also written many times event.stopPropagation() without including a comment. I write it automatically when the need arises without giving it a second thought.

Conclusion

While not commenting your event.stopPropagation() calls is usually not a big deal, it may be a problem in situations like mine where I had to go over many ambiguous use cases at once. This resulted in a lot of wasted time that could be otherwise avoided if the original author added a comment.

So the next time you are writing event.stopPropagation(), stop and think if adding a comment where you explain why it is there would benefit a future reader. Your colleagues or your future self will be grateful that you did.

Lessons Learned: The Passionate Programmer

Žiga Miklič — Thu, 19 May 2022 13:00:25 +0000

Below are 5 of my favorite lessons from the book The Passionate Programmer by Chad Fowler.

Learn How to Fail

Everyone makes mistakes so it's important to learn how to react when we do make a mistake:

We shouldn't hide our mistakes and instead, we should let our team know as soon as possible. Mistakes that we catch early are easier to fix and will likely have a lesser negative impact.
We need to find a solution as quickly as possible. That means we should take responsibility for the issue (even if we are not 100% at fault) and start searching for a solution instead of searching for who to blame.
We should propose a specific plan (step-by-step, goals) on how to fix the issue. This is especially important if the issue affects others on our team.
We shouldn't be afraid to ask for help, especially if we are unsure with our proposed solution.
We shouldn't panic. Panicking will not give us an advantage but will instead cause us to underperform in a situation when we need to perform our best.

Step off of the Hedonic Treadmill

We all want to progress in our careers. Be it a bigger salary, a shinier title, or more responsibilities. While we should be ambitious and have goals, being too ambitious can sometimes produce negative results.

When we constantly want more, we become more focused on our next job rather than our current one. And while we fantasize about our future job, we may start to produce mediocre work at our current one.

When we compare our current job with a fantasized future job, our current job will always appear worse. It makes us ignore the good parts and amplify the bad parts of our current job.

While it seems counterintuitive, we may produce better results by focusing on the present. With our focus on our current job, we enjoy it more. We start noticing the small (daily) wins that we would otherwise miss and feel more of an essential part of the team. As we enjoy our job more, we become more motivated and productive, which makes it more likely for us to succeed.

Make Borings Tasks Fun

We do our best work when the task is fun, exciting, or interesting. But when a task is boring, we drag our feet and as a result, produce mediocre results. Almost every job has tasks that few enjoy doing but still need to get done. These are usually tasks where we don't get to flex our creative muscles or test our skills. But what if we could make those boring tasks more fun?

One way to do so is to make the boring tasks more challenging. How would we do a task if we wanted to do it "perfectly" or what if we imposed a very tight deadline?

Another way to make boring tasks more fun is to turn them into a competition with our coworkers to see who can do them better.

Improve Your Writing

We write a lot during our workdays so it's a good idea to become good at it. This is especially important for distributed teams where we won't be effective team members if we are unable to express ourselves.

Improving our writing will also make us better programmers. The ability to explain an idea and lead the reader to a logical conclusion is very similar to the ability to design a maintainable system that future developers will be able to understand.

Improving our writing also saves time (and patience) as our team members don't need to guess what we meant or have to reimplement a feature because of a misunderstood of our writing.

Take One Small Step at a Time

When we are facing large, complex projects we quickly become demotivated. We become farsighted and only see the amount of work and time that it will take to fully complete the project instead of seeing all the small (easy) steps that will get us closer to finishing it.

On larger projects, we don't make a lot of progress in the short term. This can make it seem like our efforts are for nothing and can lead us to abandon the project completely.

The key to completing large projects is to focus on making our project better today than it was yesterday.

If we work on an application with poor codebase health, the task of improving it will look daunting. Instead of focusing on the end state where the codebase is in "perfect" health, we should instead focus on making it just a tiny bit better. We should look for easy wins in the parts of the codebase we are currently working on. See a poorly named function? Rename it. See a large section of commented-out code? Remove it. See a long class method? Refactor it into smaller, well-named methods.

Do you find these lessons interesting? Well, I've got good news for you. The book contains many more such lessons (53 to be exact). It also features a couple of amazing essays by successful people from our industry, my favorite one being from the founder of GitHub. So if you are looking for a book to improve your career, this is a great choice.

The post Lessons Learned: The Passionate Programmer appeared first on Blog by Žiga Miklič.

Refactoring With Confidence

Žiga Miklič — Mon, 16 May 2022 13:20:48 +0000

Last week while I was making changes to our constants.js file, I spotted that we could improve its maintainability and readability. But before I could put on my refactoring glasses, I first had to make sure I would not introduce any new bugs into the codebase.

Because the list of constants that I would need to change was quite long, there was a high chance I would make a mistake. Our automated tests would most likely not catch the bug as our constants.js file is not explicitly tested as it does not contain any logic. So to improve my confidence in making the changes, I first created a (throwaway) unit test. Although I generally avoid using Jest snapshots, I think this was an ideal use case for them.

Inspecting the Problems

Let's start by taking a look at our initial constants object:

const constants = {
  CHANNEL_BLOG: 1,
  CHANNEL_WORDPRESS: 2,
  CHANNEL_SHOPIFY: 3,
  CHANNEL_FACEBOOK: 5,
  CHANNEL_TWITTER: 6,

  CHANNEL_GROUP_PERSONAL: [1, 2, 3, 4],
  CHANNEL_GROUP_SOCIAL: [5, 6],
  CHANNEL_NAMES: {
    1: 'Blog',
    2: 'WordPress',
    3: 'Shopify',
    5: 'Facebook',
    6: 'Twitter',
  },
  CHANNEL_ROUTES: {
    1: 'channels/blog',
    2: 'channels/wordpress',
    3: 'channels/shopify',
    5: 'channels/facebook',
    6: 'channels/twitter',
  },
};

The above object is a smaller version of the real one so it more easily illustrates all the issues that I wanted to fix:

The value of each channel is located in three or more locations. This means if we would need to change the value of a channel, we would need to make sure to do so in all locations. Spoiler alert, you will most likely miss changing one of them.
The values can become out of sync as the values are not linked with each other. You may have spotted such an issue in the above example. At some point, someone removed the CHANNEL_ARTICLE with the value of 4. They removed its definition, name, and route; but forgot to exclude it from all the channel groups (see CHANNEL_GROUP_PERSONAL).
The channel groups are hard to read as you need to manually/mentally map a value to the corresponding channel name. This means if you want to remove the Facebook channel from the CHANNEL_GROUP_SOCIAL, you first need to find out which value (5 or 6) is referring to Facebook.

Making the Improvements

An object with all the above issues fixed would look something like this:

const CHANNEL_BLOG = 1;
const CHANNEL_WORDPRESS = 2;
const CHANNEL_SHOPIFY = 3;
const CHANNEL_FACEBOOK = 5;
const CHANNEL_TWITTER = 6;

const constants = {
  CHANNEL_BLOG,
  CHANNEL_WORDPRESS,
  CHANNEL_SHOPIFY,
  CHANNEL_FACEBOOK,
  CHANNEL_TWITTER,

  CHANNEL_GROUP_PERSONAL: [CHANNEL_BLOG, CHANNEL_WORDPRESS, CHANNEL_SHOPIFY],
  CHANNEL_GROUP_SOCIAL: [CHANNEL_FACEBOOK, CHANNEL_TWITTER],

  CHANNEL_NAMES: {
    [CHANNEL_BLOG]: 'Blog',
    [CHANNEL_WORDPRESS]: 'WordPress',
    [CHANNEL_SHOPIFY]: 'Shopify',
    [CHANNEL_FACEBOOK]: 'Facebook',
    [CHANNEL_TWITTER]: 'Twitter',
  },
  CHANNEL_ROUTES: {
    [CHANNEL_BLOG]: 'channels/blog',
    [CHANNEL_WORDPRESS]: 'channels/wordpress',
    [CHANNEL_SHOPIFY]: 'channels/shopify',
    [CHANNEL_FACEBOOK]: 'channels/facebook',
    [CHANNEL_TWITTER]: 'channels/twitter',
  },
};

Let's look at the new object more closely:

We refactored the channel definitions into variables. This allows us to reuse them as values for other constants.
We replaced the fixed values inside the channel groups. This makes it easy for someone to read what channels are in each group. The mistake mentioned above where someone forgot to remove all values of a constant also won't be possible to repeat in the future. This is because as soon as you remove a constant and don't remove all its usages, you will start seeing the ReferenceError: "x" is not defined error.
We replaced the fixed property keys of CHANNEL_NAMES and CHANNEL_ROUTES with the channel variables. We did so by using computed property names.

As you can see, there are multiple changes that we need to do for each constant. In the case of our app where we have 30+ channels, there is a good chance that I would make a mistake while refactoring. I hope at this point you agree that the upfront investment of writing a unit test is well worth it.

Creating the Unit Test

For this task, the unit test needs to check the original object and compare it to the modified version. In our app we use Jest to write our unit and integration tests.

We could include the original object inside the test and use toMatchObject matcher but I instead opted to create a snapshot:

import constants from '../constants';

it('should match the original object', () => {
  expect(constants).toMatchSnapshot();
});

When we run the test for the first time, Jest will remember the object value by storing it as a snapshot. It will then use the snapshot for comparison when we run the test again.

To run the test I used the following Jest CLI command:

jest contants.test.js --watch

The --watch mode makes sure to automatically run the test each time we make a change.

Once I had the unit test in place, I went step-by-step making small changes. If I made a mistake after a change, the unit test would let me know and point to the issue. Fixing a mistake was fast and simple. After I had made all the changes, I removed the unit test and opened a pull request.

Conclusion

We need to be confident that we will not introduce any issues in our codebase when making refactorings.

If the change is small and isolated (changing a variable name, fixing a typo, etc.), checking the change manually can be a valid approach.

But in a lot of cases making sure a unit test is watching over our changes is the practical way to go. This is especially true in situations when making:

large changes,
changes that affect multiple parts,
changes that require you to test many cases.

When we are refactoring a part of the codebase that is not yet covered by an automated test, we should check to see if introducing a test to the codebase will provide value. If so, then we need to make sure to write a test that is not going to be a maintenance burden. Jest snapshots were a great use case for this throwaway test but in a lot of cases can be a maintenance nightmare as your tests can report false negatives.

This post first appeared on my personal blog.

A Short Cautionary Tale About Refactoring

Žiga Miklič — Thu, 18 Mar 2021 23:18:52 +0000

This week I was making changes in a part of the codebase I was unfamiliar with. While reading the code I spotted an inlined PNG image. This was not a small icon but an image of about 100kb in size. I searched the project and found the same inlined image in a couple of different components. Maybe it was because I was in a need of some coffee, but at that moment I could not think of a good reason for this approach. As the components with the inline image were quite similar, I surmised that they were copied from each other and the inline image is just one of those weird things that you find in legacy code.

I decoded the embedded image from Base64 and created a separate PNG file from it. The final step was to replace the inlined image source with a relative path to the new image file. The diff for the commit was almost perfect: large red blocks of deleted code and just a few green lines of added code. A quick and easy refactoring win and a textbook example of the boy scout rule. Or so I thought.

I took a short break and grabbed some much-needed coffee. Fast forward a few hours and I completed the task that I was originally working on. I was in the final stages of testing when I noticed a bug. At that same moment, I also realized the exact purpose of the inlined image. Can you guess?

To give you more context, we use the components with the image to display error messages. The image is a cute illustration of the company mascot with the expression “Oops something went wrong”. One of the possible errors that can happen, is when the user losses their connection or goes offline. So when that happens and you render a component that still needs to download an image, the image will fail to load. Oh yeah… 🤦‍♂️

I thought I had learned the hard way earlier in my career not to presume I know better than the original author. Of course, there are situations where a less experienced coder (ie. yourself from a couple of years ago) worked on a feature or where someone had to cut corners (eg. due to a deadline). But usually, there is a good reason why someone took the approach that they did.

Luckily I spotted the issue before I pushed my branch. I could use git reset --soft and alter my changes. I extracted the inlined image into a separate React component and used it where needed. This way we still get both benefits: the error components are much cleaner and easier to read, and the inlined image is located in one place. After that, I reapplied all the other commits. The Git history was clean and none of my coworkers were the wiser. Unless they are reading this, in which case: “Oh hey, how’s it going?” 😅

I know, this bug would not be the end of the world. But this small example shows how even the best of our refactoring intentions can do more harm than good. If you find yourself looking at a piece of code that looks strange or needlessly too complex, ask your coworkers. This is especially true if the original author is still at the company. You might be missing some context and you’ll get to learn something new. Or you can indeed improve the code, in which case the original author may learn the better approach. In both cases, it’s a win. Also if there is a good reason for the current approach, document it. Add a more descriptive name or a comment to avoid confusion in the future.

Learn from my small mistake and research before making refactorings. This is 10 times more important for larger changes and when editing the core files of the project. And the most important thing: don’t forget to drink some coffee before starting.

This post first appeared on my personal blog.