C++ console app with two colliding objects not Working

Question

#include "stdafx.h"
#include <iostream>
#include <string.h>
#include <math.h>
using namespace std;

struct spaceship { // create the ship
    int x, y;
    char callsign[51];
};

void shiprandloc(spaceship *ship, int maxrange) { //randomize location
    ship->x = rand() % maxrange;
    ship->y = rand() % maxrange;
}

int shipdetcol(spaceship *ship1, spaceship *ship2, float colrange) { //if they collide return a 1
    colrange < 10;
    return 1;
}

int main()
{
    int maxloc = 100, maxcol = 10;
    int numloops;
    cout << "Enter the Number of Collisions to Simulate: ";
    cin >> numloops;
    for (int i = 0; i < numloops; i++) {
        int loopcnt = 0;
        spaceship *ship1, *ship2;
        ship1 = new spaceship;
        ship2 = new spaceship;
        strcpy_s(ship1->callsign, "Red1");
        strcpy_s(ship2->callsign, "Blue1");
        shiprandloc(ship1, maxloc);
        shiprandloc(ship2, maxloc);
        d = sqrt((ship1->x - ship2->x)*(ship1->y - ship2->y)); //find distance between the two ships.
        while (!shipdetcol(ship1, ship2, maxcol)) {
            ++loopcnt;
        }
        delete ship1, ship2;
    }
    return 0;
}

The square root function to check the distance isn't working also the collide returning a 1 if it hits and a 0 if it misses. What am I missing? .

what do you mean by "The square root function to check the distance isn't working" ? You calculate some distance, store it in `d` and then never do anything with `d`...how is this supposed to "work" ? — 463035818_is_not_an_ai, Sep 10 '18 at 16:38
what is the purpose of incrementing `loopcnt`? Seems like instead of incrementing the counter you actually want to run the loop again or do something, no? i think you should explain this code to your rubber duck ;) — 463035818_is_not_an_ai, Sep 10 '18 at 16:40
You should look up the usage of `strcpy_s()` ... you're using it wrong. — Swordfish, Sep 10 '18 at 16:40
There is indeed a _lot_ of faulty logic here. You need to trace your program's meaning on paper until you get what it's doing, then instead make it do whatever it is that you want it to do instead. — Lightness Races in Orbit, Sep 10 '18 at 16:43
@LightnessRacesinOrbit uh right, nevertheless I think this code could use a rubber duck. At least I couldnt convince mine of the correctness... — 463035818_is_not_an_ai, Sep 10 '18 at 16:43
`delete ship1, ship2;` - try *not* to do manual memory management in modern C++. Use smart pointers and containers. Naked `new`/`delete` is very much a code smell in modern code, *except* when *implementing* smart pointers and containers (and a *few* other rare corners). 99+% of all programs should not need to ever write `delete` anywhere. — Jesper Juhl, Sep 10 '18 at 16:54
Prefer the facilities in [](https://en.cppreference.com/w/cpp/numeric/random) over `srand()`/`rand()`. — Jesper Juhl, Sep 10 '18 at 16:57
Regarding `delete ship1, ship2;`, [read up on the comma operator](https://en.cppreference.com/w/cpp/language/operator_other). — user4581301, Sep 10 '18 at 16:58
Two situations are commonly despised: one, where user says something "doesn't work" and developer have to guess or ask additional question and one where developer-being-user tells exactly what is wrong and developer asks "What? What doesn't work?" Now the situation, where programmer says that something doesn't work, is simply confusing. — Swift - Friday Pie, Sep 10 '18 at 17:56

score 1 · Answer 1 · answered Sep 10 '18 at 18:11

This wild beast of human imagination

delete ship1, ship2;

deletes ship2, but doesn't delete ship1. Comma here is treated as sequence (comma) operator, and result of such expression is result of last sub-expression.

Your function always returns 1. You probably meant something like this

int shipdetcol(spaceship &ship1, spaceship &ship2, float colrange) 
{
    return  colrange > sqrt(abs(((ship1.x - ship2.x)*(ship1.y - ship2.y)));
}

Note you need absolute value of difference between coordinates.

Lastly, it's C++, so don't use:

#include <string.h>
#include <math.h>

use

#include <cstring>
#include <cmath>

Don't use

char callsign[51];

Use

#include <string>


std::string callsign;

Now you can do:

ship1 = new spaceship { 0, 0, "Red1"};

Also worth nnoting that there's probably no reason to dynamically allocate the `spaceship`s at all. `spaceship ship{ 0, 0, "Red1"};` will probably be sufficient. — user4581301, Sep 10 '18 at 18:20
@user4581301 true, but if I continue digging, there would be roughly 10 lines of code left of entire program :P There is also bad a really bad RNG used, class could have some member functions, yadayada. — Swift - Friday Pie, Sep 10 '18 at 18:24

score 0 · Answer 2 · answered Sep 10 '18 at 16:53

sqrt((ship1->x - ship2->x)*(ship1->y - ship2->y));

Your sqrt will try to take the square root of a number, but if negative, will cause a domain error to occur. So, you should check if any of the subtractions result in a negative number, since it could make the multiplication result on a negative number too, messing up the result.

And here:

colrange < 10;
return 1;

Your code isn't checking IF colrange is less than 10, it's just writing an expression. It should be:

if(colrange<10){
return 1;
}
else
{
return 0;
}

it \*really\* should be `return colrange < 10;` – Swordfish Sep 10 '18 at 16:55 — Swordfish, Sep 10 '18 at 16:55

C++ console app with two colliding objects not Working

2 Answers2