ctm / mb2-doc

Mb2, poker software
https://devctm.com
7 stars 2 forks source link

Big O was incorrectly labeled "(High Only)" #1412

Closed ctm closed 4 months ago

ctm commented 4 months ago

Fix so "Big O" is called "Big O" and not "Big O (High Only)" and make sure it doesn't happen with any other games.

This is a regression I introduced when I made it so Options implements Display. It needs to know that some games are implicitly high low. I thought I did that, but obviously I made at least one mistake.

ctm commented 4 months ago

It looks like TwoOrFive also has this problem. I need to review the description method that I removed from all the games. That's where the code was for BigO.

ctm commented 4 months ago

Fixed. Deploying now.

ctm commented 4 months ago

Here's a post-mortem for the curious.

It may seem like amazingly bad programming to have a game label itself "(High Only)" when it is actually high/low with an eight qualifier. One would think that there would be a canonical source of whether a game is high low or not and that canonical source would be used for both the name of the game as well as deciding how to award a pot.

One of the sources of the problem is that some games' names should not include a parenthetical clarification of how the pot is split. Consider Binglaha. "Binglaha (High/Low Eight Qualifier)" would be misleading and "Binglaha (High Only or High/Low Eight Qualifier, depending on roll of a die)" would be a bit long. How the pot is split, in Binglaha, is baked into the game via a die roll, so "Binglaha" is by far the best choice.

Initially, mb2 had a sense of a default way the pot would be split or not for each "game" and it only added a parenthetical clarification when a non-default was chosen. Texas Hold'em, for example, would just be "Hold'em" if it were spread high-only and "Hold'em (High/Low Eight Qualifier)" if it were played high/low with an eight qualifier. Some people did not like that, because it meant that in order to know whether a particular game was split-pot, you had to know what the default was. As such, I changed it so that the default would be to add the parenthetical clarification.

My understanding is that "Big O", is always played high/low split with an eight qualifier and that if the game is played high-only, it's not "Big O (High Only)", but "Five Card Omaha (High Only)". So, before I introduced the bug that caused an incorrect parenthetical clarification from being posted, Big O was correctly labeled as "Big O (High/Low Eight Qualifier)" because it was always high/low split with an eight qualifier. There was special case code that did this:

-        let high_low = match self.high_low {
-            None => match self.sub_type {
-                Binglaha | Dramadugi | Dramaha | Dramaha49 | Dramaha3D | OmahaXOrBetter => "",
-                Omaha | FiveCardOmaha => " (High only)",
-                BigO | TwoOrFive => " (High/Low Eight Qualifier)",
-            },

The leading - in the above code fragment is from the git diff that shows what I changed. It denotes code that was removed. Specifically, it was removed from the description method of the Omaha type in the file src/game/omaha.rs. The reason it was removed is that over time, mb2 had developed different ways of assigning names to games, and I chose to unify them (ironically, to reduce the chance of weird bugs where things are misnamed).

Here's the code that contains the bug that bit us, followed by a small explanation of how it crept in:

+        let implicit_high_only = self.high_low.high_only()
+            && matches!(
+                self.sub_type,
+                Binglaha | Dramadugi | Dramaha | Dramaha49 | Dramaha3D | OmahaXOrBetter
+            );
+
+        write!(
+            f,
+            "{short_deck}{double_board}{bomb_pot}{win_the_button}{}",
+            self.sub_type
+        )?;
+        if !implicit_high_only {
+            write!(f, " {}", self.high_low)
+        } else {
+            Ok(())
+        }

As you can see above, the "match arm" for BigO or TwoOrFive that is present in the first block of code has been (incorrectly!) removed in the second block of code, but so are the text strings "(High Only)" and "(High/Low Eight Qualifier)". Those clarification strings were appearing in more than one place, so part of my unification was to make an actual HighLow type that could add the parenthetical clarification. Then, if I were later to choose a slightly different format, e.g. "(High/Low-8)", I would just change it in the way that HighLow displays itself, rather than have to change it in the way games themselves described themselves.

While making that change, I overlooked the fact that BigO was implicitly high/low. In fact, I overlooked the fact that any game was implicitly high/low (#1413); I only accounted for implicitly high only games, sub-consciously assuming that high/low split games would be identified as such, even though a closer reading of the code that I deleted would have alerted me to the fact that at one level, mb2 incorrectly considered Big O to be high-only.

From a Rust POV, this bug crept in when I moved from Option<bool> as a tri-state where None denoted high-only, Some(false) denoted high/low without a qualifier and Some(true) representing high/low with a qualifier to HighLow, an enum whose variants are HighOnly, HighUnqualifiedLow and HighQualifiedLow. That was a good change. However, I replaced a match which requires exhaustiver arms with a match! which doesn't and in doing so, I overlooked BigO and TwoOrFive.