[QFJ-109] Session management endDay calculation is bad (I would say wrong) Created: 16/Nov/06 Updated: 12/Apr/07 Resolved: 17/Nov/06 |
|
Status: | Closed |
Project: | QuickFIX/J |
Component/s: | Engine |
Affects Version/s: | 1.0.4 |
Fix Version/s: | 1.1.0 |
Type: | Bug | Priority: | Default |
Reporter: | Welf Wustlich | Assignee: | Steve Bate |
Resolution: | Fixed | Votes: | 0 |
Labels: | None | ||
Environment: |
linux, netbeans |
Description |
Since session management is important for a good engine (and QFJ is my favourite) I spent the afternoon a little in testing and debugging. if (endTime.getDay() != -1 My suggestion is, to make it more precise with: The important change is the little change ">=" to ">" in both if's If you are in hurry you can finish reading here . For most applications this works practically, but there are some problems with the original construction: 1. for testing I have tried to test with small sessions (less than one hour) these sessions were able to start, but did not stop, even if I have put the same startDay and endDay in the config, the endDay was set to the next week. 2. I can not see any good reason for adding on day to the end day only because start and end are in the same hour, but there is a good reason for doing so, when startTime > endTime (thatswhy my suggestion) 3. If you take a look at the code (I mean the whole class SessionSchedule.java, the code is not very nice, no documentation, no clean structure, not effective. To understand what I mean, just take a look at the if-constructions in the method: MostRecentIntervalBefore Now the good news: Well this is a recommendation but in some cases for example if you run multiple different sessions with the same engine you can not restart the engine but the session management should work properly like described. Well, slowly QFJ looks more and more friendly. Good night from germany, Welf |
Comments |
Comment by Steve Bate [ 17/Nov/06 ] |
I've fixed the bug for the very short sessions and did some refactoring and cleanup of the code. I've also added two unit tests (~ 10 test cases) for the bug fix. |