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

tournament: Add JSON file #287

Merged
merged 1 commit into from
Jul 20, 2016
Merged

tournament: Add JSON file #287

merged 1 commit into from
Jul 20, 2016

Conversation

petertseng
Copy link
Member

@petertseng petertseng commented Jul 3, 2016

For the most part, the existing implementations all used the same tests:

xgo is the clear outlier: Whereas all other tracks ignore invalid lines,
xgo declares that inputs with invalid lines cause an error (but empty
lines and comments are okay). Please refer to discussion in #286 where
we decide how to make this consistent across tracks.

For now, these tests follow the behavior of the other tracks.

xgo also had a test for ties which no other track had, so that's been
added. This tests that ties are broken alphabetically.

Discussion point: The README specifies that ties are broken first in
favor of the team with more wins. It's not possible to get a tie with a
different number of wins in a four-team group (one team must get one
win and no draws, while another team must get three draws and no wins).
We can test this by doing one of two things:

  • A group with five teams
  • A group where a pair plays each other more than once.

Should we test such a thing? Note that this is not usually seen in
football tournaments so such a test may be surprising to students.

Behaviors not tested:

  • What happens on an empty input? Should the header be printed, with no
    rows in the result table?
  • Should a team not be allowed to play itself? Should we test this? If
    so, it should be declared in the README that it's invalid for a team
    to play itself, of course.

Closes https://github.com/exercism/todo/issues/155

@petertseng
Copy link
Member Author

petertseng commented Jul 3, 2016

Things I'd like to happen before this gets merged:

  • should we test empty input? If so, what should the expected output be? (current answer: no)
  • should we test what happens when a team plays itself? If so, we should edit the README (current answer: no)
  • should we explicitly test the tiebreaking logic that breaks ties in favor of team with more wins? In that case, we'd have to add a fifth team to a group, or allows pair to play each other more than once. (current answer: no, remove that line from README)
  • I may have had copy/paste errors, and also I had to make up a new case to as not to test the tie logic earlier than necessary. It's not appropriate to merge this until I verify that the results are correct.

Sorry, something went wrong.

@IanWhitney
Copy link
Contributor

  1. I don't think we need to test empty input. I've definitely seen some exercises suffer from a case of Edge-itis, where we try to test every weird edge case. Some edges can be interesting, but it also helps to remember that we're not writing production-ready code here.
  2. Same.
  3. Or just drop that tiebreak sentence from the Readme. That also solves the problem.

]
},
{
"description": "incomplete competition (not all pairs have played)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't there some restriction on this problem that only complete tournaments could be tallied? I remember seeing that, but now I can't find it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see such a thing, and most tracks are testing this case as well

@IanWhitney
Copy link
Contributor

I have the one question about an incomplete tournament, but the test input & expectations seem fine to me.

@petertseng
Copy link
Member Author

petertseng commented Jul 12, 2016

OK, so my plan is: I'll merge this once I check that the test expectations are correct. I still need to do that. If someone wants to beat me to it feel free, but don't worry too much since I'm pretty sure I'll get to it.

In a separate PR, the README probably needs a tweak on the tiebreak rule since we won't see ties needing to be broken by # wins in things we test.

@petertseng
Copy link
Member Author

petertseng commented Jul 20, 2016

Merging with this diff on top (correct Alaskians to Alaskans)

$ git diff
diff --git a/tournament.json b/tournament.json
index a42170c..b674454 100644
--- a/tournament.json
+++ b/tournament.json
@@ -44,11 +44,11 @@
         "description": "ties broken alphabetically",
         "input": [
           "Courageous Californians;Devastating Donkeys;win",
-          "Allegoric Alaskians;Blithering Badgers;win",
-          "Devastating Donkeys;Allegoric Alaskians;loss",
+          "Allegoric Alaskans;Blithering Badgers;win",
+          "Devastating Donkeys;Allegoric Alaskans;loss",
           "Courageous Californians;Blithering Badgers;win",
           "Blithering Badgers;Devastating Donkeys;draw",
-          "Allegoric Alaskians;Courageous Californians;draw"
+          "Allegoric Alaskans;Courageous Californians;draw"
         ],
         "expected": [
           "Team                           | MP |  W |  D |  L |  P",
@@ -81,7 +81,7 @@
       },
       {
         "description": "invalid match result",
-        "input": "Devastating Donkeys;Allegoric Alaskians;dra"
+        "input": "Devastating Donkeys;Allegoric Alaskans;dra"
       }
     ]
   }

For the most part, the existing implementations all used the same tests:

* https://github.com/exercism/xcsharp/blob/master/exercises/tournament/TournamentTest.cs
* https://github.com/exercism/xfsharp/blob/master/exercises/tournament/TournamentTest.fs
* https://github.com/exercism/xgo/blob/master/exercises/tournament/tournament_test.go
* https://github.com/exercism/xrust/tree/master/exercises/tournament/tests
* https://github.com/exercism/xswift/blob/master/exercises/tournament/TournamentTest.swift

xgo is the clear outlier: Whereas all other tracks ignore invalid lines,
xgo declares that inputs with invalid lines cause an error (but empty
lines and comments are okay). Please refer to discussion in #286 where
we decide how to make this consistent across tracks.

For now, these tests follow the behavior of the other tracks.

xgo also had a test for ties which no other track had, so that's been
added. This tests that ties are broken alphabetically.

Behaviors not tested:
* Empty input.
* Team playing itself.

Closes https://github.com/exercism/todo/issues/155
@petertseng
Copy link
Member Author

Verified:

require 'json'

data = JSON.parse(File.read('tournament.json'))

class Team
  attr_reader :name

  def initialize(name)
    @name = name.freeze
    @wins = 0
    @losses = 0
    @draws = 0
  end

  def plays
    @wins + @losses + @draws
  end

  def points
    @wins * 3 + @draws
  end

  def win
    @wins += 1
  end

  def lose
    @losses += 1
  end

  def draw
    @draws += 1
  end

  def to_s
    '%-30s | %2d | %2d | %2d | %2d | %2d' % [@name, plays, @wins, @draws, @losses, points]
  end
end

HEADER = ('%-30s | MP |  W |  D |  L |  P' % 'Team').freeze

def tally(lines)
  teams = Hash.new { |h, k| h[k] = Team.new(k) }
  lines.each { |line|
    parts = line.chomp.split(?;)
    next unless parts.size == 3
    t1, t2, r = parts
    t1 = teams[t1]
    t2 = teams[t2]
    case r
    when 'win'
      t1.win
      t2.lose
    when 'loss'
      t2.win
      t1.lose
    when 'draw'
      t1.draw
      t2.draw
    end
  }
  HEADER + "\n" + teams.values.sort_by { |t| [-t.points, t.name] }.join("\n")
end

data['valid_inputs']['cases'].each_with_index { |c, i|
  got = tally(c['input'])
  want = c['expected'].join("\n")
  raise "#{c} got #{got}" if got != want
  puts "#{i} OK"
}

valid = data['valid_inputs']['cases'].first['input']
want = tally(valid)
data['invalid_lines']['cases'].each_with_index { |c, i|
  got = tally(valid + [c['input']])
  raise "#{c} got #{got}" if got != want
  puts "#{i} OK"
}

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.

None yet

2 participants