18

Be careful with your inequalities. You've listed 500 twice: once as if (Score <= 500) and again as else if (Score >= 500). That's confusing, as the first condition takes precedence. I think that you would be better off with one state variable representing the current difficulty level. The code would also be simpler if you determine NewDifficulty ...


14

Just a quick note about naming: bool Increased100500 = false; bool Increased5001500 = false; bool Increased15003000 = false; Please, don't do that. I mean, yeah, private fields should be camelCase in the first place, but that chunk of numbers is fairly annoying, and it encodes your game logic into an identifier. If you wanted to change your levels, let ...


14

using namespace std Don't use using namespace std. It's considered bad practice. Consistency Use a consistent coding style. You use std::endl and cout, and states and this->states. public first, private last This is a matter of personal preference, but you are usually more interested in how you can access your class. That's why the public section is ...


12

You don't talk about if the score can decrease or if it is possible for the score to jump over one of the difficulty ranges. If neither of these things are possible, the logic to set the difficulty can become very simple by using an enum. enum Difficultly { Normal, Hard, Harder, Hardest } class State { private Difficultly _currentDifficultly = ...


10

Two beginners mistakes: Leading underscore in identifers #ifndef _BasicFsmT_h #define _BasicFsmT_h Using a leading underscore is a bad idea. The exact rules are complex and people usually get them wrong. So just don't use them and you will be safe. See: What are the rules about using an underscore in a C++ identifier? using directives. using namespace ...


10

AJ, I like it. Very nicely coded. Here are some comments. I've omitted things already mentioned by @LokiAstari forward declaration of struct Transition is unnecessary. die, prefer to print errors to stderr exit(1) prefer exit(EXIT_FAILURE) some noisey comments ("exit unsucessfully", "allocate memory" etc) State_create does not initialise transitions (...


10

Ahhh, tile based 2D grid layouts. It's an OO problem for sure. How can you keep things separate if objects on tiles need to know what tile they're on and their neighboring tiles and what's on their neighboring tiles? I did a similar exercise and I still need to think carefully about it. I do feel the best solution is the following: Create an enum ...


10

As the classic "Stop Writing Classes" puts it: the signature of "this shouldn't be a class" is that it has two methods, one of which is __init__ Virtually all of your classes fall foul of this; just because you can use OOP, doesn't mean you always should. Looking at the use of the classes in the code, this was a big red flag: code_input = GetCodeInput(...


9

I don't have much to say. Your implementation is similar to the GoF's (posted here) except that your machine instance is passed by reference to the states' constructors, instead of being passed-in to the states' state-transition methods. Advantage: cleaner syntax of the state-transition method Disadvantage: state instances can't be flyweights I wonder ...


9

One thing that could be changed is making the DFA delta function fully defined: delta :: s -> i -> s because delta should be defined over the entire alphabet for any state. If you wanted to use a state with a Maybe, you could just extract the Nothing portion into the isFinal function. It would also simplify evalDFA to evalDFA (DFA startState delta ...


8

If Score cannot jump suddenly in the levels, for example after Score=50 it cannot jump to Score=550, then you could simplify by using half-ranges in the if statements, so instead of Score >= X && Score <= X + C you could use just Score >= X, like this: void IncreaseScore() { if (Score >= 100 && !Increased100500) { ...


8

Initial impressions There's a lot of code. Interviewers don't like to read a lot of code. In many ways, you have overdelivered on the specification. Since you have indicated that this was an interview-question, I'm going to give you my personal value judgements on these points. I'm ambivalent about the logger. On one hand, it is a nice debugging aid. ...


8

I would store the value to write directly in the Instruction. That way you don't need the if-else in ExecuteInstruction. Instead it's just an assignment. this.tape[currentIndex] = inst.ValueToWrite; I would also store the NextCard value directly. This requires ControlCard to be a reference type but that's already the case. You do need to pass which state ...


8

Java 8 constructs The following function.putIfAbsent(startState, new HashMap<>()); function.get(startState).put(character, goalState); can be simplified by using the computeIfAbsent(key, mappingFunction) method. This method will return the current value associated with the given key or compute a new value for that key and return it. As such, you ...


8

MatchDelegate Much as I love .NET's nominal delegates, I almost always regret using a delegate rather than an interface, so I would introduced an IMatcher (which MatcherAttribute can implement directly) in its place. Granted delegates usually go wrong because I need to serialise them, which won't be an issue here, but the ability to attach meta data could ...


7

You can use list unpacking to transform : takeaction = state_table[current_state][a][1] current_state = state_table[current_state][a][0] into the more concise : current_state, takeaction = state_table[current_state][a] . Also, it shows that the type you should be using to convey current_state and takeaction should probably be ...


6

A riff on @200_success answer // value is the base-score for that level public enum Difficulty { Beginner = 0, Easy = 100, Medium = 500, Hard = 1500 } void IncreaseScore() // yeah, needs a different name // increasing difficulty, not the score { Difficulty NewDifficulty = (Score < Difficulty....


6

You can get rid of the busy-wait loop since Swing keeps another thread alive. It was hogging my cpu doing nothing. You should have a good model/view separation from the start. When I do a small project like yours, I always start by doing a version where the GUI is just some output text to the console. But I am aware that I will move to a real GUI, so I ...


6

Don't Repeat Yourself (DRY) TokenizerT *tokenizer = malloc(sizeof(TokenizerT)); if (!tokenizer) { return NULL; } tokenizer->current = ts; return tokenizer; So if tokenizer is NULL, you return NULL. If not, you return tokenizer. Consider TokenizerT *tokenizer = malloc(sizeof *tokenizer); if (tokenizer) { ...


6

This isn't exhaustive, because I don't have much time now, so I might add some more later. As I said in a comment, I don't buy the idea of looping over one char at a time: it's not documented that the patterns should match exactly one character, and it complicates the definitions of things like Scheme, which could be [a-z]+ and everyone would be happy. Of ...


5

That's typically how C enums are written, but it's done differently in C++. The typedef keyword is no longer needed, and the name is given after the enum keyword. enum State { Idle, Start, Stop, ChangeSpeed };


5

Josay's suggestions are good; I only mention where I have a different idea: The way you create state_table is a bit of an anti-idiom, but in this case I see a benefit too: seeing the number in state_table[2] = ... improves the readability of the state machine as a whole. However, initializing the variable to the nested list comprehension is redundant. I ...


5

Identifiers with two underscores are reserved for the implementation (so don't use them). Also MACROS (things defined by #define) are traditionally all uppercase (because they do not obey scope rules (because they are not part of the C language but part of the pre-processor system) we make them all uppercase to make sure that we don't accidentally clash with ...


5

I am not fond of the hardcoding since that will make it difficult to change your score thresholds later.This rules out Enum's as well. I liked Janos's second answer best, but that means storing a copy of the thresholds in a hash. In addition it will continue to loop through every threshold after the max difficulty is reached just to do nothing. The ...


5

The validation of the text input is not great: The validation logic is sort of split between the action listener (ensure number) and the Team enum (ensure non-zero and not -1). It would be better to have all the validation logic in one place Throwing IllegalArgumentException in case of 0 or < -1 input doesn't seem very useful: nothing happens on the UI ...


5

I see some things that may help you improve your program. Fix your includes Case matters to a C++ compiler, so when you write #include <IOStream> it is not the same as the standard #include <iostream> even if your compiler and/or operating system happens to accept it. Prefer references to pointers The argument to update is a State * but I ...


5

I don't think you should have constructors that don't take a List<ControlCard> as parameter. It doesn't have any value since this way the TuringMachine won't be able to operate. If I want a machine that'll execute 0 operations, I'll pass an empty List<ControlCard>. You should specify visibility modifier for your class members, at least I think. ...


5

In all, this looks like pretty solid code. I have just a few suggestions that may help you improve your code. Use an enum for related constants The states are all related and not just standalone constants. For that reason, I'd recommend encapsulating them all in an enum: enum { NORMAL, SINGLE_QUOTE, DOUBLE_QUOTE, SLASH, MULTI_COMMENT, INLINE_COMMENT, ...


5

I believe you're looking for std::prev: std::prev(tape.end()); The linked page even states the exact comparison of the two: Although the expression --c.end() often compiles, it is not guaranteed to do so: c.end() is an rvalue expression, and there is no iterator requirement that specifies that decrement of an rvalue is guaranteed to work. In particular, ...


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