While loop issue when retrieving records from a sql database

Question

Ive been trying to get this working for about 6 hours now and cant seem to find the solution or the error in my code...

Basically the code SHOULD pick up every instance of a date/time that is 2 minutes past the current date/time but for some reason it only returns one instance. Ive tried so many ways of solving this i couldnt possibly write them all down.

I have two methods removedexpiredusers() and getexpiredusersdate().

Getexpiredusersdate() should retrieve each records date/time added and feed them through to removeexpiredusers() which should use that method to filter out which records are two minutes past the current date and if they are delete them.

The two minutes deletion time is purely for testing as the plan is to change this to delete the user account after 24 hours giving the user a temporary account for one day.

Here is my code so far any help appreciated

private void removeExpiredUsers()
{
    while (DateTime.Now >= getExpiredUsersDate().AddMinutes(2))
    {
        //remove the user
        dal.spDeleteExpiredUsers(getExpiredUsersDate());
    }
}
private DateTime getExpiredUsersDate()
{
    DateTime date = new DateTime();
    try
    {
        using (SqlDataReader datareader = dal.getUsers())
        {
            while (datareader.Read())
            {
                date = ((DateTime)datareader["DateAdded"]);
                MessageBox.Show(date.ToString());
            }
        }
    }
    catch (SqlException sqlex) { }
    catch (Exception ex) { }

    return date;
}

EDIT

Im going to include the stored procedures here also guys

GetUsers

Create proc spGetUsers
as
Begin
    SELECT * 
    FROM [User]
    ORDER BY Username
End

*spDeleteExpiredUsers

Create proc spDeleteExpiredUsers
@DateAdded datetime
as
Begin
    DELETE 
    FROM [User] 
    WHERE DateAdded = @DateAdded
End

You should comment out all the try-catch blocks so that we can debug better. — King King
– King King, Commented Aug 19, 2013 at 8:12
But the messagebox shows all the users or just the one? Because you return just the last date and only this last date is checked in the if that executes the delete operation — Steve
– Steve, Commented Aug 19, 2013 at 8:16
@Steve Hi steve, it shows just the one user, should there be a way to feed the other dates through? — Master Yoda
– Master Yoda, Commented Aug 19, 2013 at 8:21
@Steve He is returning a single date but isn't he deleting the details so for every loop he will fetch single but different value?? — Suraj Singh
– Suraj Singh, Commented Aug 19, 2013 at 8:31
If the messagebox shows only one date and you expect to find more than one user then the problem is not here but in the dal.GetUsers() call — Steve
– Steve, Commented Aug 19, 2013 at 8:31

Christian Phillips · Accepted Answer · 2013-08-19 09:19:43Z

2

Why dont you simplyfy this by creating a stored procedure which deletes users where based on your logic, i.e.

DELETE FROM UserTable WHERE DATEDIFF(day,getdate(),DateAdded) < -1

You can then call this stored procedure from your .NET application using ExecuteScalar (if you want number of rows effected) or just ExecuteNonQuery if you don't need any result.

I think it's better to handle something like this in SQL server. I tend to only use C# if there's some business rules that need to be parsed or applied, before issuing a delete. If it's a simple Time Comparison that decided on whether a user is removed, then keep it simple.

EDIT: I have updeted the SQL, this will delete anyone that was added more that 1 day ago.

edited Aug 19, 2013 at 9:19

answered Aug 19, 2013 at 8:34

Christian Phillips

18.8k8 gold badges55 silver badges85 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Master Yoda Over a year ago

What is the RegisteredDate() function you are using?

Christian Phillips Over a year ago

This is just psuedo code, Should be DateAdded (now I looked at the code again).

Master Yoda Over a year ago

ChristianDev, will this code then retrive the current date and compare it to the dates from the table?

Christian Phillips Over a year ago

I have edited the SQL for you, since that was onlu pseudo. It will remove anyone with a DateAdded greater than 1 day. Consider maybe setting a flag (soft delete), instead of a hard delete also.

Master Yoda Over a year ago

All answers were great but this one appears to be the best answer based on performance. Thank you Christian Dev

Mario S · Accepted Answer · 2013-08-19 08:48:36Z

2

Basically the code SHOULD pick up every instance of a date/time that is 2 minutes past the current date/time but for some reason it only returns one instance.

You are actually returning only one date (the last set) in the getExpiredUsersDate() method. Use a list instead. Somehting like this:

// Change return type to a list of DateTime's
private List<DateTime> getExpiredUsersDate()
{
    // Don't need this
    //DateTime date = new DateTime();

    var dates = new List<DateTime>();

    // The try-catch is unnecessary, 
    // the using-statement will catch all exceptions before the try-catch.
    //try
    //{
        using (SqlDataReader datareader = dal.getUsers())
        {
            while (datareader.Read())
            {
                // Add the date to the list.
                dates.Add((DateTime)datareader["DateAdded"]);

                MessageBox.Show(date.ToString());
            }
        }
    //}
    //catch (SqlException sqlex) { }
    //catch (Exception ex) { }

    // Return all dates.
    return dates;
}

And a shorter version could be:

private IEnumerable<DateTime> getExpiredUsersDate()
{
    using (SqlDataReader datareader = dal.getUsers())
    {
        while (datareader.Read())
        {
            yield return (DateTime)datareader["DateAdded"];

            MessageBox.Show(date.ToString());
        }
    }
}

Then in removeExpiredUsers() you can do something like this with linq:

private void removeExpiredUsers()
{
    var toRemove = getExpiredUsersDate()
        .Where(date => DateTime.Now >= date.AddMinutes(2));

    foreach (var date in toRemove)
    {
        dal.spDeleteExpiredUsers(date);
    }
}

But you can also do all of this in SQL as mentioned by christiandev.

edited Aug 19, 2013 at 8:48

answered Aug 19, 2013 at 8:25

Mario S

12k24 gold badges42 silver badges47 bronze badges

3 Comments

sloth Over a year ago

date => DateTime.Now => date.AddMinutes(2) should probably be date => DateTime.Now >= date.AddMinutes(2)

Mario S Over a year ago

@DominicKexel Thanks, typed to fast =)

Master Yoda Over a year ago

@Mario hey, the spDeleteExpiredUsers() method requires a datetime to pass to the stored procedure, is there a way of cycling through the list then to pass through to the method each time?

Collectives™ on Stack Overflow

While loop issue when retrieving records from a sql database

2 Answers 2

5 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related