Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

I have been trying to optimize the filtering process of a Collection based on the DateTime GeneratedOn property of Alert class. Below is the code block which filters the List by taking in From Date and To Date.

 if (this.Alerts != null)
        {
            var fromDt = Convert.ToDateTime(this.FromDateAlert.ToShortDateString() + " " + this.FromTimeAlert.ToLongTimeString());
            var toDt = Convert.ToDateTime(this.ToDateAlert.ToShortDateString() + " " + this.ToTimeAlert.ToLongTimeString());

            if (fromDt > toDt)
            {
                Messages.InfoMessage("FromDate cannot be greater than ToDate", "Alert"); return;
            }
            this.IsBusy = true;
            var filteredAlerts = this.Alerts.Where(a => (DateTime.Parse(a.Created) >= fromDt) && (DateTime.Parse(a.Created) <= toDt));              
            this.PagedAlerts = new PagedCollectionView(filteredAlerts);
            this.IsBusy = false;
        }

Is there a better way to doing this. Your comments would be highly appreciated.

Here is the code snippet with suggested changes. 

 private DateTime _toDateAlert;
    public DateTime ToDateAlert
    {
        get
        {
            return this._toDateAlert;
        }
        set
        {
            this._toDateAlert = value;
            RaisePropertyChanged("ToDateAlert");
        }
    }

     private DateTime _fromDateAlert;
    public DateTime FromDateAlert
    {
        get
        {
            return this._fromDateAlert;
        }
        set
        {
            this._fromDateAlert = value;
            RaisePropertyChanged("FromDateAlert");
        }
    }

 public void FilterAlerts()
    {
        if (this.Alerts != null)
        {
            if (this.FromDateAlert > this.ToDateAlert)
            {
                Messages.InfoMessage("FromDate cannot be greater than ToDate", "Alert");
                return;
            }
            this.IsBusy = true;
            var filteredAlerts = this.Alerts.Where(a => a.Created >= this.FromDateAlert && a.Created <= ToDateAlert);
            this.PagedAlerts = new PagedCollectionView(filteredAlerts);
            this.IsBusy = false;
        }
    }

I have cleaned up all those unnecessary parsing blocks and 

FromTimeAlert and ToTimeAlert

ViewModel properties and bound the single

FromDateAlert and ToDateAlert

to both DatePicker and TimePicker controls.

I would be glad to hear from you guys if there is still room for optimisation.

share|improve this question
 
You mean you're trying to make this code faster? Did profiling tell you this code is really a bottleneck in your application? –  svick Aug 21 '12 at 10:28
2  
But my primary advice would be to try to avoid strings for representing dates and times as much as possible. Certainly don't use them to combine a date with a time, that will be slow and unreliable. –  svick Aug 21 '12 at 10:35
 
I want a very precise filtering that can even filter out records based on time, that's why I combined the Date and Time, by converting it to ToShortDateString() and ToLongTimeString(). –  v-syyad Aug 21 '12 at 12:00
2  
Yeah, but I don't see any reason why you would use strings for that. You should work only with the DateTimes directly. –  svick Aug 21 '12 at 17:49

1 Answer

up vote 2 down vote accepted

Why are you storing FromDate and FromTime seperately to begin with?

Instead of storing using four properties, use only two, storing both Date and TimeOfDay inside them:

public DateTime FromAlert { get; private set; }
public DateTime ToAlert { get; private set; }

In the following code:

var fromDt = Convert.ToDateTime(this.FromDateAlert.ToShortDateString() + " " + this.FromTimeAlert.ToLongTimeString());
var toDt = Convert.ToDateTime(this.ToDateAlert.ToShortDateString() + " " + this.ToTimeAlert.ToLongTimeString());

You are converting multiple DateTime values to strings in order to parse them again (this should immediately raise a red flag). When you use the DateTime fields like they were meant to (i.e. containing both Date and TimeOfDay), you can get rid of these redundant conversions:

if (FromAlert > ToAlert)
{
    Messages.InfoMessage("FromDate cannot be greater than ToDate", "Alert"); 
    return; //always put this on a separate line!!
}

I have been trying to optimize the filtering process of a Collection based on the DateTime
GeneratedOn property of Alert class.

And why are you parsing the string Alert.Created if you already have a DateTime Alert.GeneratedOn? Just use it:

var filteredAlerts = Alerts.Where(a => a.GeneratedOn >= fromDt && a.GeneratedOn <= toDt);
share|improve this answer
 
Hi @codesparkle thanks for your valuable post. For some strange and anonymous reason I had been getting the Alert's CreatedOn prop as a string. So I went up to that guy who was associated with this retrieving Alerts of a system from system center operations manager and asked him why can't you just get CreatedOn as DateTime instead of a string so he changed its type to DateTime. The whole mess of multiple conversions and parsing was 'cause of the CreatedOn prop being a string type. –  v-syyad Aug 23 '12 at 7:06

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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