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.

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

Refactor loading chronologically last passed schedule #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pszemus
Copy link
Contributor

@pszemus pszemus commented Feb 24, 2023

Tested playlists:

<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2060-01-14 14:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="1999-01-11 11:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2000-01-12 12:00:00">

result (logs):

ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Wed Jan 12 12:00:00 CET 2000
ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Wed Jan 14 14:00:00 CET 2060
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2060-01-14 14:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2020-01-11 11:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2000-01-12 12:00:00">

result (logs):

ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Sat Jan 11 11:00:00 CET 2020
ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Wed Jan 14 14:00:00 CET 2060
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="1999-01-11 11:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2000-01-12 12:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2060-01-14 14:00:00">

result (logs):

ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Wed Jan 12 12:00:00 CET 2000
ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Wed Jan 14 14:00:00 CET 2060
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2020-01-11 11:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2000-01-12 12:00:00">
<playlist name="schedule1" playOnStream="test" repeat="false" scheduled="2060-01-14 14:00:00">

result (logs):

ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Sat Jan 11 11:00:00 CET 2020
ServerListenerStreamPublisher scheduled playlist: test on stream: test for:Wed Jan 14 14:00:00 CET 2060

Copy link
Contributor

@rogerlittin rogerlittin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for reducing the number of lines in the code but I don't like to modify the list inside the if condition check. How about

schedules.add(schedule);
if (schedule.start.before(now))
	schedules.removeIf(s -> s.start.before(schedule.start)))

I think it should have the same effect but intention is clear.

@pszemus
Copy link
Contributor Author

pszemus commented Feb 24, 2023

It won't work for:

<playlist scheduled="2060-01-14 14:00:00">
<playlist scheduled="2022-01-11 11:00:00">
<playlist scheduled="2000-01-12 12:00:00">

all 3 playlists are started (including both past playlists).

We could split my condition into 2 different if statements and introduce a local variable removed = schedules.removeIf(s -> s.start.before(schedule.start) to make it more obvious.

@pszemus
Copy link
Contributor Author

pszemus commented Feb 24, 2023

@rogerlittin does the following look better to you?

schedules.add(schedule);
if (schedule.start.before(now))
   schedules.stream()
	.sorted()
	.filter(s -> s.start.before(now) && s != schedule)
	.findFirst()
	.ifPresent(pastSchedule ->
	{
		if (pastSchedule.start.before(schedule.start))
			schedules.remove(pastSchedule);
		else
			schedules.remove(schedule);
	});

@rogerlittin
Copy link
Contributor

That's really just replacing one large block of code with another large block. The original PR is probably cleaner. I won't be able to merge it for at least a week so if you come up with a better solution in the meantime please update here.

@pszemus
Copy link
Contributor Author

pszemus commented Mar 1, 2023

Another idea: always keep the last past schedule at index 0:

if (!schedules.isEmpty() && schedule.start.before(now))
{
	ScheduledItem lastPastSchedule = schedules.get(0);
	if (!lastPastSchedule.start.before(now))
		schedules.add(0, schedule);
	else if (lastPastSchedule.start.before(schedule.start))
		schedules.set(0, schedule);
} else
	schedules.add(schedule);

@pszemus pszemus requested a review from rogerlittin March 21, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants