Refactor Riichi point calculations #22

TwelveNights · 2016-10-06T19:14:27Z

Resolution for #16 and #8

0spooky

Please split the constant definitions and the actual work in two seperate branches. Also do not adapt the code to your own style choices. All you are doing is making the code messier by having multiple styles existing at once in the same code base.

0spooky · 2016-10-07T06:20:06Z

imports/api/Constants.js

+	NORTH: "north",
+
+	MAHJONG_CLUB_LEAGUE: "Mahjong Club League",
+	MANGAN: 2000


You have good intentions here, but "MANGAN = 2000" is meaningless. You may as well have called the constant "Y2K" or "Andre minus 1000". At least call this something like MANGAN_BASE_POINTS or something.

0spooky · 2016-10-07T06:28:48Z

imports/api/NewGameUtils.js

-			else //if (round > 8)
-				return "西";;
-			break;
+			case Constants.GAME_TYPE.HONG_KONG:


This is not the style I use for switch/case. Case stays indented the same as switch.

I'll update my code to have the case be aligned with switch.

0spooky · 2016-10-07T06:29:58Z

imports/api/NewGameUtils.js

-				return "西";
-			else //if (round > 12)
-				return "北";
-			break;


Why are you so keen on putting breaks in every case? I prefer to not include breaks in switch cases which are returned from. Looks cleaner and the break is never called anyway.

It wasn't me that had them to begin with lmao

0spooky · 2016-10-07T06:31:02Z

imports/api/NewGameUtils.js

 		};
 	},

 	// Helper function to ensure all players are selected
 	allPlayersSelected() {
-		return (Session.get("current_east") != Constants.DEFAULT_EAST && 


This is just the style I've been taught, but can formatting please be put in a seperate branch if you think it's important? Otherwise there's just a lot of clutter.

Most of this pull request seems unrelated to point calculation. Either rename the commit or change this to reflect what the commit is.

Most of the code changes I wanted to make in this ticket are relating to refactoring, hence the refactor name of the commit. Actually, no behaviour should be changed from this ticket.

Have you done extensive testing of point submission? This is the most critically important part of the submission form so every possible input must work exactly correct or the entire system is broken.

0spooky · 2016-10-07T06:34:50Z

imports/api/PointCalculationUtils.js

+import { Constants } from './Constants';
+import { NewGameUtils } from './NewGameUtils';
+
+export var PointCalculationUtils = {


If you're going to do this, make it generic (include HK calculation too it's much more simple anyway)

I'd like to do it later, since this ticket is already quite large.

I'm just worried about JPN and HKG code diverging too much when it should be converging.

0spooky · 2016-10-07T06:36:12Z

imports/ui/JapaneseNewGame.js

@@ -601,7 +601,7 @@ function save_game_to_database(hands_array) {
 	};

 	// Initialise ELO calculator to update player ELO
-	var jpn_elo_calculator = new EloCalculator(2000, 5, [15000, 0, -5000, -10000], game, Constants.GAME_TYPE.JAPANESE);
+	var jpn_elo_calculator =  new EloCalculator(2000, 5, [15000, 0, -5000, -10000], game, Constants.GAME_TYPE.JAPANESE);


Remove this space

0spooky · 2016-10-07T06:37:59Z

imports/api/PointCalculationUtils.js

+    let winds = {};
+    winds[Constants.EAST] = winds[Constants.SOUTH] = winds[Constants.WEST] = winds[Constants.NORTH] = 0;
+
+	let basicPoints;


Some funky indentation here

I'll fix that in my next commit.

0spooky · 2016-10-07T06:38:22Z

imports/api/PointCalculationUtils.js

+    let manganPayout = multiplier * Constants.MANGAN;
+
+	if (points < 5) {
+        if (fu == 20 || (points == 1 && fu == 25)) {


Super funky indentation. Use tabs for indentation and spaces for alignment please.

TwelveNights · 2016-10-07T07:20:12Z

imports/ui/JapaneseNewGame.js

-	}
-	else if (winnerWind == "south") {
+		}
+	} else if (winnerWind == Constants.SOUTH) {


Force of habit, although keep in mind that all this change does is modify it so that the oneline if statement is encapsulated in curly braces instead of following in a logical line.

Just an encompasing statement I happened to put on this line. I actually generally agree with wrapping single line conditional logic.

0spooky · 2016-10-26T17:57:56Z

Some responses posted. Please refactor as well as there are now merge conflicts.

TwelveNights · 2016-11-21T06:41:33Z

imports/api/PointCalculationUtils.js

+				winds[w] = (nonDealerPays * 3) + (currentBonus * 300) + (riichiSticks * 1000);
+			} else {
+				winds[w] = -(nonDealerPays + (currentBonus * 100));
+		}


Gotta correct this as an aside, lol

0spooky

Review to continue once these changes are made.

0spooky · 2016-11-21T06:30:59Z

imports/api/Constants.js

 };

+Constants.WINDS = [Constants.EAST, Constants.SOUTH, Constants.WEST, Constants.NORTH];


Why is this defined outside the Constants object?

Those values don't exist until the Constants object is defined. I added this line afterwards so I didn't have to be redundant.

0spooky · 2016-11-21T06:32:40Z

imports/api/NewGameUtils.js

-		        Session.get("current_south") != Constants.DEFAULT_SOUTH &&
-		        Session.get("current_west") != Constants.DEFAULT_WEST &&
-		        Session.get("current_north") != Constants.DEFAULT_NORTH);
+				Session.get("current_south") != Constants.DEFAULT_SOUTH &&


Align these blocks with spaces. For instance, this line should be two tabs followed by 8 spaces in order for the Session(...) code to align properly on all environments.

Did not address.

0spooky · 2016-11-21T06:33:12Z

imports/api/NewGameUtils.js

@@ -129,13 +123,13 @@ export var NewGameUtils = {
 		let westRoundOver = Session.get("current_round") > 12;
 		// End condition where game has reached the end of south round with at least one player above minimum
 		let someoneAboveMinimum = Session.get("current_round") > 8 &&
-		                          this.someoneAboveMinimum(Constants.JPN_END_POINTS);
+								  this.someoneAboveMinimum(Constants.JPN_END_POINTS);


Same as above (RE: Spaces)

Did not address.

0spooky · 2016-11-21T06:33:29Z

imports/api/NewGameUtils.js

-		                                 Session.get("current_bonus") > 0 &&
-		                                 this.getDirectionScore("north") >= Constants.JPN_END_POINTS &&
-		                                 this.getFirstPlace() == "north";
+										 Session.get("current_bonus") > 0 &&


Same as above (RE: Spaces)

0spooky · 2016-11-21T06:40:25Z

imports/api/PointCalculationUtils.js

+
+export var PointCalculationUtils = {
+	jpn: {
+		dealin_delta,


Perhaps these should be renamed jpn_dealin_delta to more easily accomodate later addition of hkg

0spooky · 2016-11-21T06:42:44Z

imports/api/PointCalculationUtils.js

+	}
+};
+
+function dealin_delta(points, fu, winnerWind, loserWind, riichiSticks) {


Would it hurt to put comments in your code additions? It makes it faster for others to review, and easier for others to maintain. I think we should probably start including function headers as well just to make things easier to understand.

0spooky · 2016-11-21T06:46:47Z

imports/api/PointCalculationUtils.js

+	} else { basicPoints = manganValue(points); }
+
+	let nonDealerPays = Math.ceil(basicPoints / 100 * (dealerWind == winnerWind ? 2 : 1)) * 100;
+	let dealerPays = Math.ceil(basicPoints / 100 * 2) * 100;


Should declare used variables at the beginning of the function.

0spooky · 2016-11-21T06:49:52Z

imports/api/PointCalculationUtils.js

+		}
+	} else { basicPoints = manganValue(points) * multiplier };
+
+	let currentBonus = Number(Session.get("current_bonus")) * 300;


Bonus amount should probably be defined in Constants.js

0spooky · 2016-11-21T06:50:08Z

imports/api/PointCalculationUtils.js

+	} else { basicPoints = manganValue(points) * multiplier };
+
+	let currentBonus = Number(Session.get("current_bonus")) * 300;
+	winds[winnerWind] = basicPoints + currentBonus + (riichiSticks * 1000);


So should riichi amount

0spooky · 2016-11-21T06:50:45Z

imports/api/PointCalculationUtils.js

+
+function mistake_delta(loser) {
+	let winds = {};
+	winds[Constants.EAST] = winds[Constants.SOUTH] = winds[Constants.WEST] = winds[Constants.NORTH] = 4000;


Mistake points should probably be defined in Constants.js

0spooky · 2016-11-21T07:23:43Z

imports/api/Constants.js

+	RIICHI_POINTS: 1000,
+
+	// Points paid for a mistake
+	MISTAKE_POINTS: 12000,


Please specify this is the Japanese chombo amount. The Hong Kong amount is different.

Might as well give the JPN_ prefix to bonus and Riichi too, since that is sort of the convention it seems.

0spooky · 2016-11-21T07:25:23Z

imports/api/NewGameUtils.js

-		        Session.get("current_south") != Constants.DEFAULT_SOUTH &&
-		        Session.get("current_west") != Constants.DEFAULT_WEST &&
-		        Session.get("current_north") != Constants.DEFAULT_NORTH);
+				Session.get("current_south") != Constants.DEFAULT_SOUTH &&


Did not address.

0spooky · 2016-11-21T07:25:32Z

imports/api/NewGameUtils.js

@@ -129,13 +123,13 @@ export var NewGameUtils = {
 		let westRoundOver = Session.get("current_round") > 12;
 		// End condition where game has reached the end of south round with at least one player above minimum
 		let someoneAboveMinimum = Session.get("current_round") > 8 &&
-		                          this.someoneAboveMinimum(Constants.JPN_END_POINTS);
+								  this.someoneAboveMinimum(Constants.JPN_END_POINTS);


Did not address.

0spooky · 2016-11-21T07:26:40Z

imports/api/NewGameUtils.js

@@ -129,13 +123,12 @@ export var NewGameUtils = {
 		let westRoundOver = Session.get("current_round") > 12;
 		// End condition where game has reached the end of south round with at least one player above minimum
 		let someoneAboveMinimum = Session.get("current_round") > 8 &&
-		                          this.someoneAboveMinimum(Constants.JPN_END_POINTS);
+		                this.someoneAboveMinimum(Constants.JPN_END_POINTS);


So close, but this doesn't actually line up with the above line.

0spooky · 2016-11-21T07:26:50Z

imports/api/NewGameUtils.js

-		                                 this.getDirectionScore("north") >= Constants.JPN_END_POINTS &&
-		                                 this.getFirstPlace() == "north";
-
+		                Session.get("current_bonus") > 0 &&


0spooky · 2016-11-21T07:28:44Z

imports/api/PointCalculationUtils.js

+			basicPoints = Math.ceil((fu * Math.pow(2, 2 + points)) * multiplier / 100) * 100;
+			basicPoints = basicPoints < manganPayout ? basicPoints : manganPayout;
+		}
+	} else { basicPoints = manganValue(points) * multiplier };


If you're going to do this, the semicolon should be at the end of the line, not the end of the block. I would still make this and the other one like it a seperate line.

0spooky · 2016-11-21T07:31:09Z

imports/api/PointCalculationUtils.js

+ * 
+ * @return {Object} containing the point difference for each seat
+ */
+function jpn_dealin_delta(points, fu, winnerWind, loserWind, riichiSticks) {


If we're adding function headers, make sure to add @param entries

0spooky · 2016-11-21T07:34:17Z

imports/api/PointCalculationUtils.js

+export var PointCalculationUtils = {
+	jpn_dealin_delta,
+	jpn_selfdraw_delta,
+	jpn_mistake_delta


Hmm, is it possible to keep the jpn object in PointCalculationUtils with members "dealin_delta = jpn_dealin_delta"? It seems you removed it this entry in favour of just the suffix.

0spooky · 2016-11-21T07:36:30Z

imports/ui/JapaneseNewGame.js

-	northDelta += dealin_delta(points, fu, "north", winnerWind, loserWind) +
-				  rewardRiichiSticks(riichiSum, "north", winnerWind);
+	allDelta = PointCalculationUtils.jpn_dealin_delta(points, fu, winnerWind, loserWind, riichiSum);
+	Session.set("free_riichi_sticks", 0);


This is redundant as it exists in PointCalculationUtils (I would remove it from there personally)

This is located in a different location than in self_draw. Should be consistent.

0spooky · 2016-11-21T07:58:35Z

imports/api/PointCalculationUtils.js

+
+	// Check to see if you have to count basic points
+	if (points < 5) {
+		if (points == 1 && (fu == 20 || fu == 25)) {


You also need to protect 2 point 25 selfdraw as that combination does not exist.

That is prevented in NewGameUtils#noIllegalSelfDraw. This should be removed for issue #27.

So will this be removed or augmented with 2p25f? In between makes no sense

0spooky · 2016-11-21T08:03:12Z

imports/api/PointCalculationUtils.js

+ * Calculates the change in points as a result of a deal-in hand
+ * Uses the formula from https://en.wikipedia.org/wiki/Japanese_Mahjong_scoring_rules
+ *
+ * @param {number} points - the total number of han + dora


I think param variable types need to be capitalized.

0spooky · 2016-11-21T08:20:43Z

imports/ui/JapaneseNewGame.js

@@ -1089,36 +1091,37 @@ function push_split_pao_hand(template) {
 		Session.set("north_riichi_sum", Number(Session.get("north_riichi_sum")) + 1);
 	}

-	var value = dealin_delta(points, fu, winnerWind, winnerWind);
+	var value = PointCalculationUtils.jpn.dealin_delta(points, fu, winnerWind, loserWind, 0)[loserWind];


Is this correct? I'm worried this might not be fully tested.

It is wrong. I use the value of loserWind instead of winnerWind. Good catch.

As an aside, this one is weird where I have to pass 0 in the riichi value because of the unique splitting rules :/
The code previously would always return 0, too, because you compared winnerWind with winnerWind. The code for the original dealin_delta was:
if (playerWind != winnerWind && playerWind != loserWind) return 0;

0spooky

Don't forget to put in constants where we spoke about.

0spooky · 2017-02-21T22:01:36Z

imports/api/PointCalculationUtils.js

+
+	// Check to see if you have to count basic points
+	if (points < 5) {
+		if (points == 1 && (fu == 20 || fu == 25)) {


So will this be removed or augmented with 2p25f? In between makes no sense

0spooky · 2017-02-21T22:03:46Z

imports/api/Constants.js

@@ -43,7 +55,21 @@ export const Constants = {
 	SELF_DRAW: "selfdraw",
 	NO_WIN: "nowin",
 	RESTART: "restart",
-	MISTAKE: "fuckup"
+	MISTAKE: "fuckup",


This might conflict now

0spooky · 2017-02-21T22:41:48Z

imports/ui/JapaneseNewGame.js

@@ -790,7 +791,7 @@ function save_game_to_database(hands_array) {
 		if (Number(Session.get("west_score")) >= Number(Session.get("north_score"))) position--;
 		Players.update({_id: west_id}, {$inc: {japanesePositionSum: position}});

-		switch (position) {
+    switch (position) {


There's generally a lot of ruined indentation in this patch. Please fix. (Tabs for indentation, spaces for alignment)

0spooky · 2017-02-21T22:48:31Z

imports/ui/JapaneseNewGame.js

@@ -1188,36 +1195,37 @@ function push_split_pao_hand(template) {
 		Session.set("north_riichi_sum", Number(Session.get("north_riichi_sum")) + 1);
 	}

-	var value = dealin_delta(points, fu, winnerWind, winnerWind);
+	var value = PointCalculationUtils.jpn.dealin_delta(points, fu, winnerWind, loserWind, 0)[winnerWind];


It's worth creating another PointCalculationUtils function for Pao situations.

0spooky · 2017-10-15T16:11:38Z

Can you rebase so I can review this again?

0spooky2me · 2018-09-26T08:44:59Z

Now a duplicate of #64

0spooky requested changes Oct 7, 2016

View reviewed changes

TwelveNights commented Oct 7, 2016

View reviewed changes

TwelveNights force-pushed the refactor-point-calculations branch from ee0dc55 to 22eb957 Compare October 7, 2016 08:04

TwelveNights force-pushed the refactor-point-calculations branch from 22eb957 to 10006f6 Compare November 10, 2016 00:40

TwelveNights force-pushed the refactor-point-calculations branch from 10006f6 to c03cf1b Compare November 20, 2016 22:40

TwelveNights added 5 commits November 20, 2016 22:15

Refactor Riichi point calculations

5fd8666

Match coding style

5c07cfb

Convert tabs to spaces

b4fd46d

Correct and test point values

25695b8

Remove trailing whitespaces and convert tabs to space

75df9b0

TwelveNights force-pushed the refactor-point-calculations branch 2 times, most recently from 87844b5 to 093651b Compare November 21, 2016 06:28

Reconvert spaces to tabs

7f00868

TwelveNights force-pushed the refactor-point-calculations branch from 093651b to 7f00868 Compare November 21, 2016 06:29

TwelveNights commented Nov 21, 2016

View reviewed changes

0spooky requested changes Nov 21, 2016

View reviewed changes

TwelveNights added 2 commits November 20, 2016 23:12

Update code based on review

5aff7f1

Rename functions

149c1b1

0spooky requested changes Nov 21, 2016

View reviewed changes

Submit one more set of changes based off of review

c2a13da

0spooky requested changes Nov 21, 2016

View reviewed changes

TwelveNights added 2 commits November 23, 2016 10:31

Capitalize number and modify pao points to use correct wind

7f254dd

Merge branch 'master' into refactor-point-calculations

4ffe150

0spooky requested changes Feb 21, 2017

View reviewed changes

0spooky mentioned this pull request Oct 10, 2017

Add additional constants for directions and Japanese points #51

Merged

0spooky2me closed this Sep 26, 2018

		};

		Constants.WINDS = [Constants.EAST, Constants.SOUTH, Constants.WEST, Constants.NORTH];

Refactor Riichi point calculations #22

Refactor Riichi point calculations #22

Conversation

TwelveNights commented Oct 6, 2016

0spooky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TwelveNights Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0spooky commented Oct 26, 2016

Choose a reason for hiding this comment

0spooky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0spooky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0spooky commented Oct 15, 2017

0spooky2me commented Sep 26, 2018

TwelveNights Oct 7, 2016 •

edited

Loading