Change Json format for #2 by IsaacPD · Pull Request #14 · MetaStudents/SchoolSim

IsaacPD · 2018-08-04T17:42:11Z

Resolves #2 . See test.json for the new format.
"project" - has an int value that represents the total work required to complete the project
"homework" - is an array of int arrays with 2 values [size, duration], homeworks of that size will be assigned for duration # of classes [0, x] will "assign" no HW for x days.
"exceptions" - is an array of days which there are no lectures or hw assigned. i.e. for [x, y, z] the x class from the 0th class is canceled.

…igned at these points)

meygerjos

Don't worry about the daylight savings stuff if you don't want. The real solution (as opposed to a quick fix, which I had done) to it is to have a Day class with no time component which keeps tracks of numbers of days in months and leap years. Doing this real solution now is out of the scope of this PR. (You could do a quick fix though if you feel like it.)

meygerjos · 2018-08-05T13:49:52Z

+        var start = Util.splitAndParseInt(lecture.days[0].time.split("-")[0], ":");
+
+        this.startDate = new Date(lecture.start.year, lecture.start.month, lecture.start.day, start[0], start[1], 0);
+        var delta = lecture.days[0].weekday - startDate.getDay();


What if delta is negative? In this case it should be incremented by 7.

meygerjos · 2018-08-05T13:52:06Z

+        this.startDate = new Date(lecture.start.year, lecture.start.month, lecture.start.day, start[0], start[1], 0);
+        var delta = lecture.days[0].weekday - startDate.getDay();
+
+        startDate = DateTools.delta(this.startDate, DateTools.days(delta));


It is not guaranteed that startDate.getHours() does not change in this step. You are adding a whole multiple of 24 hours, but some days have 23 or 25 hours.

meygerjos · 2018-08-05T13:55:31Z

+        for (i in lecture.days) {
+            times.push(i.time);
+            weekdays.push(i.weekday);
+        }


Then why not have a times array and a weekdays array in the json like you originally suggested, since you are converting it to that anyway?

meygerjos · 2018-08-05T14:03:04Z

    private var workRate:Float;
-    private var lectures:Array<Lecture>;
+    private var lectures:Lecture.LectureObject;
    private var lectureNum:Int;


You can delete lectureNum if you are no longer using it.

meygerjos · 2018-08-05T14:16:37Z

+
+    //Returns the next date that a homwork will be due/lecture will occur
+    private function nextHomework(gameDate:Date):Date {
+        var nextIndex = (lectures.weekdays.indexOf(gameDate.getDay()) + 1) % lectures.weekdays.length;


What if there are two lectures on the same day? E.g. what if lectures.weekdays == [1,2,2,5]? You'll end up doing the Tuesday lecture over and over.

meygerjos · 2018-08-05T14:22:01Z

+        if (delta <= 0) {
+            delta = 7 + delta;
+        }
+        delta = DateTools.days(delta) + DateTools.hours(nextStart[0] - gameDate.getHours()) + DateTools.minutes(nextStart[1] - gameDate.getMinutes());


This could cause problems with daylight savings.

meygerjos · 2018-08-05T14:44:25Z

+        var nextIndex = (lectures.weekdays.indexOf(gameDate.getDay()) + 1) % lectures.weekdays.length;
+        var next = lectures.weekdays[nextIndex];
+        var nextStart = Util.splitAndParseInt(lectures.times[nextIndex].split("-")[0], ":");
+


This should be [1] because the homework gets assigned when the lecture ends.

meygerjos · 2018-08-05T14:54:23Z

+                }
+				var index = lecture.weekdays.indexOf(j);
+				// If lecture is not held within the range of this week
+				if (!Util.DayinRange(lecture.startDate, lecture.endDate, cursorDate))


Why is cursorDate incremented at the start of the for loop? Wouldn't this cause the last lecture to be canceled if it occurs on lecture.endDate?

meygerjos · 2018-08-05T14:55:21Z

+                    lecture.curLectureNum++;
+                    continue;
+                }
+				var index = lecture.weekdays.indexOf(j);


This won't work if lecture.weekdays is something like [1,3,5,2,4]. We could perhaps set index = lecture.weekdays[lecture.curLectureNum % lecture.weekdays.length instead.

meygerjos · 2018-08-05T14:58:01Z

+    public var homework:Array<Array<Int>>;
+    public var title:String;
+    public var exceptions:Array<Int>;
+    public var curLectureNum:Int;


Cool idea to include curLectureNum as a field of LectureObject. Maybe LectureObject could be an Iterator.

It occurs to me that curLectureNum is only used in Schedule, but not in Bar.

meygerjos

Here's an alternative approach that might be easier. First parse the json into an object, then convert that object into an array of lectures, similar to what was in the original json format. This conversion would handle all the mod stuff and exceptions and everything all at once, and you wouldn't need to handle it twice in Bar and Schedule. The only problem with this is it wouldn't as easily generalize to an "infinite mode" where you have the same schedule every week for eternity; but we might not care about such a mode. What do you think?

meygerjos · 2018-08-07T06:10:03Z

+
+		var sunday = DateTools.delta(endDate, -DateTools.days(7));
+		for (lecture in scheduleObject) {
+			var lecCursor:Int = lecture.curLectureNum;


I was wondering why we never increment lecture.curLectureNum but I get it finally! Bar.getHomework() increments lecture.curLectureNum, so that after a week has gone by, it has incremented lecture.weekdays.length times. Then when Schedule.makeCols() is called, it is already incremented the desired amount. makeCols() shouldn't increment it because that would mess up Bar.getHomework(), so instead it uses lecCursor, a temporary variable.

I don't know if this is the best approach but it works for now.

meygerjos · 2018-08-07T06:15:32Z

+
+		var delta:Float = next - gameDate.getDay();
+
+		if (delta < 0 || lectures.weekdays.length == 1) {


What if lectures.weekdays = [2,2]? Then this if block will never run!

Maybe you could have nextIndex == 0 as the condition here instead.

I see the issue here, nextIndex == 0 however wouldn't work in the case of lecture.weekdays = [1,3,5,2,2] so a better case is use the full delta on the line after the if statement and use delta <= 0

That's a good idea.

meygerjos · 2018-08-07T06:22:18Z

+
+				lectureCols.push(lectureCol);
+				if (lecture.weekdays.length == 1) break;
+			} while (lecture.weekdays[lecCursor % lecture.weekdays.length] >= lecture.weekdays[(lecCursor - 1) % lecture.weekdays.length]);


What if lecture.weekdays == [2,2]? Then the while condition will always be satisfied, and the break statements will never be run.

A better condition is lecCursor % lecture.weekdays.length == 0.

A better condition I propose is similar to that I mentioned above. That is check if delta between this lecture and the next one is < 0. This will additionally allow cycles such as these [1,3,4,4] where the 2nd 4 occurs during a time before the 1st 4. This however doesn't allow two week cycles of the type weekdays = [2,2], times = [10:30-11:30, 14:30-15:30] which is also currently not capable.

That's a good point that such cycles are not capable. Maybe my idea of doing two week cycles like weekdays = [1, 3, 5, 2, 4] is shortsighted because it doesn't allow for periods of more than 7 days between consecutive classes. What if we used weekdays = [1, 3, 5, 9, 11] instead and also had a field cycleLength = 14?

meygerjos · 2018-08-07T06:37:42Z

+				if (lecture.exceptions.indexOf(lecCursor) != -1) {
+					lecCursor++;
+					if (lecture.weekdays.length == 1) break;
+					continue;


These if blocks were confusing me for a while but now I understand. They occur in 2 places: 1) right before continue statements and 2) right before the end of the do block. This is because in both places we are about to evaluate the while condition. So the if blocks are an additional condition tacked on to the condition in the while statement.

In general,

do { [stuff1] if (something) { [stuff2] if (otherCondition) break; continue; } [stuff3] } if (otherCondition) break; } while (condition);

is equivalent to

do { [stuff1] if (something) { [stuff2] continue; } [stuff3] } } while (!otherCondition && condition);

It would be cleaner to do it the second way.

But it turns out that !otherCondition && condition is not sufficient either: see the comment below on the while condition.

IsaacPD · 2018-08-07T10:56:25Z

As for the alternative approach my main concern is how much memory it would take up to store all those arrays of lectures ahead of time. And you would additionally loose the functionality you mentioned above so it isn't really convincing me.

meygerjos · 2018-08-07T18:49:52Z

OK here's an alternative alternative approach. Don't convert into an array of lectures, convert into an iterator object that you can interact with kind of like an array, but which can be infinite, and which doesn't store all the lectures internally. All the exception stuff could be dealt with internally. And Bar and Schedule could use two different copies of this object so that they don't step on each others toes.

# Conflicts: # Source/Bar.hx # Source/Schedule.hx

IsaacPD · 2018-09-03T12:42:36Z

This line in Lecture.hx delta += Datetools.days(7); presents issues with daylight savings as well as making it incompatible with lectures that have schedules in the form of [Week A Tues, Week B Monday,...]

Isaac added 12 commits July 28, 2018 08:43

Changed tabs to be consisitent with conventions

8e4c33b

Changed tabs to be consisitent with conventions

8c58b59

Merge branch 'master' of https://github.com/IsaacPD/SchoolSim

b20ac33

Resolved conflicts by incorporating new changes

541be67

Suggested change and code for new json format

94d4694

Made lecture days independent from each other

909bc96

Fixed bug with time

b607a14

Merged from main branch and incorporated new Json format

881cf42

Added different kinds of homeworks for lectures to Json

764c6c0

Fixed bug where schedule had wrong colors and added projects to Json

d88964a

Merged color scheme changes

17fd97a

Added Handling of Exceptions to when lectures are held (HW is not ass…

9a85c8d

…igned at these points)

IsaacPD changed the title ~~resolves #2~~ Change Json format for #2 Aug 4, 2018

meygerjos requested changes Aug 5, 2018

View reviewed changes

Isaac added 3 commits August 6, 2018 16:43

Lectures can alternate weeks and be held multiple time in one day

d1d7e4f

removed reference to project for pull request

5862300

Fixed bug when reaching end date for lectures

7150580

meygerjos requested changes Aug 7, 2018

View reviewed changes

Isaac added 2 commits September 3, 2018 08:34

Made lectures an iterator, issues with daylight savings still present

0b5a91c

Merge remote-tracking branch 'origin/master'

dfdb412

# Conflicts: # Source/Bar.hx # Source/Schedule.hx


		var delta:Float = next - gameDate.getDay();

		if (delta < 0 \|\| lectures.weekdays.length == 1) {

Conversation

IsaacPD commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meygerjos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meygerjos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IsaacPD Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IsaacPD commented Aug 7, 2018

Uh oh!

meygerjos commented Aug 7, 2018

Uh oh!

IsaacPD commented Sep 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IsaacPD commented Aug 4, 2018 •

edited

Loading

meygerjos left a comment •

edited

Loading

IsaacPD Aug 7, 2018 •

edited

Loading