1
\$\begingroup\$

I am wondering if should change my if statement that compares two dates if they are equal. Is there a better way of doing this operation? The code does what it supposed to do. No errors. The Code:

if((Get-date).ToString("yyy-MM-dd") -eq $ListItem["Aviseringsdatum"].ToLocalTime().ToString("yyy-MM-dd")){
Write-Output "True"
}
\$\endgroup\$
3
\$\begingroup\$

I think it's okay, but we can do better. Let's see what Get-Date returns. Run this:

> get-date | get-member

It shows that Get-Date returns a DateTime object, and it also lists all the properties that has. I notice that DateTime has a Date property. Looking on the Web, it says that this is the date part of the DateTime but with a zero time component. So let's use that instead of converting to "yyy-MM-dd":

if((Get-date).Date -eq $ListItem["Aviseringsdatum"].Date){
Write-Output "True"
}

Since this is a code review, let's improve the formatting, and indent and add spaces:

if ((Get-Date).Date -eq $ListItem["Aviseringsdatum"].Date) {
    Write-Output "True"
}

I think that's as far as you need to go. If it were my code, however, I would probably go a bit further.

I would use [DateTime]::Today instead (Get-Date).Date because I think it is clearer.

I would also introduce local variables. I think if you are ever using a "today" or "now" value in your code, then it's better to capture them in a variable for the sake of consistency so that all parts of the script agree on what "today" or "now" is. What if for instance your script ran over midnight? Then the (Get-Date).Date value would suddenly change. A local variable might be overkill in this case, but it's a good practice in general to avoid some subtle bugs.

I also like making local variables to capture bigger expressions because they make for self-documenting code. It might be overkill again in this case, but I will add a $notificationDate variable.

$today = [DateTime]::Today
$notificationDate = $ListItem["Aviseringsdatum"].Date
if ($notificationDate -eq $today) {
    Write-Output "True"
}

I think that $notificationDate -eq $today makes the logic very clear to anyone reading the code.

\$\endgroup\$
  • \$\begingroup\$ Thank you sir! This is exactly what I need. A Structrued way to improve my undestanding of well structured code. I tend to not break down the code into varibales. Thanks again! \$\endgroup\$ – AllramEst Jan 11 '18 at 8:01
  • \$\begingroup\$ Isn't there a presicision issue with equating dates? I seem to remember to have to use Get-Date and set the -Hour, -Minute and -Second parameters to zero. \$\endgroup\$ – John Donnelly Jan 11 '18 at 15:34
  • 1
    \$\begingroup\$ @JohnDonnelly, that's why we use the Date property. Evaluate this in PowerShell: (Get-Date).Date, or (Get-Date).Date.TimeOfDay. You will see that the hours, minutes, etc. are zero. \$\endgroup\$ – Dangph Jan 12 '18 at 1:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.