Automated tournament shortened name derivation#112
Conversation
| @@ -206,10 +210,11 @@ impl Tournament { | |||
| patch: TournamentPatch, | |||
| pool: &Pool<Postgres>, | |||
| ) -> Result<Tournament, OmniError> { | |||
| let name = patch.full_name.unwrap_or(self.full_name); | |||
| let tournament = Tournament { | |||
| id: self.id, | |||
| full_name: patch.full_name.unwrap_or(self.full_name), | |||
| shortened_name: patch.shortened_name.unwrap_or(self.shortened_name), | |||
There was a problem hiding this comment.
Provided that my understanding is correct, the shortened name will always be generated using your function. This is wrong, as the user should be able to define their own shortened name, which is a part of a tournament branding.
| fn shorten(word: &str) -> String { | ||
| let len = word.chars().count(); | ||
| let id = short_id(); | ||
|
|
||
| if len <= 5 { | ||
| return capitalize(&format!("{}{}", word, id)); | ||
| } | ||
|
|
||
| let first = word.chars().next().unwrap(); | ||
| let last = word.chars().last().unwrap(); | ||
|
|
||
| capitalize(&format!("{}{}{}{}", first, len - 2, last, id)) | ||
| } | ||
|
|
||
| fn short_id() -> String { | ||
| let now = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_nanos(); | ||
|
|
||
| let count = COUNTER.fetch_add(1, Ordering::Relaxed) as u128; | ||
|
|
||
| let value = now ^ count; | ||
|
|
||
| base36(value % 1679616) // 36^4 = 4 chars | ||
| } | ||
|
|
||
| fn base36(mut value: u128) -> String { | ||
| const CHARS: &[u8] = b"0123456789abcdefghijklmnopqrstuvwxyz"; | ||
|
|
||
| if value == 0 { | ||
| return "0".to_string(); | ||
| } | ||
|
|
||
| let mut result = Vec::new(); | ||
|
|
||
| while value > 0 { | ||
| let index = (value % 36) as usize; | ||
| result.push(CHARS[index] as char); | ||
| value /= 36; | ||
| } | ||
|
|
||
| result.iter().rev().collect() | ||
| } | ||
|
|
||
| fn capitalize(word: &str) -> String { | ||
| let mut chars = word.chars(); | ||
|
|
||
| match chars.next() { | ||
| None => String::new(), | ||
| Some(first) => first.to_uppercase().collect::<String>() + chars.as_str(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Why did you try so hard to guarantee the shortened name's uniqueness, while there's aren't any business requirements imposing it? Given the task at hand (automatically shortening a tournament's name), don't you think this piece of code is overly complex? Also, this is purely subjective, but I can imagine some users perceiving random, inexplicable strings with numbers and letters to be straight-out ugly. Capitalization of first letters of each word (e.g. This is a test tournament => TIATT would be enough.
There was a problem hiding this comment.
While the existing tests are modified, the implemented feature isn't. This should be changed.
| async fn tournament_creation_should_be_possible_for_infrastructure_admin() { | ||
| // GIVEN | ||
| let app = TestApp::spawn().await; | ||
| let short_name_str = "WrLD"; | ||
| let shortened_name: Option<String> = Some(short_name_str.to_string()); | ||
|
|
||
| // WHEN | ||
| let token = get_session_token_for_infrastructure_admin(&app).await; | ||
| let res = create_tournament(&app, "Wrocławska Liga Debat", &token).await; | ||
| let res = | ||
| create_tournament(&app, "Wrocławska Liga Debat", shortened_name, &token).await; | ||
|
|
||
| // THEN | ||
| assert_eq!(res.status(), StatusCode::OK); | ||
| } |
There was a problem hiding this comment.
There should be an assertion on whether the user-defined shortened name is preserved or not. Similarly, generating the shortened name should be tested.
| fn shorten(name: &str) -> String { | ||
| let words: Vec<String> = name | ||
| .split_whitespace() | ||
| .map(|word| { | ||
| word.chars() | ||
| .filter(|c| c.is_alphabetic()) | ||
| .collect::<String>() | ||
| }) | ||
| .filter(|word| !word.is_empty()) | ||
| .collect(); | ||
|
|
||
| let mut result = String::new(); | ||
|
|
||
| match words.len() { | ||
| 0 => {} | ||
| 1 => { | ||
| result.extend(words[0].chars().take(3)); | ||
| } | ||
| 2 => { | ||
| result.extend(words[0].chars().take(2)); | ||
| result.extend(words[1].chars().take(1)); | ||
| } | ||
| _ => { | ||
| for word in words.iter().take(3) { | ||
| result.extend(word.chars().take(1)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result.iter().rev().collect() | ||
| capitalize(&result) | ||
| } |
There was a problem hiding this comment.
Why is this function declared both here and in mod.rs? Can't it be defined in one place and imported in the other?
| fn shorten(name: &str) -> String { | ||
| let words: Vec<String> = name | ||
| .split_whitespace() | ||
| .map(|word| { | ||
| word.chars() | ||
| .filter(|c| c.is_alphabetic()) | ||
| .collect::<String>() | ||
| }) | ||
| .filter(|word| !word.is_empty()) | ||
| .collect(); | ||
|
|
||
| let mut result = String::new(); | ||
|
|
||
| match words.len() { | ||
| 0 => {} | ||
| 1 => { | ||
| result.extend(words[0].chars().take(3)); | ||
| } | ||
| 2 => { | ||
| result.extend(words[0].chars().take(2)); | ||
| result.extend(words[1].chars().take(1)); | ||
| } | ||
| _ => { | ||
| for word in words.iter().take(3) { | ||
| result.extend(word.chars().take(1)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result.iter().rev().collect() | ||
| capitalize(&result) | ||
| } |
There was a problem hiding this comment.
Please cover this function with a unit test.
Mateusz-Dobrzynski
left a comment
Mateusz-Dobrzynski
left a comment
There was a problem hiding this comment.
It seems that the only thing to fix now are the merge conflicts.
…rivation-automatic
Mateusz-Dobrzynski
deleted the
SCRUM-94-Make-tournament-shortened-name-derivation-automatic
branch
2 participants
Automated tournament shortened name derivation
Modified all tournaments-related files so shortened name is generated automatically on the tournament name basis.
Example of name shortening:
"ThisIsMyTestTournament" -> "T20tl0mk"Logic behind name shortening:
Testing
cargo test --testsand ensured that they pass.