# I'd like feedback on my first working script. Where can I improve in the future?

This code is for a spreadsheet that tracks weekly picks for NFL games. Four friends and I pick winners for every NFL game each week. Suppose there are 18 games being played this week; for each game, we all pick a winner, and also a level of confidence in that winner. We rank each game 1 to 18 based on our confidence-- so if a really good team plays a really bad team, we'll pick the good team for 18; if two teams are evenly matched, we'll pick a team for 1. (Each rank may only be used once each week per player.) If our team wins, we get the number of points we put on the game. If our team loses, we get no points.

The spreadsheet displays all of the games each week, and we enter our rankings under our column in the row of the corresponding team (see spreadsheet), and on the same side as the team we want to win. Then when a game is over, we just have to enter "w" or "l" in the appropriate column, and all player's scores are automatically calculated. The code also attempts to inform us as soon as a player is eliminated from contention for top score of the week.

Here is the currently implemented method of setting an elimination status: 1) Start by taking the current scores into account. 2) Consider each player vs each other player (aka opponent) one at a time 3) If there is ever a time when a player cannot beat one or more of his opponents, then he is eliminated.

To check whether he can beat an opponent is tricky, though-- because the best case scenario for a player is not necessarily getting all of his picks right. Suppose they both pick the same team, but the opponent ranks that game higher-- it's actually better for the player (at least against this opponent) for the team he picked to lose.

So instead, I calculate a best possible scenario score (again, for each player against each individual opponent). It starts with each player's current score. Then for all unplayed games, if the player and the opponent picked opposite sides, then the player's pick is added to his score (and the opponent gets no points in this scenario.) If they picked the same team, then we have to check who ranked the game higher; if the player ranked the game higher, then we add his points to his score, AND we add the points the opponent put on the game to the opponent's score. If the opponent picked higher, then nothing happens, since the best outcome is that that team loses and no one gets any points.

At the end of all of that, if the best possible player score is smaller than the opponent's score in this scenario, then that player is eliminated.

It does a few other things as well-- there's a comment section of the spreadsheet that detects the user who entered the comment, colors it appropriately, then shifts all the comments up one cell.

Following is the code, and here is a link to the spreadsheet into which we enter the information.

function onEdit(event){
var sheet = ss.getSheets()[0];
var sh = event.source.getActiveSheet();
var r = event.source.getActiveRange();
var user = Session.getUser();
var lastCellModified = r.getA1Notation();
//only update the elimiantions/must win/lose scenarios if the w/l column is updated:
if (lastCellModified == "A10" || lastCellModified == "A11" || lastCellModified == "A12" || lastCellModified == "A13"
|| lastCellModified == "A14" || lastCellModified == "A15" || lastCellModified == "A16" || lastCellModified == "A17"
|| lastCellModified == "A18" || lastCellModified == "A19" || lastCellModified == "A20" || lastCellModified == "A21"
|| lastCellModified == "A22" || lastCellModified == "A23" || lastCellModified == "A24" || lastCellModified == "A25"){
ss.getRange("D2:D7").clearContent();
//clear "elimination" status, so it doesn't have to be done manually if a wrong game outcome is entered.
var sheetArray = [];
sheetArray = ss.getRange("A1:N25").getValues()
//A1: [0][0]
//A2: [1][0]
//B1: [0][1]
// IN OTHER WORDS: [Y][X] AND REMEMBER Y IS THE NUMBER, X IS THE LETTER
//BECUASE I AM DUMB HERE IS A HANDY ALPHANUMERIC CONVERSION CHART:
//A B C D E F G H I J K  L  M  N  O  P  Q  R  S  T  U  V  W  X  Y  Z
//0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25

//ALSO BECAUSE IT IS FUNNY TO ME HERE IS A HANDY NUMERONUMERIC CONVERSION CHART:
//COLUMN DESIRED       : 1  2  3  4 5 6 7 8 9 10 11  12  13  14  15  16  17  18  19  20  21  22  23  24  25  26   27
//ARRAY NUMBER TO ENTER:[0][1][2][3]4 5 6 7 8 9  10  11  12  13  14  15  16  17  18  19  20 [21][22][23][24][25][26]

//player values: Steve=1, Chris=2, Tyler=3, Billy=4, Drew=5
//go through each player one at a time
for (var player=1; player<6; player++){
if(player==1){var pScore=sheetArray[1][1];var leftPCol=2; var rightPCol=11; var elimRow=1}
if(player==2){var pScore=sheetArray[2][1];var leftPCol=3; var rightPCol=10; var elimRow=2}
if(player==3){var pScore=sheetArray[3][1];var leftPCol=4; var rightPCol=9; var elimRow=3}
if(player==4){var pScore=sheetArray[4][1];var leftPCol=5; var rightPCol=8; var elimRow=4}
if(player==5){var pScore=sheetArray[5][1];var leftPCol=1; var rightPCol=12; var elimRow=5}
//Now we take select opponents one at a time for the current player
for(var opponent=1; opponent<6; opponent++){
if(player==opponent){break}
if(opponent==1){var oScore=sheetArray[1][1]; var leftOCol=2; var rightOCol=11}
if(opponent==2){var oScore=sheetArray[2][1]; var leftOCol=3; var rightOCol=10}
if(opponent==3){var oScore=sheetArray[3][1]; var leftOCol=4; var rightOCol=9}
if(opponent==4){var oScore=sheetArray[4][1]; var leftOCol=5; var rightOCol=8}
if(opponent==5){var oScore=sheetArray[5][1]; var leftOCol=1; var rightOCol=12}

//Set best possible scenario score initially to the player's current score, and opponent's to opponent's current score
var bPScore=pScore;
//toastMessage = toastMessage + "bPScore for player "+player+" is "+bPScore+", ";
var bOScore=oScore;
//toastMessage = toastMessage + "bOScore for opponent "+opponent+" is "+bOScore+", "

for(var row=9; row<24; row++){ //Iterate through game rows.
if(sheetArray[row][0]==""){ //But only if the game has not been played yet

//determine which team player and opponent picked and if they picked the same team. If their left column ISN'T empty, they picked the team on the left;
//if it IS empty, they picked the team on the right.
if(sheetArray[row][leftPCol]!=""){var pPick="left"} else{var pPick="right"}
if(sheetArray[row][leftOCol]!=""){var oPick="left"} else{var oPick="right"}
if(pPick==oPick){var sameTeam=true} else {var sameTeam=false}

//get values of each pick
if(pPick=="left"){var pV=sheetArray[row][leftPCol]}
if(pPick=="right"){var pV=sheetArray[row][rightPCol]}
if(oPick=="left"){var oV=sheetArray[row][leftOCol]}
if(oPick=="right"){var oV=sheetArray[row][rightOCol]}
//if both players picked the same team, add the picks to the scenario score if player picked higher than opponent
if(sameTeam==true){
if(pV>oV){bPScore=bPScore+pV;bOScore=bOScore+oV}//if player picked for more points, add each players' picks to their score
}//if opponent picked for more points, don't do anything.

//if both players picked opposite teams, add player's pick value to his total score
if(sameTeam==false){bPScore=bPScore+pV} //don't change opponent's bp scenario score
}//end of cycling through opponent
}
//After checking each unplayed game by the current player against a given opponent and assigning points based on a best case scenario, check if player is eliminated
if (bPScore<bOScore){sheetArray[elimRow][3]="ELIMINATED"};
}
}
// sheet.getRange("O26").setValue(toastMessage);
//var elimArray = [[sheetArray[1][3]],[sheetArray[2][3]],[sheetArray[3][3]],[sheetArray[4][3]],[sheetArray[5][3]]];
//sheet.getRange("A26").setValue(sheetArray); //for debugging
sheet.getRange("D2").setValue(sheetArray[1][3]);
sheet.getRange("D3").setValue(sheetArray[2][3]);
sheet.getRange("D4").setValue(sheetArray[3][3]);
sheet.getRange("D5").setValue(sheetArray[4][3]);
sheet.getRange("D6").setValue(sheetArray[5][3]);
//end of ELIMINATIONS part of script
}

//This is for the moving comment box. It formats a cell with the colors of the commenter's favorite team,
//and then moves all the comments up, just like in a chat room, leaving the bottom cell open.
if (lastCellModified == "G8"){
var user = Session.getEffectiveUser().getEmail()
switch (user){
case "fakeemail1@gmail.com":r.setBackgroundColor("MidnightBlue").setFontColor("LimeGreen");break;
case "fakeemail2@gmail.com":r.setBackgroundColor("MidnightBlue").setFontColor("Orange");break;
case "fakeemail3t@gmail.com":r.setBackgroundColor("Gray").setFontColor("DarkBlue");break;
case "fakeemail4@gmail.com":r.setBackgroundColor("Orange").setFontColor("DarkBlue");break;
case "fakeemail5@gmail.com":r.setBackgroundColor("Gray").setFontColor("DarkBlue");break;
case "fakeemail6@gmail.com":r.setBackgroundColor("Green").setFontColor("Yellow");break;
}
ss.getRange("G2").copyFormatToRange(sheet, 7, 15, 1, 1);//sheet, column, columnEnd, row, rowEnd
ss.getRange("G2").copyValuesToRange(sheet, 7, 15, 1, 1);
ss.getRange("G3").copyFormatToRange(sheet, 7, 15, 2, 2);
ss.getRange("G3").copyValuesToRange(sheet, 7, 15, 2, 2);
ss.getRange("G4").copyFormatToRange(sheet, 7, 15, 3, 3);
ss.getRange("G4").copyValuesToRange(sheet, 7, 15, 3, 3);
ss.getRange("G5").copyFormatToRange(sheet, 7, 15, 4, 4);
ss.getRange("G5").copyValuesToRange(sheet, 7, 15, 4, 4);
ss.getRange("G6").copyFormatToRange(sheet, 7, 15, 5, 5);
ss.getRange("G6").copyValuesToRange(sheet, 7, 15, 5, 5);
ss.getRange("G7").copyFormatToRange(sheet, 7, 15, 6, 6);
ss.getRange("G7").copyValuesToRange(sheet, 7, 15, 6, 6);
ss.getRange("G8").copyFormatToRange(sheet, 7, 15, 7, 7);
ss.getRange("G8").copyValuesToRange(sheet, 7, 15, 7, 7);
ss.getRange("G8").clearContent().clearFormat;

}

}


SO, that's my request. Some specific things I'm interested in: 1) Am I doing things efficiently? I don't care how fast the code runs (within reason) but I do want to know if I'm wasting a lot of my own time typing needless repetitions of code.

2) It would really have helped me to take some of these little sections out of the main function, creating new functions and inserting calls to them. That would have helped me keep parentheses straight, and likely would have sped up the process of bug-fixing quite a bit. However, I kept having problems when I tried.

One more thing:

You may have noticed that there is a problem with the way I am calculating eliminations-- it assumes that if a player could beat all other players in a one-on-one contest, given that everything goes exactly right, then the player is still alive. In fact, though, it is often impossible for a player to have something happen that is the best possible outcome against multiple opponents at once.

(Example: Player has 5 points, Opponent1 has 3 points, Opponent2 has 5 points. One game remains, A vs B. Player picks A for 1 point. Opponent1 picks A for 4 points; Opponent2 picks B for 1 point. Individually, Player can beat Opponent1 if A loses, since he will finish with 5 and Opponent1 will finish with 3. And he can beat Opponent2 if A wins, because he will finish with 6 and Opponent2 will finish with 5. However, if A loses, then Opponent2 will actually get 1 point and win; if A wins, Opponent1 will get 4 points while Player will only get 1 point, finishing with 6 vs Opponent1's 7 points. So in reality, Player is already eliminated.)

So that's the problem I'm working on right now. And I wanted to kill 2 birds with 1 stone, so while checking for these scenarios where a player can't take on both opponents at once, I figured it would also be fun to highlight (in green) all games that a player MUST WIN to remain alive, as well as games that he MUST LOSE to remain alive (red), and of course games that he has to simultaneously win and lose, which cause him to be eliminated (highlighted in black).

Unfortunately, there's only one week left in the season, and I'm not on pace to finish :( So if you want to look over that code as well, you're more than welcome-- it's here on this spreadsheet, you just need to go to "tools->script editor."

Thank you very much for your time!

-
Are you sure this code works correctly? Looking at your player/opponent comparison loops, the if(player==opponent){break} line means that (e.g.) "player" 3 will be compared to "opponent" 1 and 2, but player 1 will never be compared to any opponent, and cannot be eliminated, because if (bPScore<bOScore){sheetArray[elimRow][3]="ELIMINATED"}; will only eliminate the "player" in any cycle of the loop. –  Stuart Dec 22 '12 at 3:32
Actually, now that you mention it, Stuart... It used to work perfectly, but in my effort to cut down on parentheses, I recently changed that part. It used to say "if player != opponent {" and then a close bracket way down at the end. And I recently did experience a problem with it breaking and not calculating eliminations properly for some players against some opponents, but it seemed like the problem cleared up again after that. That may explain part of why the new function isn't working, too. Thanks for your comment. –  Dr.Queso Dec 22 '12 at 15:12

Few general notes on the code

1. The method onEdit is doing too many things which is making the code difficult to read. I would prefer to break the entire function and distribute responsibilities. The structure would look something like this (this is just an example) -

var Game = {};

Game.setStatus = {
validateInput: function () {
},
isLastCallModified: function() {
},
update: function() {
},
moveBox: function() {
}
};

2. you have check for boolean values in this way if (sameTeam == true) and if (sameTeam == false) which is not the right way. It should be if (sameTeam) and if (!sameTeam)

-

You can get rid of repetition in several places.

The long if statement could be replaced with this:

if (lastCellModified.indexOf(['A10', 'A11', 'A12', 'A13', 'A14', 'A15', 'A16',
'A17', 'A18', 'A19', 'A20', 'A21', 'A22', 'A23', 'A25']) > -1) {


or this:

if (lastCellModified.length == 3 && lastCellModified >= "A10" && lastCellModified <= "A25")


Setting variables for players and opponents can be reduced to:

var pScore = sheetArray[player][1];
var leftPCol = player % 5 + 1;
var rightPCol = 13 - leftPCol;
var pElimRow = player;


The list of sheet.getRange('D2') etc. could be replaced with a loop:

for (var i = 1; i < 6; i++) {
sheet.getRange("D" + (i + 1)).setValue(sheetArray[i][3]);
}


and similarly for the ss.getRange... list:

for (var i = 1; i < 8; i++) {
var range = ss.getRange('G' + (i + 1));
range.copyFormatToRange(sheet, 7, 15, i, 1);
range.copyValuesToRange(sheet, 7, 15, i, 1);
}


See the full code with a few other changes here

EDIT: I looked at this in a bit more detail. Ultimately what I think you need is to go through every possible combination of outcomes for the remaining games, and check who is the winner under each scenario, and eliminate the players who are not winners under any scenarios. (There might be some shortcut method, but as you have already noted, your solution of comparing pairs of players does not quite do it.) This does involve looking at 2 to the power of N scenarios where N is the number of remaining games. That should be manageable for, say, 16 games, although it may get a little slow. I've altered the script to do this here but couldn't figure out how to test it.

Whether you use this or not, I recommend using the first part of that code, which reads the players' current scores, their picks, and their bets, into arrays with meaningful names at the beginning. This simplifies the rest of the code a lot as you can deal with those arrays instead of having to keep thinking about what reference you need in the spreadsheet.

-
Wow, it looks like there is a lot of great stuff in there that I can learn from. It'll probably take me a couple weeks to completely understand some of the things you've done, but believe me, I will keep at it. In addition, I was really surprised by how you calculated eliminations. I had tried asking around previously but didn't get an answer-- the idea of calculating all possible remaining scenarios was appealing to me, of course, but it seemed like it would be really slow. I know very little of computer science, but I'm excited to know that this is doable. –  Dr.Queso Dec 22 '12 at 15:20
Usually javascript is slow when inputting/outputting (e.g. to a webpage or in this case a spreadsheet) but fast when just comparing numbers in arrays. Going through a few hundred thousand values should take less than a second generally. But obviously the number of possible outcomes increases exponentially with the number of games. Anyway try to test it. –  Stuart Dec 22 '12 at 15:46
I have to run to work, but after a brief test, I do come across a few problems. However, I lost connection to the Google Server at one point (even while other sites were working), and maybe the problems are not in the code but somewhere else. Anyway, after trying the updated code, the commenting section stopped working, and the eliminations updated regardless of what cell I edited. Eliminations worked sporadically, which I am chalking up to the connection issue. I'll re-try it tonight, assuming I get off work at a reasonable hour. Thanks again for your help. –  Dr.Queso Dec 22 '12 at 15:56
there was a bug - the if statement towards the top should read if (r.getColumn() == 1 && r.getRow() >= 10 && r.getRow() <= 25) (remove the !) –  Stuart Dec 22 '12 at 16:59
Made another fix: towards the end range.copyFormatToRange(sheet, 7, 15, i, 1); should be range.copyFormatToRange(sheet, 7, 15, i, i); –  Dr.Queso Dec 23 '12 at 7:12

var ss = ... ;
var sheet = ... ;
var sh = ... ;
var r = ... ;
var user = ... ;
var lastCellModified = ... ;


with

var ss = ... ,
sheet = ... ,
sh = ... ,
r = ... ,
user = ... ,
lastCellModified = ... ,

-