CODEWITHSUNDEEP
c#.net
Nebseb
Nebseb

Reputation: 39

Issue with If Check in C#

OK, So here is my if check, no matter how I've tried it, it's not going through properly, it keeps stopping.

DateTime now = new DateTime();
string s = "Everyday";
string currentTime = now.ToString("HH:mm");
string remDate = "21:00"; //set to a minute or two i the future
string CurrentDay = "Sunday";

if ((s.ToUpper() == "WORKDAYS") ||(s.ToUpper() == "EVERYDAY" || s.ToUpper() == CurrentDay.ToUpper()) 
&& ((currentTime == remDate) && (s.ToUpper() != "SUNDAY") && (s.ToUpper() != "SATURDAY")))
{
     MessageBox.Show("Success!!!!");
}

So when it hits the if check, it stops, never gets to the message box. No matter what order I've tried the checks it's not working, been staring at this for a while, thought someone out there could either show me a better way, or see what I'm missing.

Upvotes: 0

Views: 129

Answers (6)

Awtszs
Awtszs

Reputation: 341

replace:

string currentTime = now.ToString("HH:mm");

and

(currentTime == remDate)

by this:

DateTime currentTime = DateTime.Now

and 

(currentTime.ToString("HH:mm") == remDate)

Upvotes: 0

Nebseb
Nebseb

Reputation: 39

I got it working. I should have explained it better, what I was trying to do is have it check the time of day and then check the day, and then have an alert pop up, the values come from the end user, stored in a sqlite db.

The problem was the grouping and the order and I had a duplicate check in my code.

what i ended up with was:

if ((s.ToUpper() == "WORKDAYS") || (s.ToUpper() == "EVERYDAY" || s.ToUpper() == CurrentDay.ToUpper())
&& ((currentTime == remDate) && (s.ToUpper() != "SUNDAY") && (s.ToUpper() != "SATURDAY")))
{
}

I had it do all my OR checks first, then had it pull in the last group of AND's and then i grouped them, this works for me and does the job.

I'm writing my own reminder application and this was my weekday check that i implemented. Probably a better way to do it, but this was what i came up with.

Thanks for the help

Upvotes: 0

Thilipan Croose
Thilipan Croose

Reputation: 79

Here I assume that your variable CurrentDay is equal to DayOfWeek.Monday.ToString().ToUpper().

according to your statement, it looks like

if(false || true && false)

Because (s.ToUpper() == "WORKDAYS") is always false, until you change s.
and (s.ToUpper() == "EVERYDAY" || s.ToUpper() == CurrentDay.ToUpper()) is always true until you change s
and ((currentTime == remDate) && (s.ToUpper() != "SUNDAY") && (s.ToUpper() != "SATURDAY") && (CurrentDay == s)) is always false, because (CurrentDay == s) is false.

Hence if statement is always false, then MessageBox.Show("Success!!!!"); cannot be executed..

Upvotes: 0

Timothy Walters
Timothy Walters

Reputation: 16874

Lets start by making your code more readable, which also helps show your || and && issues.

DateTime now = new DateTime();
string s = "Everyday";
string currentTime = now.ToString("HH:mm");
string remDate = "21:00"; //set to a minute or two i the future
string CurrentDay = "Sunday";

if (
    // condition 1
    (s.ToUpper() == "WORKDAYS") 
    // condition 2
    || (
        s.ToUpper() == "EVERYDAY" 
        || s.ToUpper() == CurrentDay.ToUpper()
    ) 
    // condition 3
    && (
        // condition 3.1
        (currentTime == remDate) 
        && (s.ToUpper() != "SUNDAY") 
        && (s.ToUpper() != "SATURDAY")
    )
)
{
    // I'm using LINQPad, so I just output this to the results
    "Success!!!!".Dump();
}

One issue is condition 3.1: (currentTime == remDate). Given the sample input provided this has to be true, so your message box will only display when the time is 21:00.

Another issue is that condition 2 and condition 3 will (because they are joined with &&) be treated as a group. Is this what you really meant?

The logic of this if statement is hard to follow and the grouping or/and expressions add to the confusion.

Try writing the conditions out in english, maybe as a bullet list, and see if the mistake doesn't become obvious to you.

Upvotes: 0

x2.
x2.

Reputation: 9668

Cause it will work only at 21:00

UPD: Sorry, it was mistake. Real reason in first line You have to write DateTime now = DateTime.Now;

Upvotes: 3

Ehsan
Ehsan

Reputation: 32671

If you see that your s is Everyday, hence the first condition is met. But you have AND condition with

(currentTime == remDate) && (s.ToUpper() != "SUNDAY") && (s.ToUpper() != "SATURDAY")

which will only be true when time is 21:00

which isn't true at this time. Hence the condition is failed.

Upvotes: 0

Related Questions