Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTime.Now usage analyzer #9

Open
tugberkugurlu opened this issue Nov 17, 2014 · 4 comments
Open

DateTime.Now usage analyzer #9

tugberkugurlu opened this issue Nov 17, 2014 · 4 comments

Comments

@tugberkugurlu
Copy link

DateTime.Now is 99.9% of time is evil. Hmm, let's make it 100% 😄

The analyzer should analyze DateTime.Now usage and give warnings for usages. It should also suggest replace it with DateTime.UtcNow.

@sharwell
Copy link
Member

It should first prefer to change it to DateTimeOffset.Now, but also offer the secondary option to change it to DateTime.UtcNow.

@sharwell
Copy link
Member

Fields, parameters, and local variables of type DateTime could also be changed to DateTimeOffset to reduce the change of application errors.

@tugberkugurlu
Copy link
Author

DateTimeOffset.Now case would be really hard to cover as you implied. You need to possibly scan all the solution to do the refactoring which would be hard I assume.

I think it is possible to suggest more than one code action for a diagnostic. So, the simpliest one would be to replace it with DateTime.UtcNow which would be trivial. Later, it can have the DateTimeOffset.Now refactoring.

@mattjohnsonpint
Copy link

I love the idea of this analyzer. I've been advocating against DateTime.Now for quite awhile. It should also find any usages of DateTimeKind.Local, including various methods on DateTime such as ToLocalTime, ToUniversalTime, and IsDaylightSavingTime.

In client-side code (desktop, phone, store apps, etc), DateTime.Now should usually be replaced with DateTimeOffset.Now, since the local time zone is still applicable. The benefit here is that the offset doesn't get lost when communicated to the server, and that offset changes (like those caused by daylight saving time) are still accounted for.

However, on the server, the local time zone is almost never appropriate. Server applications should either work strictly with UTC, or use TimeZoneInfo to work with specific time zones. DateTimeOffset.Now isn't a good idea for server applications.

I'm not sure exactly how suggested replacements should be implemented. It's great that something would warn you about the problems of using local time. However, when you get in to automatic refactoring, swapping in DateTime.UtcNow where there was previously DateTime.Now is not necessarily a complete solution. That would create invalid data unless the developer takes the extra step to understand how the data impacts their system and compensate accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants