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

Cumulative time limit (issue #5) #51

Merged
merged 3 commits into from Feb 14, 2020
Merged

Conversation

@danieljames-dj
Copy link
Member

danieljames-dj commented Feb 7, 2020

Just like converting the attempt to DNF, this will convert the last time(s) to DNF if cumulative time limit is crossed. This will allow the delegate to know while data entry that cumulative time limit is exceeded.

Daniel James
Copy link
Member

jonatanklosko left a comment

Hey Daniel, thanks for contributing and proposing the solution! 🚀 Leaving some comments for you, let me know if you have any doubts about them!

updatedAttempts[i] = -1;
}
}
} else if (value >= timeLimit.centiseconds) {

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Feb 7, 2020

Member

The first condition is not satisfied for cross-cumulative time limits (i.e. cumulativeRoundIds.length > 1), in which case we definitely don't want to apply the time limit to each of the solves, so this should go in pair with cumulativeRoundIds.length === 0.

const updatedValue =
timeLimit && value >= timeLimit.centiseconds ? -1 : value;
const updatedAttempts = setAt(attempts, index, updatedValue);
var updatedAttempts = setAt(attempts, index, value);

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Feb 7, 2020

Member

Please use const everywhere to stay consistent across the project (either way var should be avoided since let and const introduction, as these are block-scoped and thus most predictable).

While on that, functional approach is preferable here (partially enforced by React, but I'd rather stick to it in general), so please avoid any sort of mutations and imperative-style computation ^^

On a side-note, as this is quite some logic now, it would be best to move it to another module (logic/attempts.js), here's how I'd go about it (iterating over the attempts, keeping track of the sum, and producing an updated list of attempts):

const applyTimeLimit = (attempts, timeLimit) => {
  if (timeLimit.cumulativeRoundIds.length === 0) {
    return attempts.map(attempt =>
      attempt >= timeLimit.centiseconds ? -1 : attempt
    );
  } else {
    /* Note: for now cross-round cumulative time limits are handled
       as single-round cumulative time limits for each of the rounds. */
    const [updatedAttempts, sum] = attempts.reduce(
      ([updatedAttempts, sum], attempt) => {
        const updatedSum = attempt > 0 ? sum + attempt : sum;
        const updatedAttempt =
          attempt > 0 && updatedSum >= timeLimit.centiseconds ? -1 : attempt;
        return [updatedAttempts.concat(updatedAttempt), updatedSum];
      },
      [[], 0]
    );
    return updatedAttempts;
  }
};

(for cross-cumulative time limits it's still better to check them within each round, than do nothing)

Then here it would become

const updatedAttempts = applyTimeLimit(
  setAt(attempts, index, value),
  timeLimit
);

As you can see all the functions return new arrays, keeping the arguments untouched.

If you feel like it, you can also define a similar applyCutoff function to move the cutoff logic outside =)

Also please write some tests for that in logic/tests/attempts.test.js (meetsCutoff may be a good example to base on).

This comment has been minimized.

Copy link
@danieljames-dj

danieljames-dj Feb 8, 2020

Author Member

Hi Jonathan, thanks a lot for the detailed comments. It was really helpful.
I'm a bit confused because I couldn't find the code you wrote in the comment in the latest code. May I know whether you wrote it for me so that I can use that function?
If so, I'll write it and write a similar apply cutoff too as you said. Can I proceed?

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Feb 8, 2020

Member

Hey, none of that code already exists! All I meant was to define the applyTimeLimit function in the attemtps.js module and then use it here. I suggested defining similar applyCutoff to move this logic into the attempts.js module as well. These are the tests that may be a good start for testing the new functions =)

@danieljames-dj

This comment has been minimized.

Copy link
Member Author

danieljames-dj commented Feb 14, 2020

Hi @jonatanklosko , I've made the changes as you suggested. Thanks a lot for your suggestions. It was a great lesson for me in good practices of coding. I would like to take up more challenges.
Few comments:

  1. I've just copy-pasted your applyTimeLimit code because I couldn't find any improvements.
  2. Earlier, while calling meetsCutoff in ResultForm, one extra parameter eventId was passed, but it was not used in the meetsCutoff of attempts.js. I didn't remove it as I was not sure it can be removed. Please let me know if I can remove the extra parameter from all the places the function is used (it is used in disabledFromIndex as well).
  3. After writing the test cases, I tried running the test using jest attempts.test.js, but I got the error SyntaxError: Cannot use import statement outside a module. So, I couldn't test the test cases I wrote.
Copy link
Member

jonatanklosko left a comment

Thanks for writing the tests!

Sure, please remove the unnecessary argument, it must be some leftover ^^

As for running the tests, did you try npm run test? That should execute the tests in convenient watch mode (by default it runs tests for files that changed, so you can press a to run all the tests). Let me know if for some reason it doesn't work for you!

}
};

export const applyCutOff = (attempts, cutoff, eventId) => {

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Feb 14, 2020

Member

Nit: applyCutOff -> applyCutoff

]);
});

it('3th attempt becomes DNF because of exceeding cumulative time limit', () => {

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Feb 14, 2020

Member

Nit: 3th -> 3rd

});

describe('applyCutOff', () => {
it('3rd attempt becomes empty because the first two attempts did not meet cutoff', () => {

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Feb 14, 2020

Member

Nit: empty -> skipped

applyTimeLimit(
applyCutOff(setAt(attempts, index, value), cutoff, eventId),
timeLimit
)

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Feb 14, 2020

Member

It doesn't matter in most cases, but time limit should be applied before cutoff. Extreme example: we have a cumulative time limit of 5 minutes and a cutoff of 2 minutes, the competitor solves are 4:00.00 and 1:20.00, the second time is DNF due to time limit, and thus the competitor doesn't meet cutoff. At the end it probably doesn't make a difference, but I think that's a more logical order =)

@danieljames-dj

This comment has been minimized.

Copy link
Member Author

danieljames-dj commented Feb 14, 2020

Hi @jonatanklosko I've tried my best to implement all the fixes that you suggested. I hope I didn't miss anything. Please let me know if I missed anything.
Also, test is working with npm run test, thanks a lot for that too. :-)

Copy link
Member

jonatanklosko left a comment

Perfect, thanks Daniel! 🚢 🎉

@jonatanklosko jonatanklosko merged commit ad4d1ea into thewca:master Feb 14, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.