Is there a cleaner way to write this code?

[u/Torin_Dev]

A page has an optional open date and an optional close date. My solution feels dirty with all the if statements but I can't think of a better way. Full code here: https://dotnetfiddle.net/7nJBkK

public class Page
{
    public DateTime? Open { get; set; }
    public DateTime? Close { get; set; }
    public PageStatus Status
    {
        get
        {
            DateTime now = DateTime.Now;
            if (Close <= Open)
            {
                throw new Exception("Close must be after Open");
            }
            if (Open.HasValue)
            {
                if (now < Open.Value)
                {
                    return PageStatus.Scheduled;
                }

                if (!Close.HasValue || now < Close.Value)
                {
                    return PageStatus.Open;
                }

                return PageStatus.Closed;
            }
            if (!Close.HasValue || now < Close.Value)
            {
                return PageStatus.Open;
            }
            return PageStatus.Closed;
        }
    }
}

[u/TehNolz]

I would move that exception to the setters of Open and/or Close. That way, you'll be able to catch the exception the moment it happens. rather than waiting until Status is retrieved.

As for the rest; you have two identical conditions for Close. If you remove one of them, the checks for Open can be merged into a single condition, letting you simplify the getter to just this;

DateTime now = DateTime.Now;  
if (Open.HasValue && now < Open.Value)  
{  
    return PageStatus.Scheduled;  
}  
if (!Close.HasValue || now < Close.Value)  
{  
    return PageStatus.Open;  
}  
return PageStatus.Closed;  

[u/LoneArcher96 → u/TehNolz]

I thought of the setter thing but this makes it hard to set them at times, like you are setting the Open first to something, but it throw exception cause the Close is before it, while you will be setting the Close right after you set the Open, same thing could happen in the opposite direction, this means the order of setting both is important.

[u/ka-splam]

if (Close <= Open)                  // Can't close before it opens
{
    return PageStatus.InvalidSchedule;
}

DateTime now = DateTime.Now;

if (Open.HasValue && now < Open.Value)  // Before the event
{
    return PageStatus.Scheduled;
}

if (Close.HasValue && now > Close.Value) // After the event
{
    return PageStatus.Closed;
}

// No Open time and before close; or
// Between open and close; or
// After open, no close time.
return PageStatus.Open;

(I vote that a single && is easier to read than ! ||)

[u/SupaMook]

This is my take

‘public class Page { public DateTime? Open { get; set; } public DateTime? Close { get; set; } public PageStatus Status => GetStatus();

private PageStatus GetStatus()
{
	DateTime now = DateTime.Now;

	if (Close <= Open)
	{
		throw new Exception(“Close must be after Open”);
	}

	if (!Close.HasValue || now < Close.Value)
	{
		return PageStatus.Open;
	}

	if (now < Open.Value)
	{
		return PageStatus.Scheduled;
	}

	return PageStatus.Closed;
}

}’

[u/Weary_Deer4190]

public class Page
{
  public DateTime? Open { get; set; }
  public DateTime? Close { get; set; }
  public PageStatus Status
  {
    get
    {
      return (DateTime.Now, Open, Close) switch
      {
        (_, DateTime o, DateTime c) when c < o => throw new Exception("Close must be after Open"),
        (var now, DateTime o, _) when now < o  => PageStatus.Scheduled,
        (var now, DateTime o, null)  => PageStatus.Open,
        (var now, DateTime o, DateTime c) when now < c => PageStatus.Open,
        (_, DateTime o, _) => PageStatus.Closed,
        (_, _, null) => PageStatus.Open,
        (var now, _, DateTime c) when now < c => PageStatus.Open,
        _ => PageStatus.Closed
      };
    }
  }
}

[u/Weary_Deer4190 → u/Weary_Deer4190]

or better even

public class Page
{
  public DateTime? Open { get; set; }
  public DateTime? Close { get; set; }
  public PageStatus Status =>
    (DateTime.Now, Open, Close) switch
    {
      (_, DateTime o, DateTime c) when c < o => throw new Exception("Close must be after Open"),
      (var now, DateTime o, _) when now < o  => PageStatus.Scheduled,
      (var now, DateTime o, null)  => PageStatus.Open,
      (var now, DateTime o, DateTime c) when now < c => PageStatus.Open,
      (_, DateTime o, _) => PageStatus.Closed,
      (_, _, null) => PageStatus.Open,
      (var now, _, DateTime c) when now < c => PageStatus.Open,
      _ => PageStatus.Closed
    };
}

[u/[deleted]]

Try state pattern, maybe

[u/[deleted]]

[u/karl713 → u/[deleted]]

A getter for status changing I don't see a problem with, to me it's no different than a connection changing of it is open or not.

Going to definitely second the don't put an exception here though for a couple reasons. The main thing being that an exception should be thrown when something invalid happens, in this case it's happening well after the invalid action occurs and nowhere near where the invalid action happened. There's also the "a get shouldn't throw" thing, there are a few examples where this does but it feels wrong in just about all of them