48

Impressive. I mean it. Couple observations: Your enums... public enum SudokuProgress { FAILED, NO_PROGRESS, PROGRESS } Should be: public enum SudokuProgress { Failed, NoProgress, Progress } When the first thing you see is this: public class SudokuBoard { public SudokuBoard(SudokuBoard copy) { _maxValue = copy._maxValue; tiles = ...


44

Let's look at your code from an extensibility point of view: If Sheldon decides to add a new item to the game then you have to go to n classes to adjust the comparisons and winning verbs. I usually try to avoid such designs because whenever you require a developer to change stuff in n places when something new is added then he/she is bound to forget one ...


24

A few comments: [ ... ] for (; (x % 3) != 0; x--); // quickly set x to left bound of sub-board for (; (y % 3) != 0; y--); // quickly set y to upper bound of sub-board I think I'd move the code to round to a multiple of three into a function of its own. I think I'd implement that something like this: int round3(int in) { return (in/3)*3; } [ ... ] int ...


23

Can't read all this code on my phone even though it looks pretty well structured to me! Good job! I saw this. Isn't the exception message contradicting the if clause? if (value < CLEARED) throw new ArgumentOutOfRangeException("SudokuTile Value cannot be zero or smaller. Was " + value); CLEARED is set to 0, and the if checks for 'less than 0' so ...


23

Being biased toward Groovy, I say do more Groovy stuff :) There are a number of things you can do to make your code more Grooooooovy. def is your friend The def keyword makes it a cinch to declare variables and it makes your declarations easier on the eyes. // Eeewwww String s1 = 'hello' double d1 = Math.floor(10.34) ArrayList<Integer> l1 = new ...


22

To review the code as it is: It would be better not to even have result as a variable, and swap the order of operations inside the else-ifs that assign result from (result = X, gameResult++) to (gameResult++, return X) You don't need semi-colons on the end of if statements: } else { return false; }; Rather than individually testing (userInput == X, ...


20

This is amazing. Especially for person who is not using C# every day for years. My main concerns are too many thing that are public that should be internal, sometimes internal members can be turned to private. Use the most restrictive access level that makes sense for a particular member. error-prone method AddRow of SudokuBoard. I'd prefer single string ...


19

I know next to zilch about C# so I won't be much help with this review, but I can say that it looks well thought out and implements some cool features. And like retailcoder I'm still working on my version (hoping to minimize the brute-force portion) using Ruby. If C# allows enums to implement methods, I would move CombineSolvedState into SudokuProgress. ...


19

thisButton.attr('data-x') You can use jQuery's .data(). It uses an internal cache for storing key-value pairs, but if it doesn't find it there, it looks up the data-* attributes of the associated element. This means you can do thisButton.data('x'). Note that dash-space becomes camelCase. Number(thisButton.attr('data-x')) If you use jQuery's .data(), it ...


18

Some minor syntax detail: C# has collection initializers so one can write: List<string> listOfGestures = new List<string> { "rock", "paper", "scissors", "lizard", "spock" } You use magic strings for the gestures all over the place. This can pose a problem if you want to change them. You could do a find and replace but this could ...


18

Well, first questions is: why don't you use MVVM? Contents of your App class look like something that should be implemented on model/view model level. This looks like a lot of copy-pasting private void Blue_MouseDown(object sender, MouseButtonEventArgs e) { OnSimonButtonClicked(SimonButton.Blue); e.Handled = true; } private void Yellow_MouseDown(...


17

You can write your Box method without foreach loops or yield return: public static IEnumerable<Tuple<int, int>> Box(int sizeX, int sizeY) { return from x in Enumerable.Range(0, sizeX) from y in Enumerable.Range(0, sizeY) select Tuple.Create(x,y); } or equivalently: public static IEnumerable<Tuple<int, int>&...


17

There's also a generalization problem. I agree! Whenever you start writing things like: f0 = Floor(0) f1 = Floor(1) f2 = Floor(2) f3 = Floor(3) f4 = Floor(4) f5 = Floor(5) f6 = Floor(6) f7 = Floor(7) f8 = Floor(8) stop immediately, and consider using lists and/or dictionaries instead: floors = [Floor(i) for i in range(9)] This now makes subsequent ...


15

this sounded like IComparable<T> But it's not. The documentation of Compare() states that the relation has to be transitive: If A.CompareTo(B) returns a value x that is not equal to zero, and B.CompareTo(C) returns a value y of the same sign as x, then A.CompareTo(C) is required to return a value of the same sign as x and y. This isn't true in ...


15

Magic numbers, you have at least one of them in class App: private async Task PlaySequence() you should extract this to either a constant or better to a property, so that the speed, aka less sleep, can be adjusted. I don't see the point of the _seed variable. Yeah, I know why you use it, but it just forces you, each time the GenerateSequence() method is ...


15

This looks pretty nice! I have only a few minor nitpicks and practical tips for you. Returning boolean values directly I'm a bit surprised by this: if type(self) != type(other): return False return True I'm wondering if you have a particular reason for not writing simply: return type(self) == type(other) The same goes for all the ...


14

An enum will serve you much better than a list of strings. enum Gesture { Rock = 1, Paper = 2, Scissors = 3, Spock = 4, Lizard = 5 } A naming convention. If a variable is a counter and its name describes what is being counted, it should then be plural. int wins = 0; int loses = 0; int ties = 0; And now at this point, I don't want to ...


14

It's method extraction time! But before that, I have to comment on your Player enum: str is a bad name, name would be better. Speaking of name, all enums have a name() method by default. You don't need your str variable, return name(); instead. Speaking of return name();, that's exactly what the default implementation of toString() already does for enums. ...


14

Implicit worksheet references Sub CreateGrid() ActiveSheet.Name = "GameBoard" Columns("B:Y").ColumnWidth = 2.14 Columns("A").ColumnWidth = 50 Columns("Z").ColumnWidth = 50 Rows(1).RowHeight = 100 Rows(24).RowHeight = 100 Range("A1:Z1").Merge Range("A1").Interior....


13

Here's a bug: Someone can play on an already-won quadrant (is this allowed by the rules?). If they play on an empty space of an already-won quadrant they can 'win again' i.e. trigger the Recalculate logic and potentially get a different Winner. Cell state is bool? and Player is a corresponding bool. It makes sense (code would be more readable) to replace ...


13

Missing the specs You're missing a very important rule of the game: The pattern gets longer each time the player completes the pattern. If the player presses a wrong button, the game ends. You implemented only a one-shot round, which is not so interesting. Mutually exclusive conditions These conditions are mutually exclusive: if n == 1: # ... if ...


13

Style I'm sorry for making it personal here, camelCase, but the preferred styling for function names is that they're, well, camelCased. (Apologies for the bad pun). So not GetBinarySum, but getBinarySum. Removing the Operations class I think your approach for implementation of Operations is flawed. You're taking two Strings, converting them to numbers, ...


12

In my original review of this program I mentioned I really wanted to re-do this in OO. So I have. I recommend you read my first review to get most of the reasons behind much of the code refactoring I did. Right off the bat I created two enums which will be helpful in the rest of the program. a Gesture enum which holds all possible playable Gestures, and a ...


12

The array does not seem like the logical choice for container. You are going to spend most of the time looking things up. This suggests that you need some form of map. Because of its size I would just use std::map. But you only have the true values stored so we only need to check for existence so we can use std::set instead. const std::set<PlayerMove> ...


12

ItemPlayer is a really good idea. Keeping the logic and score centralized in an abstract class for the AI and Human player is a good design, but, you have not taken the idea far enough. The abstract class should have a name field, and a constructor that takes the name: abstract class ItemPlayer { private final String name; ItemPlayer(String name) {...


12

There is not much to criticize in your code.... and there is a lot of code to go though. Mostly really neat, and, it takes a while to 'grok' it, but it comes together OK. Here's a laundry-list of small items (there are no big Items I can see): In TTUtils: getWinCondsWith is a method that is never called from anywhere getAllSmallestFields should be called ...


12

I've a few things you could improve on: For Each c In Range("B2:Y2") c seems like it could use a better name. As I found out, cells shouldn't be used other than for the system function/methods, so avoid naming it that. x = Sheets("GameBoard").Range("A100") y = Sheets("Gameboard").Range("A101") x = x + Vx y = y + Vy Instead of var = var + otherVar, you ...


11

One quick observation I have is about the way you are storing the playable spaces. While a 9 by 9 grid is a simple way to do it, it isn't the most clear way to store and ends up requiring some complicated addressing logic to get each sub-board. Your options to deal with it are a bit limited by C, but there is support for nesting types. You can use this to ...


11

I'm going to leave a broader review to other answers, and only focus on a single line of code: throw new Exception("Gesture is sealed"); Don't do that. There are really two alternatives to throwing exceptions. One is to throw an existing exception, derived from System.Exception. This is trickier than it sounds, because you need to pick your exception ...


Only top voted, non community-wiki answers of a minimum length are eligible