Hot answers tagged

45

I see some things that may help you improve your code. Move loop invariants outside the loop To maximize performance, moving loop invariants outside the loop often helps. This is an optimization that many compilers can make on their own, but it helps to write it explicitly. In particular, the nested loop within main where the image is recalculated. ...


18

Namings Unlike in Python, all-caps names are usually left to macros (which you should avoid, by the way) in C++; for example: M_PI, CHAR_BIT and so on. Moden Language Features There are several places where you can take advantage of modern C++ features: constexpr: pure constants and magic numbers should be declared as constexpr. static constexpr int ...


13

I see a number of things that may help you improve your program. Fix the formatting It's not clear to me if the code as posted is what you see when you look at it, or if there was a problem pasting the code, but in either case, it's worthwhile to fix the formatting. In particular, consistent indentation and consistent use of a particular style is really ...


13

I'm going to comment mostly on performance aspects of your code here. Stylistic parts of your code should be improved upon by someone more conversant in C++ than I am. Magic Numbers You have factored out some constants, but when you calculate zooming, 0.9 and 40 are "magic numbers", numbers with no explanation as to what they do, and those whose usage ...


11

Here are some of the notes i jotted down while reading through your code: If you're going to make a copy of a string that is passed in by const reference, you might as well pass it by value and then move it. For example, in split: std::vector<std::string> split(std::string str, const std::string& delimiter) { // Just use str directly without ...


10

Use library functionality Mandelbrot images are calculated with complex numbers. C++ offers the <complex> header which contains a template for generating complex numbers out of floats or other numeric types. Initialize at definition Many compilers can give you warnings about code paths with uninitialized variables but in general it might be good to ...


8

Looking at your profiling information you spend a total of 14.5s in handle_collision() out of which 3.8s is spent in get_nearby_entities. So your culprit may actually be somewhere else (I can't tell without the rest of your source). Precalculate/Cache nearby status You are calculating the nearby entities too many times. Consider this, you have a cluster of ...


8

Here are some things that may help you improve your code. Don't declare enum const In snake.h, the direction enum is declared as const but this is an error, since only functions and objects can be declared const. Use const where practical The Snake::getSnakeSize() doesn't alter the underlying Snake and so it should be declared const. Additionally, I'd ...


8

Your program is definitely improved over the last version. Good job! Here are some ideas for you about further improvements. Make the object interface easy for the user The app object has two public functions, start and end that are apparently intended to be called in that order. To me, it would make more sense to eliminate end and simply move the ...


7

The code was big so my review of it will probably not be that complete. I'll try to go in order of top to bottom of the code that was listed. Force vs Acceleration In your body structure, you track forceX and forceY. It would be better to track accelerationX and accelerationY, because you will save yourself some computation. When you compute the force ...


7

Just a little comment. I see this pattern used way too often in C++ code: #define ARRAY_LEN(x) (sizeof(x)/sizeof(*x)) That's fine and dandy if you are writing C, but macros are a very primitive tool to be using on a language that has metaprogramming capabilities (AKA templates). The correct way to implement such construct in C++ is using a template ...


7

I don't know SFML at all, so very partial review. auto random(const std::uniform_real_distribution<float>& dist) { static auto& RandomEngine = randomEngine(); return dist(RandomEngine); } There are two portability bugs here: randomEngine() returns a temporary, but auto& will deduce a non-const reference type here - that's an ...


6

Use auto-properties when you can like in RigidObject and set default values in the constructor. public double Mass { get; set; } I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent. While I'm on naming and ...


6

One may use the following closed formula to compute numFlashes: numFlashes = 5 * mAccumulatedTime / pi. This follows from the fact that the period of the function abs(cos(x)) is pi and, if an oscillating function in variable x has a period T, then the number of oscillations, n, is given by the formula: n = x / T. Thus, your function definition may be ...


6

This is an interesting piece of code, and a well written question, so I'm sure you'll get a number of good reviews. Here are some observations that may help you improve your code. Refine your objects The Node and Body objects both have exclusively public members, which is not very good design. Every piece of code that interacts with them then reaches ...


6

I just want to say that your code is very readable and understandable! Things you could do better: Indentation The function and class definitions are not indented when they should be. I know it means that most of the code would be indented, but it adds to the readability, so please do it. How this helps you: It lets you separate functions more easily ...


6

Use Meaningful Names Rather than: dungeon.loadFromFile("/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/DungeonBack.png"); I'd rather have something like: char const *background_file = "/Users/danielrailic/Desktop/Xcode /NewGame/ExternalLibs/Sprites/DungeonBack.png"; dungeon.loadFromFile(background_file); Likewise, rather than: if (...


6

Design - Singletons A singleton may be a reasonable choice for the logger (it's used everywhere and there should only be one of it). But for the other systems, it's best to stick with ordinary class instances, and pass references to them around as necessary. This may seem like more work, but it makes dependencies clearer. It also makes the lifetimes and ...


6

No singletons The singleton pattern tries to solve exactly one problem and that is to make sure you always just have one single instance of an object. The issue with your code is, that this requirement isn't needed. Why should there only ever be one resource manager? Are there any critical issues when there are more than one? As such the singleton pattern ...


5

Here are some observations that may help you improve your code. Refine your objects The Node and Body objects both have exclusively public members, which is not very good design. Every piece of code that interacts with them then reaches inside and manipulates the object members directly which is prone to error and makes maintenance difficult. Also, the ...


5

John Deters did a great job answering this; I just have a couple more comments I'd like to make: You can start off by creating an EntityManager class that you put all your game entities in (player, ball, enemy paddle). This will take out the ball *gameBall; enemy *enemyPaddle; player *playerPaddle; code, and instead, you could have EntityManager ...


5

A "master class" is a very close relative to a singleton or a global. It can easily be a design which violates the Dependency Injection principle, and leaves you with non-modular code. In this case your engine is also violating the Single Responsibility principle, as it's doing a lot of unrelated tasks: it's handling windows, providing the game's main ...


5

few thing will come in handy for your game at this point Resource Management resources are heavyweight multimedia items, such as images, music themes, or fonts. "Heavyweight" refers to the fact that those objects occupy a lot of memory, and that operations on them, especially copying, perform slowly. This affects the way we use them in your application, as ...


5

Basic structure With all its members static, your Game class looks a whole lot like a namespace. I'd either make it a namespace, or else make everything that's specific to a particular game (e.g., the current score) non-static so that each game object really represents an actual game. The latter is probably preferable, but either is an improvement on the ...


5

I have to start with saying that this is a really cool and well made project! If you would like to improve the performance of your code, I recommend offloading work to GPU shaders. mandelbrot easily fits into the description of a fragment shader. You can use OpenGL through SFML to achieve this, see this and this.


5

I'm a little bothered by the piecemeal conversion from rectangular to polar coordinates. I think I'd rather wrap them up into a little neater package: std::pair<float, float> r2p(float cx, float cy) { auto len = std::sqrt(cx * cx + cy * cy); auto angle = atan2(cy, cx)*180/M_PIl - 90; return {len, angle}; } It's open to some argument that ...


5

Adding to what @BenjaminPhillipe has already mentioned: You shouldn't include math.h, but should include cmath instead. math.h is a C header while cmath is a C++ header (with ultimately about the same content). I am somewhat astonished that your current code compiles, as you use std::cos and std::sin while the math.h is only guaranteed to put its ...


5

I see some things that may help you improve your code and offer some suggestions regarding your main question. Think carefully about class design The main question you've asked here is about class design. It often does make sense to separate the logic from the presentation of the data. Reasons for doing so include: simplifies any later adaptation (e.g. ...


4

for this little bit of code here // TODO: Insert Update Code Here if (Keyboard.IsKeyPressed(Keyboard.Key.Down)) MainCircle.AddForce(0, 1.5F); if (Keyboard.IsKeyPressed(Keyboard.Key.Up)) MainCircle.AddForce(0, -1.5F); if (Keyboard.IsKeyPressed(Keyboard.Key.Right)) MainCircle.AddForce(1.5F, 0); if (Keyboard.IsKeyPressed(Keyboard.Key.Left)) ...


4

Python isn't fabulous at handling large numbers of math operations, but hundreds should be feasible. I agree that this doesn't look like it's primarily the fault of the collision grid. That said, it should be possible to speed up the grid calculation. I'd switch the grid to a really crude modulus of the object's position so that you don't calculate ...


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