Vfrdtyky

You may know Bocce, so you should know that I've tweaked the rules a little. Three types of balls are thrown: a red type, a blue type, and single black ball called the jack.

These are all on the Cartesian coordinate system. The coordinates of the jack are given, and also distances of an equal amount of red and blue balls from the origins of the system (0, 0). If a ball's distance is larger than the distance of the jack, and if the distance of this ball is larger than the distance of its peer, it's a score. The team with the highest score wins.

Here's the code:

#include <iostream>

#include <map>

#include <vector>

#include <string>

#include <cmath>

#include <array>

#include <algorithm>



typedef std::map<std::string, std::vector<int>> bocce_balls;

typedef std::vector<int> int_vec;

typedef std::array<int, 2> int_arr;



std::string bocce(bocce_balls balls, int_arr jack)

{

    auto distance = std::sqrt(std::pow(jack[0], 2)+std::pow(jack[1], 2));

    int red_count = 0;

    int blue_count = 0;



    int_vec red = balls["red"];

    int_vec blue = balls["blue"];



    if (blue.size() != red.size())

    {

        std::cerr << "Balls were different numbers!" << std::endl;

        return "Fail!";

    }



    std::sort(blue.begin(), blue.end());

    std::sort(red.begin(), red.end());



    for (int i = 0; i < blue.size(); ++i)

    {

        if (red[i] > distance && red[i] > blue[i])

            ++red_count;

        else if (blue[i] > distance && blue[i] > red[i])

            ++blue_count;

    }





    if (red_count > blue_count)

        return "Red Wins!";

    else if (blue_count > red_count)

        return "Blue Wins!";

    else

        return "Draw!";



}







int main() {



    bocce_balls insert;

    insert["blue"] = {2, 9, 3, 4, 5, 1, 10, 2};

    insert["red"] = {20, 11, 3, 4, 3, 1, 2, 3};

    int_arr dist = {2, 2};



    std::cout << bocce(insert, dist) << std::endl;

    return 0;

}

Bocce, as far as I know it, counts all the balls of one player that are closer to the jack than the closest one of his opponent.

That would mean that the distances from origin are irrelevant, instead, you need the distances from the balls to the jack:

auto dx = ball.x - jack.x;

auto dy = ball.y - jack.y;

auto distance2 = dx*dx + dy*dy;

Note that I avoided std::pow in favour to multiplying with itself, as std::pow inefficient for this purpose.

Note, too, that as the root function is strictly monotonely rising, you don't need to calculate it, but you can compare the squares as well and still will get the same results.

I personally strongly recommend that you create a class Ball containing three members: x, y for the coordinates and distance for the distance to jack:

class Ball

{

     int x, y;

     int distanceToJack2;

};

I'm not too happy with int as data type - it depends on the range of valid coordinates, though, if it is suitable or not. Assuming we have 32-bit int, then if the valid ranges for x and y can be covered with 16 bit, you will be safe from overflow while multiplying (this would yield undefined behaviour!), otherwise you'd need to calculate in next larger data type. For selecting the correct data types more safely, you might prefer the data types from <cstdint> header, e. g.:

 int32_t x, y;

 int64_t distanceToJack2;

Or you simply have coordinates and distance in double right away. Be aware that with double, you can quickly get issues due to rounding, so if you compare for one value being less than the other, you should consider them only so if difference is larger than some yet to be defined epsilon value (minimum distance for two values for not considering them equal). You could get some hints to from this question. Sure, it is java, but as long as both C++ and Java follow IEEE 754 for floating point values, it applies for both languages equally. On the other hand, you could then use std::hypot for calculating the distances in a very safe manner (thanks Toby for the hint).

Then you might add:

 void Ball::calculateDistance(Ball const& jack)

 {

     uint64_t dx = static_cast<uint64_t>(x) - jack.x;

     uint64_t dy = static_cast<uint64_t>(y) - jack.y;

     distanceToJack2 = dx*dx + dy*dy;

 }

Finally, you can sort your balls according to the distances to jack and select all balls from one vector of which the distance is smaller than the distance of the first element in the respective other vector.

One last point: std::string is a rather bad key type for the map if you only have two fix values (beware of typos, additionally, map mangement is rather expensive). You might prefer an enum instead: enum class Color { RED, BLUE }; and use this one as key instead. Even better: don't use a map at all, instead:

std::string bocce(std::vector<Ball>& red, std::vector<Ball>& blue, Ball const& jack);

Note that as you need to calculate the distances, which are stored in the balls, you cannot have const references for the two vectors (which otherwise would have been preferrable). From point of view of design, this is at least questionable (but at least, it's quite some improvement already). Ideally, the design would allow to pass in const vectors, but that would require some overhead, so for pragmatism, we might decide to live with the current design...

typedef std::map<std::string, std::vector<int>> bocce_balls;

Are you interested in the ordering of the container? There really is no need to use std::set to wrap the ball distances for each player. std::unordered_map would be faster (consumes more space!), but do you even need a map-type? Have you considered using an array of vectors to represent the distances for each player? Do you need to wrap the distances or can you just pass each player as arguments?

Should the key be a string or an enumeration? Strings are great when you need to make keys on the fly at run-time. At compile-time, spelling errors with strings will result in new entries being created in mutable maps. With strongly typed enumerations, the possible range of keys is predefined.

Should distances be stored as integers or as a floating-point type?

std::string bocce(bocce_balls balls, int_arr jack)

Pick descriptive names that help readers understand what your function is supposed to do. bocce is the name of the game. What does this function do? Calculates the winner for a game of bocce. Cartesian point makes it clear what the type of jack should be. Balls is holding the results for one frame in a game of bocce.

    auto distance = std::sqrt(std::pow(jack[0], 2)+std::pow(jack[1], 2));

Use const to define objects with values that do not change after construction. const is useful for providing an immutable view of mutable data. This allows you to clarify to readers that the variable will not be modified and it prevents surprises when someone unexpectedly changes the objects value.

Also, distance from an origin exists in <cmath>. See std::hypot.

    int_vec red = balls["red"];

    int_vec blue = balls["blue"];

Key-access through a map returns a reference to the mapped value. Here, you've opted into making a copy of the mapped value. If you look back at your function signature, bocce_balls is passed by value. Another copy. I'd advise that you pass the parameter by reference-to-const as to prevent accidental modifications to the map. Keep the copies of red and blue here as you'll need those copies for the sort.

    for (int i = 0; i < blue.size(); ++i) {

        if (red[i] > distance && red[i] > blue[i])

            ++red_count;

        else if (blue[i] > distance && blue[i] > red[i])

            ++blue_count;

    }

i is a signed integer. blue.size() returns an unsigned integer. Each loops does a signed/unsigned comparison. Every time you index with i, the subscript operator is expecting an unsigned integer which leads to implicit conversions that change signedness. Turn up your warning levels.