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.

Sign up for GitHub

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

Jump to bottom

Handling Equal Start and End Times in IsBetween Method #152

Open
BaptisteSE opened this issue Feb 12, 2024 · 3 comments · May be fixed by #204
Open

Handling Equal Start and End Times in IsBetween Method #152

BaptisteSE opened this issue Feb 12, 2024 · 3 comments · May be fixed by #204

Comments

@BaptisteSE
Copy link

Greetings,

Using the DateTimeExtensions library, I noticed a potentially ambiguous case in the "IsBetween" method of the "TimeExtensions.cs" file. When the "startTime" and "endTime" are the same, the method does not explicitly specify the expected behavior. This could lead to different interpretations among library users.

Proposal:
To clearly handle this case, it would be helpful to add clarification in the method documentation or adjust the logic. A suggestion would be to consider this condition to cover the whole day, which could be a logical interpretation in many usage scenarios.

A suggested code change to reflect this logic is shown below :

public static bool IsBetween(this DateTime dateTime, Time startTime, Time endTime) {
    var currentTime = dateTime.TimeOfTheDay();
    // If startTime and endTime are equal, consider it as covering the entire day
    if (startTime.Equals(endTime)) {
        return true;
    }
    //start time is lesser or equal than end time
    if (startTime.CompareTo(endTime) <= 0) {
        //currentTime should be between start time and end time
        if (currentTime.CompareTo(startTime) >= 0 && currentTime.CompareTo(endTime) <= 0) {
            return true;
        }
        return false;
    } else {
        //currentTime should be between end time time and start time
        if (currentTime.CompareTo(startTime) >= 0 || currentTime.CompareTo(endTime) <= 0) {
            return true;
        }
        return false;
    }
}
@geesadbandara
Copy link

It would work fine if we made the change in
if (startTime.Equals(endTime)) { return true; }

to
if (startTime.Equals(endTime)) { if (currentTime.CompareTo(startTime) >= 0 && currentTime.CompareTo(endTime) <= 0) { return true; } }

@AhmedFaizanDev
Copy link

Hey is this issue resolved? Is not could I be assigned to this ?

@drippypop drippypop linked a pull request Oct 18, 2024 that will close this issue
Open
@joaomatossilva
Copy link
Owner

Why is this really an issue, do you have a test that proves the logic is wrong, or is it just the interpretation?

I just did a simple test like this and it passes.

        public void can_compare_same_time()
        {
            var startTime = new Time(14);
            var endTime = new Time(14);

            var date = new DateTime(2011, 1, 1, 14, 0, 0);

            Assert.IsTrue(date.IsBetween(startTime, endTime));
        }

If time is a moment, and all your test date, start instant, and end instant is the same, then it should be true.

If it's just a matter of interpretation, I'm completely open to accept PR's with code documentation or proper unit tests, cause my code 10 years ago it's beyond recognition 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix IsBetween method in TimeExtensions.cs drippypop/DateTimeExtensions
4 participants
@joaomatossilva @BaptisteSE @geesadbandara @AhmedFaizanDev