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

Added checks for regulation 9m #215

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mckeenicholas
Copy link
Contributor

@mckeenicholas mckeenicholas commented Mar 27, 2024

Closes #186

I've only implemented the check on the server side as of now. I can add a client side check as well if necessary.

@OldManLink
Copy link

OldManLink commented Mar 27, 2024

There are still grammar mistakes, BTW: "rounds with less than N competitors..." should be "with fewer than N competitors..."

Also, just a suggestion, why not use the actual rule text which is subtly different, even if semantically the same as yours:

9m1) Rounds with 99 or fewer competitors must have at most two subsequent rounds.
9m2) Rounds with 15 or fewer competitors must have at most one subsequent round.
9m3) Rounds with 7 or fewer competitors must not have subsequent rounds.

@mckeenicholas
Copy link
Contributor Author

I've updated the text to use the regulation text. The original code had a check for having at least 8 competitors in the previous round which was formatted in the same way I wrote them, which was my reasoning for keeping it in the same format. (I also find "fewer than 100" more understandable than "99 or fewer", but its probably better to use the actual regulation text).

Enum.to_list(1..round.number - 1 // 1)
|> Enum.map(
fn round_offset ->
Round |> Round.where_sibling(round, -round_offset) |> Repo.one() |> Repo.preload(:results)
Copy link
Member

@jonatanklosko jonatanklosko Mar 30, 2024

Choose a reason for hiding this comment

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

Instead of querying each round separately, let's add list_previous_rounds. It should return rounds ordered by round number, and here we can reverse for the check.

0 -> "Rounds with 7 or fewer competitors must not have subsequent rounds"
1 -> "Rounds with 15 or fewer competitors must have at most one subsequent round"
2 -> "Rounds with 99 or fewer competitors must have at most two subsequent rounds"
_ -> :nil
Copy link
Member

Choose a reason for hiding this comment

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

The only other value we expected is nil (not found), so let's be explicit:

Suggested change
_ -> :nil
nil -> nil

(also, nil is an atom, but you don't need : because it's special, same with true and false :))

Comment on lines +267 to +269
0 -> "Rounds with 7 or fewer competitors must not have subsequent rounds"
1 -> "Rounds with 15 or fewer competitors must have at most one subsequent round"
2 -> "Rounds with 99 or fewer competitors must have at most two subsequent rounds"
Copy link
Member

Choose a reason for hiding this comment

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

Usually we keep {:error, _} messages lowercased. We transform to uppercase in other places as needed.

Suggested change
0 -> "Rounds with 7 or fewer competitors must not have subsequent rounds"
1 -> "Rounds with 15 or fewer competitors must have at most one subsequent round"
2 -> "Rounds with 99 or fewer competitors must have at most two subsequent rounds"
0 -> "rounds with 7 or fewer competitors must not have subsequent rounds"
1 -> "rounds with 15 or fewer competitors must have at most one subsequent round"
2 -> "rounds with 99 or fewer competitors must have at most two subsequent rounds"

Comment on lines +227 to +228
message = validate_previous_rounds(round)
if message do
Copy link
Member

Choose a reason for hiding this comment

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

To avoid nesting you can do this:

cond do
  Round.open?(round) ->
    {:error, ...}

  message = validate_previous_rounds(round) ->
    {:error, error}

  true ->
    Multi.new()
    |> ...
end

@jonatanklosko
Copy link
Member

@mckeenicholas thanks, a couple small comments. Also, please add one more test similar to this one:

test "open_round/1 returns an error if the previous round has less than 8 competitors as per #9m3" do
competition_event = insert(:competition_event)
round1 = insert(:round, number: 1, competition_event: competition_event)
round2 = insert(:round, number: 2, competition_event: competition_event)
for ranking <- 1..7, do: insert(:result, round: round1, ranking: ranking)
assert {:error, "rounds with less than 8 competitors cannot have a subsequent round"} ==
Scoretaking.open_round(round2)
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check that there are enough competitors to start the next round before opening a round
3 participants