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: Put inputs/expectations inline, not in files #152

Merged
merged 2 commits into from Jul 12, 2016

Conversation

Projects
None yet
2 participants
@petertseng
Member

petertseng commented Jul 2, 2016

The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function tally now takes only a &str (it previously took two
Path, one for input and one for output) and now returns only a
String (it previously returned a Result<u32> where the Ok
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:

format!("{:30} | {:2} | {:2} | {:2} | {:2} | {:2}",
team, games, r.wins, r.draws, r.losses, points)
}).collect();
header + &lines.join("\n")

This comment has been minimized.

@petertseng

petertseng Jul 2, 2016

Member

Note that for an empty input, this prints the header and nothing after. No test tests this behavior, so I have nothing to say about whether this appropriate.

@petertseng

petertseng Jul 2, 2016

Member

Note that for an empty input, this prints the header and nothing after. No test tests this behavior, so I have nothing to say about whether this appropriate.

Show outdated Hide outdated exercises/tournament/tests/tournament.rs
#[test]
fn test_good() {
assert_eq!(tournament::tally(&Path::new("tests/input1.txt"), &Path::new("tests/output1.txt")).unwrap(), 6);
file_equal("tests/output1.txt", "tests/expected1.txt");
let input = "Allegoric Alaskians;Blithering Badgers;win\n\

This comment has been minimized.

@petertseng

petertseng Jul 2, 2016

Member

your comment is accepted on whether you prefer to see \n\ at the end of every line or not, but if you choose "not", then all other lines will be unindented. Here's an example of what it looks like.

fn test_good() {
    let input = "Allegoric Alaskians;Blithering Badgers;win
Devastating Donkeys;Courageous Californians;draw";
    let expected = "Team                           | MP |  W |  D |  L |  P
Allegoric Alaskians            |  1 |  1 |  0 |  0 |  3"; // other expected lines omitted, this is just an example
    assert_eq!(tournament::tally(&input), expected);

As you can see, I chose the way I did because I didn't like the unindent.

@petertseng

petertseng Jul 2, 2016

Member

your comment is accepted on whether you prefer to see \n\ at the end of every line or not, but if you choose "not", then all other lines will be unindented. Here's an example of what it looks like.

fn test_good() {
    let input = "Allegoric Alaskians;Blithering Badgers;win
Devastating Donkeys;Courageous Californians;draw";
    let expected = "Team                           | MP |  W |  D |  L |  P
Allegoric Alaskians            |  1 |  1 |  0 |  0 |  3"; // other expected lines omitted, this is just an example
    assert_eq!(tournament::tally(&input), expected);

As you can see, I chose the way I did because I didn't like the unindent.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

then again, maybe that way is less confusing to students. maybe that is the way to go.

@petertseng

petertseng Jul 3, 2016

Member

then again, maybe that way is less confusing to students. maybe that is the way to go.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

I think this can be alleviated by adding a comment explaining to students that the leading whitespaces will be stripped.

@petertseng

petertseng Jul 3, 2016

Member

I think this can be alleviated by adding a comment explaining to students that the leading whitespaces will be stripped.

This comment has been minimized.

@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Does this work?

   let input = "Allegoric Alaskians;Blithering Badgers;win\n" +
                 "Devastating Donkeys;Courageous Californians;draw\n" +
                 "Devastating Donkeys;Allegoric Alaskians;win\n" +
                 "Courageous Californians;Blithering Badgers;loss\n" +
                 "Blithering Badgers;Devastating Donkeys;loss\n" +
                 "Allegoric Alaskians;Courageous Californians;win";
@IanWhitney

IanWhitney Jul 3, 2016

Contributor

Does this work?

   let input = "Allegoric Alaskians;Blithering Badgers;win\n" +
                 "Devastating Donkeys;Courageous Californians;draw\n" +
                 "Devastating Donkeys;Allegoric Alaskians;win\n" +
                 "Courageous Californians;Blithering Badgers;loss\n" +
                 "Blithering Badgers;Devastating Donkeys;loss\n" +
                 "Allegoric Alaskians;Courageous Californians;win";

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

Yes, with one small modification:

    let input = "Allegoric Alaskians;Blithering Badgers;win\n".to_string() +
        "Devastating Donkeys;Courageous Californians;draw\n" +
        "Devastating Donkeys;Allegoric Alaskians;win\n" +
        "Courageous Californians;Blithering Badgers;loss\n" +
        "Blithering Badgers;Devastating Donkeys;loss\n" +
        "Allegoric Alaskians;Courageous Californians;win";

Else, we would get:

tests/tournament.rs:5:17: 5:63 error: binary operation `+` cannot be applied to type `&'static str` [E0369]
tests/tournament.rs:5     let input = "Allegoric Alaskians;Blithering Badgers;win\n" +
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Anyway I think your suggestion combines the criteria of "Doesn't require you to realise that the trailing \ strips out the leading whitespace" and "doesn't make the code look weird", so I am going to apply it to all the strings!

@petertseng

petertseng Jul 3, 2016

Member

Yes, with one small modification:

    let input = "Allegoric Alaskians;Blithering Badgers;win\n".to_string() +
        "Devastating Donkeys;Courageous Californians;draw\n" +
        "Devastating Donkeys;Allegoric Alaskians;win\n" +
        "Courageous Californians;Blithering Badgers;loss\n" +
        "Blithering Badgers;Devastating Donkeys;loss\n" +
        "Allegoric Alaskians;Courageous Californians;win";

Else, we would get:

tests/tournament.rs:5:17: 5:63 error: binary operation `+` cannot be applied to type `&'static str` [E0369]
tests/tournament.rs:5     let input = "Allegoric Alaskians;Blithering Badgers;win\n" +
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Anyway I think your suggestion combines the criteria of "Doesn't require you to realise that the trailing \ strips out the leading whitespace" and "doesn't make the code look weird", so I am going to apply it to all the strings!

@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 2, 2016

Member

amusing fact: Since the files output1.txt, output2.txt, output3.txt were pre-provided before this commit, I believe this means an empty implementation that does nothing will pass the tests.

Edit: That comment is incorrect, because the number of lines read will still succeed, but it's close. What I mean is you don't need to write the output file.

Edit: Yup. See #153.

Member

petertseng commented Jul 2, 2016

amusing fact: Since the files output1.txt, output2.txt, output3.txt were pre-provided before this commit, I believe this means an empty implementation that does nothing will pass the tests.

Edit: That comment is incorrect, because the number of lines read will still succeed, but it's close. What I mean is you don't need to write the output file.

Edit: Yup. See #153.

pub fn tally(input_filename: &Path, output_filename: &Path) -> Result<u32> {
let reader = BufReader::with_capacity(2048, File::open(input_filename).unwrap());
let mut count = 0;
pub fn tally(input: &str) -> String {

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

I would like a reminder on the idiomatic-ness of str and String. I will see if I can find an answer myself.

@petertseng

petertseng Jul 3, 2016

Member

I would like a reminder on the idiomatic-ness of str and String. I will see if I can find an answer myself.

This comment has been minimized.

@petertseng

petertseng Jul 3, 2016

Member

http://hermanradtke.com/2015/05/03/string-vs-str-in-rust-functions.html argues for using &str as argument. I will keep that.

On the other hand String makes the most sense for what I output since I'm concatenating in write_tally

@petertseng

petertseng Jul 3, 2016

Member

http://hermanradtke.com/2015/05/03/string-vs-str-in-rust-functions.html argues for using &str as argument. I will keep that.

On the other hand String makes the most sense for what I output since I'm concatenating in write_tally

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 3, 2016

Contributor

This is good. I want to go through it a bit more carefully, but I don't think there's anything controversial here. As I mentioned in #122, no other languages use input/output files for this exercise. I don't think they are necessary.

Contributor

IanWhitney commented Jul 3, 2016

This is good. I want to go through it a bit more carefully, but I don't think there's anything controversial here. As I mentioned in #122, no other languages use input/output files for this exercise. I don't think they are necessary.

@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 3, 2016

Member

squashed in the string suggestion since I'm pretty sure it's the way to go

Member

petertseng commented Jul 3, 2016

squashed in the string suggestion since I'm pretty sure it's the way to go

@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 3, 2016

Member

a small rebase since both this and #155 removed some use lines (I just had to tell git "yup, all of them should remain removed")

Member

petertseng commented Jul 3, 2016

a small rebase since both this and #155 removed some use lines (I just had to tell git "yup, all of them should remain removed")

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 5, 2016

Contributor

With the removal of the File IO stuff, this problem could probably now be moved to a different spot in the order. It still requires a fair amount of work, but it's mostly just string parsing and some comparisons. Maybe it should move to somewhere in these problems?

  • hexadecimal
  • grade-school
  • queen-attack
Contributor

IanWhitney commented Jul 5, 2016

With the removal of the File IO stuff, this problem could probably now be moved to a different spot in the order. It still requires a fair amount of work, but it's mostly just string parsing and some comparisons. Maybe it should move to somewhere in these problems?

  • hexadecimal
  • grade-school
  • queen-attack
@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 5, 2016

Member

Yeah now the list of skills is "string parsing, custom sorting, string formatting". I'm going to say after grade-school; both are problems that may see use of a HashMap to solve (store students by grade, store reults by team). Of course, if we have a desire to not put them together since they have too similar things, just say the word (that's why I mention it!)

Member

petertseng commented Jul 5, 2016

Yeah now the list of skills is "string parsing, custom sorting, string formatting". I'm going to say after grade-school; both are problems that may see use of a HashMap to solve (store students by grade, store reults by team). Of course, if we have a desire to not put them together since they have too similar things, just say the word (that's why I mention it!)

petertseng added some commits Jul 2, 2016

tournament: Put inputs/expectations inline, not in files
The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function `tally` now takes only a `&str` (it previously took two
`Path`, one for input and one for output) and now returns only a
`String` (it previously returned a `Result<u32>` where the `Ok`
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:
* Explain the input format. Covered by
  exercism/problem-specifications#254
* Explain behavior on invalid lines. Covered by
  exercism/problem-specifications#286
config: Move tournament -> between grade-school and robot-simulator
With the removal of the file I/O portion of this exercise (via the
previous commit and through discussion in #122) now this exercise
becomes easier and can be moved. The list of skills are now roughly:
string parsing, string formatting, custom sorting.

Also HashMap usage seems natural, as you want to store the results of
each team.

Moving it to after grade-school, another problem that may see HashMap in
its solution.
@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 9, 2016

Member

Oh, I originally moved between grade-school and queen-attack.

In the meantime, robot-simulator was also added to the exact same position.

I'll add it before robot simulator.

Member

petertseng commented Jul 9, 2016

Oh, I originally moved between grade-school and queen-attack.

In the meantime, robot-simulator was also added to the exact same position.

I'll add it before robot simulator.

@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 9, 2016

Member

Merging soon since it seems that all that wants to be said has been said.

Member

petertseng commented Jul 9, 2016

Merging soon since it seems that all that wants to be said has been said.

@IanWhitney IanWhitney referenced this pull request Jul 10, 2016

Closed

Add transpose exercise #151

@petertseng petertseng merged commit 5ff7d0c into exercism:master Jul 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@petertseng petertseng deleted the petertseng:tournament-inline branch Jul 12, 2016

petertseng added a commit to petertseng/exercism-rust that referenced this pull request Sep 11, 2016

move tournament between wordy and custom-set
In the beginning, tournament was between parallel-letter-frequency and
rectangles.

Then, in #152, File I/O was removed so it was deemed to be easier. So we
moved it to between grade-school and robot-simulator.

However, #174 rightly points out that tournament is still a bit involved
simply becaue there are multiple moving parts to it and the path to a
solution that even passes the first test is a bit long.

This placement is a general feeling from how much work I think one would
need to do on the input before being able to get the expected output:

In tournament, you have to look at each line, parse it, look at the
result, increment two win/draw/loss counts (one for each team that
played). then at the end calculate scores and sort.

wordy also has string parsing, so let's try right after wordy.

Closes #174
@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Mar 12, 2017

Member

Amazing. According to exercism/problem-specifications#22 , tournament was intentionally designed to include file I/O. The Rust track was the first track to have this exercise.

Looks like that purpose has been lost since then, both in this track and others. C# is the only track with vestiges of that file I/O since it uses streams.

I generally think the file I/O portion is not well-suited for the testing style of Exercism, but I see that above we considered whether there can be an exercise specifically dedicated for file I/O (the grep exercise ostensibly requires file I/O too).

I am sorry to the creator of this exercise for playing my part in the exercise's diversion from its original purpose. It was in the name of giving the exercise some purpose at all.

Member

petertseng commented Mar 12, 2017

Amazing. According to exercism/problem-specifications#22 , tournament was intentionally designed to include file I/O. The Rust track was the first track to have this exercise.

Looks like that purpose has been lost since then, both in this track and others. C# is the only track with vestiges of that file I/O since it uses streams.

I generally think the file I/O portion is not well-suited for the testing style of Exercism, but I see that above we considered whether there can be an exercise specifically dedicated for file I/O (the grep exercise ostensibly requires file I/O too).

I am sorry to the creator of this exercise for playing my part in the exercise's diversion from its original purpose. It was in the name of giving the exercise some purpose at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment