Aimbot for open-source FPS game Assault Cube

I am actually quite new to C++, although I have previously programmed in Java. Do you see any ways I can improve readability, maintainability and performance, and make it more object-oriented?

```
/*
Control + 0 = enable aimbot
*/

#include "stdafx.h"
#include

const float pi = 3.14159265358979f;

/*
I am using int as if it were the same thing as int, int*, etc.
Another more elegant looking method is to use int as if it were int, int**, etc
* but it is more prone to undefined behaviour and I am more likely to get away with the first method.
* See: http://stackoverflow.com/questions/16256158/dereferencing-a-double-level-pointer-results-in-different-behaviour-from-derefer
*/
int* getClosestPointer(int** basePointer, int offsets[], int levels) {
for (int i = 0; i 0) {
float tempDistance = getDistanceBetween(players[0], players[i]);
if (index == -1 || tempDistance (me.xPointer) && (target.yPointer) (me.xPointer) && (target.yPointer) > *(me.yPointer)) {
return atanf(deltaX/deltaY) * 180.0f / pi + 180.0f;
} else if((target.xPointer) (me.yPointer)) {
return atanf(deltaX/deltaY) * 180.0f / pi - 180.0f;
} else {
return atanf(deltaX/deltaY) * 180.0f / pi + 360.0f;
}
}

/ Gets the crosshair vertical angle in degrees. /
float getCY(Player me, Player target) {
float deltaZ = (target.zPointer) - (me.zPointer);
float dist = getDistanceBetween(me, target);
return asinf(deltaZ/dist) * 180.0f / pi;
}

int main() {
bool aimbotEnabled = false;
Player* closestTargetPointer = NULL;

// [Base + DF73C] = Player 1 base
players[0] = Player((int**) ((UINT) GetModuleHandleW(0) + 0xDF73C));
int extraPlayersBase = ((int) ((UINT) GetModuleHandleW(0) + 0xE5F00));

while (true) {
// [Base + E5F00] = A
// [A + 0,4,8...] = Player 2/3/4... base
for (int i = 0; i < getNumberOfPlayers() - 1; i++) {
players[i + 1] = Player(extr

Your C++ is very C-like. I would start by taking advantage of C++:

-
You will almost always want to use std::vector instead of arrays.

For example:

std::vector players;
players.reserve(32);

This will create a vector large enough to hold 32 Players. If you ever need room for more, the vector will grow automatically when you add more players. By accessing elements using players.at(index), you will get range-checking.

To get the number of elements in a vector, you can use players.size() instead of using the sizeof array division hack.

-
Your struct typedef-ing is a common idiom in C, because in C you need to qualify struct identifiers with struct in order to use them: void foo(struct Bacon* bacon). In C++ you don't have to do this. I would therefore change your definition of Player to:

class Player {
public:
    // contents
};

-
The variables in Player should normally be encapsulated. This means they should be defined in the private section of the class (above public:). If you need access to the variables from other functions or classes, then you can provide accessor functions:

class Player {
    int** basePointer;
public:
    int** getBasePointer() const { return basePointer; }
};

-
If you really want to use your struct as POD, I recommend making the members const and making a copy every time your need to. This isn't a must, but reduces the chances of unintentional changes.

-
In short, your class (a struct is a class) could and probably should be designed like a class, and not like POD.

-
Like @Jamal points out, you can use enum for your offsets:

enum Offsets { playerBaseXOffset = 0x34, playerBaseYOffset = 0x38 /* ... */ };

If you are expecting to have more offsets of a kind later, use a std::vector instead:

std::vector playerBaseXOffsets{ 0x34 }; // C++11 syntax.

Note the { and } in the last example. Using ( and ) instead compiles, but means something entirely different! (It will default-construct 0x34 = 52 objects.)

-
I'm not sure if all your pointer fiddling is really necessary. If it's not, I recommend you avoid it. It is error prone, and some of the potential errors can be really hard to find. You risk forgetting to dereference a pointer if you're lucky; you get undefined behavior from violating the strict aliasing rule if you're not.

-
Use symbolic, rather than literal, constants. For example:

const std::size_t player_one_base_offset = 0xDF73C;
players[0] = Player((int**) ((UINT) GetModuleHandleW(0) + player_one_base_offset));

I'm sure you agree player_one_base_offset is much more readable and understandable than 0xDF73C.

-
I would normally recommend C++-style static_cast(object) instead of your C-style (type)object casting, but in your case I think it would just reduce readability even more without giving much benefit.

And finally, not directly code related: It's either Pythagoras' theorem, or the Pythagorean theorem.

StackExchange Code Review Q#26503, answer score: 5