4
\$\begingroup\$

I have a code that works but is very badly written. The main is very verbose, it is just to test the class. I want to make objects move on a chessboard (a coordinate grid). I've defined these object as Vehicles. They have a position (x,y) on the plane and a direction. Being forced to move only North-South-East-West possible directions could be

  1. North: (0, 1)
  2. South: (0, -1)
  3. East: (1, 0)
  4. West: (-1, 0)

Vehicles can proceed only step by step (x+1, y), (x, y+1), where x, y are integers. Two vehicles cannot stand in the same position. A vehicle maintains its direction while it is possible, that is while it encounters an occupied position.

So I have to create a void move function which has as arguments the positions of the nearby vehicles (North South East West). Such a function should:

  1. Check if the vehicle can maintain its direction and, if it can, refresh the vehicle position with the new position;
  2. If the cell is occupied, go for another direction, check if the cell in that direction is free...
  3. If every cell surrounding the vehicle is occupied, stay still.

The final situation in the main

I wrote a code that accomplish point 1 but I have difficulties with point 2. In particular, I can't think about a smart way to change the Vehicle's direction. Here's my code so far, it works but surely this is not the best way to do what I want to do:

 #include <iostream>
#include <cmath>

using namespace std;

class Vehicle {
private:
    int x, y;
    int vx, vy;
public:
    int getx() { return x;}
    int gety() { return y;}
    int getvx() { return vx;}
    int getvy() { return vy;}
void sety(int a) { y = a;}
void setx(int a) { x = a;}
void setvx(int a) { 
    if (a != 0) {   
        vx = a / abs(a);
    }else { vx = a;}
} 
void setvy(int a) { 
    if (a != 0) {   
        vy = a / abs(a);
    }else { vy = a;}
}

void move(Vehicle * v) {
    int big = 0;
    int per[8];
    per[0]=0;   // North
    per[1]=1;
    per[2]=0;   // South
    per[3]=-1;
    per[4]=1;   // West
    per[5]=0;
    per[6]=-1;  // East
    per[7]=0;   


    while(big<8) {
        int cont=0;
        for(int i=0; i<4; i++) {
            if (x+vx != v[i].getx() or y+vy != v[i].gety()) {
                cont ++;
            }
        }
        if(cont==4) {
            x = x + vx;
            y = y + vy;
            break;
        } else { 
            vx = per[big];
            vy = per[big+1];
            big = big + 2;
        }
    }
}

void print() {
    cout << "x: " << x << endl;
    cout << "y: " << y << endl;
    cout << "vx: " << vx << endl;
    cout << "vy: " << vy << endl;   
    cout << endl;
}

};

int main() {
Vehicle x;  
x.setx(0);
x.sety(0);
x.setvx(0);
x.setvy(1);
x.print();

Vehicle v[4];

v[0].setx(0);
v[0].sety(1);
v[1].setx(-1);
v[1].sety(0);
v[2].setx(0);
v[2].sety(-1);
v[3].setx(1);
v[3].sety(0);

for(int i = 0; i<4; i++) {
    v[i].setvx(0);
    v[i].setvy(1);
    v[i].print();
}

x.move(v);
x.print();

v[2].setx(0);
v[2].sety(-2);
v[2].print();

x.move(v);
x.print();


}
\$\endgroup\$
  • \$\begingroup\$ "but I have difficulties with point 2. In particular, I can't think about an effective way to change the Vehicle's direction." You probably didn't get the point of closure. Code not yet written, or not working as intended!. \$\endgroup\$ – πάντα ῥεῖ Apr 1 '18 at 14:01
  • \$\begingroup\$ It is written and it is working. My implementation though is long and not smart. \$\endgroup\$ – Gitana Apr 1 '18 at 14:03
  • \$\begingroup\$ I've changed "effective" with "smart" \$\endgroup\$ – Gitana Apr 1 '18 at 14:04
  • \$\begingroup\$ Well, let's see ... Sometimes I'm overzealous with down- and closevotes. \$\endgroup\$ – πάντα ῥεῖ Apr 1 '18 at 14:05
5
\$\begingroup\$

The first thing to do to clean up the code would be to make a

struct pos {
   int x;
   int y;
};

This will simplify the remaining of the code, making it easier to reason about the logic.

For example, right now it is possible to set the vehicle' velocity (direction) to {1,1}, which your problem statement doesn't allow.

You should then also define an equality comparison for your location.

Your move function seems to have hard-coded the number of vehicles in play. Instead you should pass it a std::vector<Vehicle> by reference, and use size().

Once you fix these things, the logic of the move function is quite OK. It is quadratic in complexity, but that is OK for few cars. If you ever intend to increase the number of cars by orders of magnitude, there are two ways of improving the code:

  1. Limit the world, and keep a grid where you mark if it is occupied or not.

  2. Store the cars in a std::unordered_map, with their position as key. Then you can look up a position in constant time.


Edit: as suggested in the comment below, using namespace std should not be used. There are several really good reasons for not using it, and the only reason to use it is "I'm lazy". Look around other answers on Code Review for reasoning. This point comes up in about half of the C++ questions.

\$\endgroup\$
  • 1
    \$\begingroup\$ @πάνταῥεῖ Agree, but you had already written that, so I didn't mention it. Since you've deleted your answer I'll add that to mine. All the other difficulties to read the code would go away with my suggestion, I hope. \$\endgroup\$ – Cris Luengo Apr 1 '18 at 14:42
  • 1
    \$\begingroup\$ @Gitana: where you write for(int i=0; i<4; i++) you are iterating over the 4 cars. You pass a vector with all cars to the move function, and look at the position of each of the 4 cars. If you want to change your program to work with 5 cars, you'll need to change this function to avoid collisions! \$\endgroup\$ – Cris Luengo Apr 1 '18 at 14:50
  • 1
    \$\begingroup\$ @Gitana: while(big<8) is a loop over the 4 directions (though that is not clear because you need an 8 here). If you use a struct for coordinates, this would be while(big<4), and more readable. \$\endgroup\$ – Cris Luengo Apr 1 '18 at 14:52
  • 3
    \$\begingroup\$ @Gitana: That is not what you do, and you don't document that anywhere. How do you intend to find these 4 nearest cars? It seems to me that that is a harder problem than just looking at all cars for possible collisions. \$\endgroup\$ – Cris Luengo Apr 1 '18 at 14:57
  • 2
    \$\begingroup\$ It's not hard replacing that 4 with v.size() if v were a std::vector as you should be using anyway. C arrays can be useful at times, but should be replaced where possible. \$\endgroup\$ – Cris Luengo Apr 1 '18 at 14:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.