Showing posts with label Refactoring. Show all posts
Showing posts with label Refactoring. Show all posts

Wednesday, May 6, 2015

Fun with DateTime: DateTimeKind.Unspecified

Last time, we changed out the sunset provider of our home automation software. Since I was trying to concentrate on something else at the time, I copy/pasta'd a code sample from documentation into the application. The code worked, but it felt like there was an easier way to do things.

I took a closer look and did find an easier way. But I also ran into the curiosity of how things behave when we have an unspecified kind on a date. Let's take a look.

Simplifying Code
The code we start with isn't that complex.

Original Class

This uses the Solar Calculator package from NuGet. The second line (in each method) creates the SolarTimes object that we need by passing in the date and latitude/longitude. The other 2 lines of code converts that date/time to a specific timezone.

It turns out that we don't need a specific timezone for our application. Instead we can use the local time. This makes the code much simpler. Here's the updated code:

Updated Class

Instead of converting the "Sunset" and "Sunrise" properties to a particular timezone, we just take the values as-is. And this gives us the output that we expect:


This code works because the "Sunset" property represents local time (sort of). The DateTime structure has a "Kind" property. We'll need to explore this a little further.

Note: I also changed the name of the class. The package I used was also called "SolarCalculator", so there could have been some nasty side-effects. It makes sense to append "SunsetProvider" since that is how this is used in the rest of the application.

DateTimeKind
The "Kind" property of a DateTime is an enum (DateTimeKind) that has these values (from the MSDN documentation):


This means that we can mark a DateTime as being "Local" or "Utc". That should make things really easy.

So, let's see what "Sunset" is:


Oops, it looks like the "Kind" is "Unspecified". But if we look at the value itself, we can see that it represents the local time for sunset.

Changing the DateTimeKind
Even though this has the right value, I feel like things would be better if we could change the "Unspecified" to "Local". And this is where we see some of the weirdness when we have an "Unspecified" kind.

DateTime has a "ToLocalTime" method (from MSDN documentation):


This looks pretty promising. After running this method, the "Kind" will be "Local". But this method assumes that the "Unspecified" time is UTC. So, if we use it, we end up with an unexpected value:


That's not what we want; sunset is not just after noon. And this value is completely wrong (since it now represents a local time).

Let's try again. DateTime also has a "ToUniversalTime" method (from MSDN documentation):


This method makes the opposite assumption. This method assumes that the "Unspecified" time is Local. So, if we use it, we end up with an unexpected output:


This is actually a bit closer. The value is correct. Yes, I know that sunset is not at 2:30 in the morning, but the Kind for this is "Utc". So the value is correct, even though the display is not what we want.

A Smelly Solution
The value is correct (2:38 a.m. UTC is 7:38 p.m. Local), so let's see what happens when we combine these methods:


Because the Kind is no longer "Unspecified" when we call "ToLocalTime", we get the output that we expect. Let's look at the progression:
  • Sunset = 7:38:58 p.m. (Unspecified)
  • ToUniversalTime() = 2:38:58 a.m. (UTC)
  • ToLocalTime() = 7:38:58 p.m. (Local)
And our output reflects this:


But this just smells bad to me. Luckily, there is another option.

Setting DateTimeKind Directly
Our other option is to set the DateTimeKind directly. Well, sort of directly. The "Kind" property is read-only, but we do have a method that we can use:


This allows us to do the following:


This returns a new DateTime object that has the Kind set to "Local". And it has the value that we want as well:


If we really want to avoid having an "Unspecified" date/time, we can set it this way. And this method is much less smelly than converting a time to UTC and then converting it back to Local.

Wrap Up
So we've seen that when we deal with DateTimeKind, things get interesting if the value is "Unspecified". The "ToLocalTime" method assumes that the value represented is UTC, so it does a conversion. The "ToUniversalTime" method assumes that the value represented is Local, so it does a conversion. If we're not careful, we can end up with an incorrect value.

Working with DateTime is not as easy as it looks -- and we're not even dealing with any crazy timezone rules here. For a deeper dive, be sure to check out Matt Johnson's material, including the Pluralsight course Date and Time Fundamentals. (And I'm sure Matt has some specifics to add to this discussion.)

I like to simplify code wherever possible. Since this application doesn't need to deal with specific time zones, we can remove that code and deal with the local time. And to figure this out, I got to explore the DateTime object a bit more and learn how to use the "Kind" property. Learning something new is always fun.

This approach isn't the right solution every time, but it works just fine here. Sometimes we do care about specific time zones, but that's not needed in our case. The updated code is easier to approach, and I prefer to keep things simple where practical.

Happy Coding!

Sunday, April 5, 2015

My Approach to Testing: Test Public Members

In my presentation "Clean Code: Homicidal Maniacs Read Code, Too!", I spend quite a bit of time refactoring code (not as much time as I'd like, which is why I put out a supplemental video: Clean Code: The Refactoring Bits).

Unit tests are a vital part of the code that I show. The unit tests are what make sure that I don't inadvertently change functionality as I refactor the code. Much of the refactoring involves extracting out pieces of code and putting them into their own methods. This makes the code easier to navigate.

I do get questions about my testing technique as I show this code. Here's a question that I got at Nebraska.Code() right after the presentation:

Question Time!
How do you modify your tests after extracting code into the new private methods?
The short answer is: I don't.

The longer answer is my approach to unit testing: I test the public members of my code.

Let's take a look at some examples so that I can show this in action, and then I'll talk about why I take this particular approach.

Refactoring Code
Here's the "Initialize" method that we start with:


During the process of making this more readable, I extract out a couple of methods and move some assignments around. What we end up with are a couple of private methods in addition to our public one:


This takes some of the details and "hides" them in private methods. This way, when we first walk up to the "Initialize" method, we can easily decide which parts of the code are important for what we're doing. If we don't care about the dependency injection container bits, then we can skip right over those and go to the "RefreshCatalog" method.

Here's another example; this time we refactor the "RefreshCatalog" method. Here's the original:


And the version with bits extracted:


This makes "RefreshCatalog" much easier to follow. We get a high-level overview of what the method is doing. If we need to look at details, then we can drill into those methods. This is especially appreciated when you walk up to this method for the very first time. (And we have to remember that sometimes we keep walking up to the same method "for the very first time" over and over again -- we get put on other projects and have to come back to this application 6 months later to make some enhancements, and we have to get our bearings again.)

Testing Public Members Only
So why don't my unit tests change as part of this process? Because I'm only testing the public members of my class -- whether public methods, properties, or events.

So if we turn on CodeLens and take a look, we'll see that only the public methods have unit tests:


In the case of "Initialize", we see that there are 19 tests (which is all of them). This is because each of our tests runs the "Initialize" method as part of the setup even if it's not explicitly testing this code.

We see something similar for "RefreshCatalog":


In this case we have 4 tests that call "RefreshCatalog" directly. In 2 of these tests, we check to see if the service is called based on the state of the cache:


If we look at the details of the first test, we see that it does call "RefreshCatalog":


And it also indirectly calls the "IsCacheValid" property and the "RefreshCatalogFromService" method.

I won't go into the other details of this test; it gets a little weird because we're testing an asynchronous service that uses APM (Asynchronous Programming Model) wrapped in a Task which (sort of) changes it to TAP (Task Asynchronous Pattern). So there's a little helper object ("tracker") to make testing easier. This would be a good topic for another day.

[Update: You can see how the tracker works in this article: Tracking Property Changes in Unit Tests.]

Why Not Test Private Members?
So the question is why don't I create tests for the private members? My reasoning is that I like to keep the code as clean as possible.

When I create production objects, I want to modify the code as little as possible for testing. When we want to test private members, we basically have 3 options:

Option 1: Reflection
Our first option to test private members is to use reflection to crack open our class so that we can access the bits of code we're not normally allowed to look at.

I don't really like this option because our tests become *very* complicated very quickly. Reflection is one of those things that is difficult to understand on its own. When we make it a requirement for our unit tests, then we're just asking for trouble.

The end result when we take this approach is that we just don't bother with tests because they are too difficult to create.

Option 2: Change Access Modifiers to Public
Another option is to change the "private" members that we want to test to "public."

I'll just cross this option off immediately. I don't want to mess with making things visible to the outside world when it's not appropriate. Scoping and visibility are huge parts of building good objects, libraries, and APIs. We can't compromise that for testing.

Option 3: Change Access Modifiers to Protected
Another option is to change the "private" members that we want to test to "protected." When we do this, we can then create a wrapper class in our tests. This wrapper class descends from our production class, so it has access to the protected members. It can then supply wrapper methods or properties to access the protected members of the base class.

I don't like this option, either. The main reason for that is that I feel like I'm not testing my production code. Instead of testing my actual class, I'm testing some mutation of that class. I'm not confident that the behavior between the test class and the production class will be identical, so I lose faith in my tests.

What if I Really Need to Test a Private Member?
In the examples that I've shown here, it's pretty easy to say, "I'm okay testing at a higher level." By testing the "RefreshCatalog" method, we end up testing all of the private members indirectly.
But what if one of those private members is so important to my functionality that I really want to test it directly?
For example, if I'm accepting credit cards, I want to do the Luhn check against them. This will at least make sure that the number itself is potentially valid before checking against a credit card processor.

My answer to this is pretty simple:
If a private method is important enough that I need to test it directly, it's probably important enough to have its own class.
This really takes me to the Single Responsibility Principle and Separation of Concerns. Needing to test a private method directly is a code smell. If I run across that, then it probably means that it's a separate concern that needs its own place in the code.

When I extract that out into its own class (whether an instance class or a set of related methods in a static class), now I can test that class directly.

Of course, we would have to get into a discussion on the proper visibility of the new class and methods. But that's easier to take care of. If I have a protected or internal class that's only available to the code in the same assembly or namespace, I can create a test class that is only responsible for directly calling into this protected class. But I'm not wrapping the class itself, just creating a test class that is capable of calling into the real class.

Many Approaches to Testing
There are many approaches to unit testing. And I will be the last one to say that this is an example that everyone should follow. This particular approach of testing the public members has worked out well for me in the majority of situations that I've run into. It has the added benefit of being fairly resistant to refactoring.

When we create the public members of our classes, these generally remain unchanged. This is because the public interface is how the outside world interacts with our objects. So we try to change these as little as possible.

The private members, however, are subject to change. We can rename private methods, move things to other methods, properties, or classes, and combine similar code into consolidated methods. If we are directly testing the private members, we're less likely to make these types of changes because it would mean making major changes to our tests as well (especially if we're using reflection which would not give us compile-time errors, only run-time errors).

Wrap Up
I encourage people to try different approaches to testing. Each has its advantages and disadvantages. I've managed to dial in an approach that works well for me and the types of applications that I normally build. But that doesn't mean that I don't keep exploring.

I'm still working on TDD. I've had some really good successes with it lately, and I've got another piece of code that needs some help, so I'll be doing some more experimentation this week.

Overall, automated testing gives me confidence in my code. The tests are proof that my code does what I think it does. And I can get immediate feedback by re-running tests whenever I change code (without having to run my application).

So experiment, try different techniques, and come up with a testing approach that works well for you. Ultimately, you will end up with better code.

Happy Coding!

Thursday, October 17, 2013

How Do I Change Someone Else's Code?

I had an interesting question come up at my Clean Code session at Silicon Valley Code Camp:
How do I change someone else's code?
This question came up as we were talking about refactoring code to improve the readability. And the question was not coming from the technical standpoint, it was coming from the interpersonal standpoint. So, the question could really be stated several different ways:
How do I deal with a developer who is possessive about the changes made to "his" (or "her") code?
How do I tell a developer that his/her code could be more readable?
Let's start with the first question.

Code Ownership
If people are possessive of "their" code in the codebase, this is a sign of a larger issue.

Note: I'm approaching this from a standpoint of developers on a team working for a company just so we have an idea of the environment. But this applies to other types of teams as well.

Our focus really should be on the business -- how the applications we build move the business forward. And the most productive environments I've worked in have this mindset.

In the agile world (and I'm reluctant to use that word since a lot of people who are "Agile" really aren't), everyone on the team is responsible for the code. If someone sees a problem in an application, he is empowered to fix it (or alert someone about it if he is not capable of fixing it himself) -- and arguably, he would be irresponsible to not do anything about it.

So, if you find yourself in a situation where you can't touch a certain module because "that's Jim's code", it might be time to bring in someone to help with the dynamics of the team.

We don't always have control over the team, but we do have control over ourselves. If you find that you're offended that someone changed "your" code, then it's time to take a step back and re-evaluate your own attitude.

Controlling the Ego
Developers are known for having large egos. This really isn't surprising because at its heart, development is more of a creative process than a technical process. The last thing that an artist wants to hear is that you think his painting is ugly. Developers generally react the same way.

But we need to take a step back and look at what we're really doing: we are building tools that propel a business forward. And we don't even own the code we write (from a legal standpoint), the company owns it. This means that the company can do whatever it wants with the code, including rewriting it, breaking it, using it incorrectly, or even never using it at all. So, we cannot get personally attached to any particular function that we create.

I won't say that this is an easy thing to do as a developer. I put a lot of thought and effort into the software that I write. When I see that it is not being used or being used improperly, it really annoys me. But then I take a step back. In my experience, there is very little benefit that comes out getting worked up over these things. I voice my opinion and then let it go (sometimes more successfully than others).

Learn from Other Developers
The same holds true if another developer makes changes to code that you originally wrote. Instead of having a kneejerk reaction, understand that the other developer is not calling your painting ugly. He (hopefully) has the same goal that we all do, to propel the business forward.

So, stop and look at the changes that were made. If you think they are good, learn from them. If you think they are bad, talk to the other developer. Communication goes a very long way in the development world. We don't always approach problems the same way (actually, we probably never approach problems the same way). It's good to understand what someone else is thinking. I've used this several times as a learning experience. And sometimes by working together, we come up with something that neither one would have come up with separately.

It's All in the Approach
So, now let's deal with the second question: How do I tell a developer that his/her code could be more readable?

A great way to approach this is to ask the developer to explain the code. "Hey, Jim, I'm having a little trouble understanding this block of code. Could you help me out?"

If he asks why you're looking at "his" code, tell him that you're just trying to get a better understanding of the system overall.

This may or may not work. It all depends on the other developer. We don't have any control over how other developers react. Just remember to keep your own reactions in check.

If he does explain the code, then you can offer some suggestions to make it more clear to someone just walking up to the code for the first time. Hopefully, you'll be able to work together on making the code better.

Sometimes We're Stuck
If we are part of a dysfunctional development team, sometimes there's nothing we can directly do about it.

But there is a big difference between being *on* a dysfunctional team and being *part of* a dysfunctional team. It is very easy to be overwhelmed by the things going on around you and let them affect you. I've had that experience. I let the problems of a team I was working with get the better of me, and it affected my attitude. I was lucky enough to have someone point this out to me, so I was able to readjust how I was reacting to things.

Attitude is Infectious
Just like a negative attitude is infectious, so is a positive attitude. We can't control others, but we can control ourselves. I've found that if I take a certain attitude toward development (focusing on the business, checking my ego, constant learning), it rubs off on other members of the team.

We *can* change the attitudes on our team. It starts with us re-evaluating our own attitudes. From there, we set a good example, pump out some excellent code, and focus on the goal. Others will want to follow. (And if that doesn't work, polish up the resume. If this is your approach to development, I want to work with you.)

Happy Coding!

Sunday, July 14, 2013

Refactoring Question

Earlier today, I got a question through Twitter in response to my recently published video: Clean Code: The Refactoring Bits.  Since I can't answer adequately in 140 characters, I thought it would be better to answer here.

Question:
What is the best way to refactor C# code containing goto statements?

Answer:
It depends.

This is my standard answer to pretty much everything.  It's nearly impossible to answer a question like this without further context.  I tend to stay away from the words "always" and "never" just because they tend to over-simplify things.  And the real world is never simple.

As an example, I have heard of developers who say that say we should never use a "switch" statement; instead, we should use the Strategy Method pattern.  A dramatic over-simplification.

I'll say the same thing here.  The "goto" statement has fallen out of favor over the years.  The primary reason is that it tends to make code more difficult to follow.  And in the circles I've been in, the developers don't even know the C# syntax off the top of their heads.  But just because you run across a "goto" doesn't mean it must be changed.  Again, it depends.

Refactoring Resources
I'm definitely not a refactoring expert, but I will point to some resources.

The first is Working Effectively with Legacy Code by Michael C. Feathers (Jeremy's review).  The thing that I really like about this book is the emphasis on unit testing.  Before doing any refactoring, we need to bring the current code under effective automated testing.  This way, when we start our refactoring, we get nearly immediate feedback if we inadvertently break something.

This is something that I show several times in the refactoring video.  If our refactoring goes awry, our unit tests let us know about it right away so that we can take action.

There are two other books that are generally recommended: Refactoring: Improving the Design of Existing Code by Martin Fowler and Refactoring to Patterns by Joshua Kerievsky.  Both of these books are currently in my library, but they are still in the "To Be Read" pile.  I've flipped through both of them and see lots of useful advice, but I haven't actually sat down to read them yet.

These resources will help get us headed in the right direction.  But, ultimately, like every other situation that we deal with in programming, what we actually should do depends on the particular situation.

Happy Coding!

Screencast for Clean Code: The Refactoring Bits

New video available: Clean Code: The Refactoring Bits

New Screencast
One of my latest presentation topics is Clean Code: Homicidal Maniacs Read Code, Too!  One problem is that I usually run out of time during the refactoring portion of the presentation (there's always too much good stuff to talk about).  We always get through the important bits: intentional naming and hierarchical methods, but it's nice to be able to see multiple examples to reinforce the concepts.

To remedy this, I put together a screencast of just the refactoring portion: Clean Code: The Refactoring Bits.  As with all of my presentations, there is a PDF walkthrough of the session available on my website at the link above.

Live Presentations
The Clean Code session has received a good response from folks who've attended.  If you want to see it live, be sure to stop by one of these upcoming events:

Saturday/Sunday, July 27-28, 2013
So Cal Code Camp
San Diego, CA
http://www.socalcodecamp.com/

Thursday, August 22, 2013
Southeast Valley .NET User Group
Chandler, AZ
http://www.sevdnug.org/Home.aspx

Saturday/Sunday, October 5-6, 2013
Silicon Valley Code Camp
Los Altos Hills, CA
http://www.siliconvalley-codecamp.com/

These are the events I currently have on my schedule; I'm sure there will be others as well.  If you'd like me to come to your user group, Code Camp, or other event, feel free to send in a request through INETA or contact me through my website.

I hope to see you at an upcoming event.

Happy Coding!

Thursday, February 14, 2013

Book Review: Working Effectively with Legacy Code

I just finished reading Working Effectively with Legacy Code by Michael C. Feathers (Amazon link).  I'll dispense with the standard review since this book has been around since 2005 and has many reviews and recommendations.

The short recommendation: Read this book.  Even if you aren't working with existing code that needs to be updated, the techniques can be used to recognize potential issues on new code that you're writing.  And if you are trying to untangle an existing code base, this book is invaluable.

Refactoring for Testability
The best way to describe this book is "refactoring for testability".  The idea behind this is that if you need to make modifications to code, you need to first have tests in place.  If you do not have tests for the existing code, then if your changes break the current functionality, you won't know about it.  With tests in place, you will know if you break something as soon as you make those changes.

This is a "Do no harm" approach.  The code that you are walking up to is working code (you're adding features, not necessarily fixing bugs).  The last thing you want to do is to remove the "working" part.

This is especially critical if you are working with code that you didn't write yourself (or even code you did write yourself several years ago).  You need to recognize that if you are going to be successful in working with unfamiliar code, you need to make small changes one step at a time.

Three Parts
The book is broken down into 3 parts.

Part I: The Mechanics of Change
Part one focuses on why we need to make changes to code, how unit testing can help us make confident changes, and several tools and techniques (such as finding/creating "seams", refactoring tools, and mocking). This is a high-level overview of why we need to start working this way (refactoring for testability).

Part II: Changing Software
Part two focuses on specific problems that you may encounter in the code -- things that make the code hard to get into a test harness.  Personally, I just like reading the chapter titles.  Here are a few:
  • I Don't Have Much Time and I Have to Change It
  • I Need to Make a Change, but I Don't Know What Tests to Write
  • Dependencies on Libraries Are Killing Me
  • I Don't Understand the Code Well Enough to Change It
  • My Application Has No Structure
  • My Test Code Is in the Way
  • My Project Is Not Object Oriented. How Do I Make Safe Changes?
  • This Class Is Too Big and I Don't Want It to Get Any Bigger
  • I Need to Change a Monster Method and I Can't Write Tests for It
  • How Do I Know That I'm Not Breaking Anything?
Part III: Dependency-Breaking Techniques
Part three consists of a catalog of 24 techniques that you can use to bring code under test.  These are all presented in a short (usually 3-5 page) format with an example and specific steps to follow.  These techniques are referenced in Part II, so you'll probably be familiar with the concepts.

Some examples:
  • Break Out Method Object
  • Encapsulate Global References
  • Expose Static Method
  • Extract Implementer
  • Extract Interface
  • Parameterize Constructor
  • Pull Up Feature
  • Push Down Dependency
  • Subclass and Override Method
This seems kind of "design-patterny" in the sense of having a catalog of well-named techniques.  These techniques aren't perfect.  Michael Feathers notes in several places that some of these techniques actually make your code "worse" structurally.  But they are still valuable because they increase the testability.  The theory is that the more you work with the code, these bad structural patterns will be refactored into more organized code.  But the point is that these changes are all made incrementally -- one, small change at a time.

An Example
One chapter that especially jumped out at me (not sure why -- probably because I've seen this before) is "I'm Changing the Same Code All Over the Place."  This has to do with reducing the duplication of code between 2 classes.

What I really liked about this chapter is that it shows the reduction in duplication is small steps.  There is no, "just extract everything to a superclass and you'll be fine."  Instead, it starts by figuring out what the commonalities are between the classes -- starting with properties and methods.

The first step was to create a superclass with *one* common method.  Then the implementation methods (in each class) are updated to make them more similar (by extracting out a method to make the body of the common method identical).  Then this implementation can be moved up to the superclass with only the differences (the extracted method) in each subclass.

The next step was to move the common properties up to the superclass.  Then the refactoring continues one step at a time.  When we are left with some unique properties in each class, the question is can we combine these into a common collection.

I don't expect that this description will make much sense out of context.  The point is that instead of thinking about the refactoring in big chunks, we are thinking about it in very small steps.  At each step, we can make sure that the tests still work as expected.  And if a test fails, we can immediately know what happened.  If we were to do a "big blast" change and the tests fail, we won't know specifically what the problem is.

As someone who has rolled back big chunks of changes (because it didn't work and I wasn't quite sure why), I really appreciate this approach.

Wrap Up
To wrap things up, whether you need to modify an existing code base or you are just looking at techniques to make your code more testable, Working Effectively with Legacy Code is an excellent read.  Note: none of the examples are in C#, but they are in C-family languages (C, C++, and Java), but C# is mentioned in many of the techniques.  (There's also one Ruby example for a technique that deals with dynamic languages.)  If you are comfortable reading C#, you should be able to understand the examples just fine.

I'm always looking to improve my coding techniques, and I'm sure that I will be thumbing through this book many times in the coming years.

Happy Coding!