A game called Bocce played on a Cartesian grid



.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;








3












$begingroup$


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;










share|improve this question











$endgroup$







  • 1




    $begingroup$
    Don't use std::pow for squaring, you can hardly be less efficient... Just multiply the value with itself instead.
    $endgroup$
    – Aconcagua
    Nov 16 '18 at 9:08

















3












$begingroup$


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;










share|improve this question











$endgroup$







  • 1




    $begingroup$
    Don't use std::pow for squaring, you can hardly be less efficient... Just multiply the value with itself instead.
    $endgroup$
    – Aconcagua
    Nov 16 '18 at 9:08













3












3








3





$begingroup$


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;










share|improve this question











$endgroup$




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;







c++ c++14






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Nov 16 '18 at 8:49









Josay

26k14087




26k14087










asked Nov 16 '18 at 7:29









ChubakBidpaaChubakBidpaa

12217




12217







  • 1




    $begingroup$
    Don't use std::pow for squaring, you can hardly be less efficient... Just multiply the value with itself instead.
    $endgroup$
    – Aconcagua
    Nov 16 '18 at 9:08












  • 1




    $begingroup$
    Don't use std::pow for squaring, you can hardly be less efficient... Just multiply the value with itself instead.
    $endgroup$
    – Aconcagua
    Nov 16 '18 at 9:08







1




1




$begingroup$
Don't use std::pow for squaring, you can hardly be less efficient... Just multiply the value with itself instead.
$endgroup$
– Aconcagua
Nov 16 '18 at 9:08




$begingroup$
Don't use std::pow for squaring, you can hardly be less efficient... Just multiply the value with itself instead.
$endgroup$
– Aconcagua
Nov 16 '18 at 9:08










2 Answers
2






active

oldest

votes


















4












$begingroup$

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...






share|improve this answer











$endgroup$












  • $begingroup$
    All these help me learn C++ better. Thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:27


















4












$begingroup$

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.






share|improve this answer











$endgroup$












  • $begingroup$
    Although it is against the rules to thank people, I shall thank you for taking your time to correct my code. So thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:26











Your Answer





StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);













draft saved

draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207781%2fa-game-called-bocce-played-on-a-cartesian-grid%23new-answer', 'question_page');

);

Post as a guest















Required, but never shown

























2 Answers
2






active

oldest

votes








2 Answers
2






active

oldest

votes









active

oldest

votes






active

oldest

votes









4












$begingroup$

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...






share|improve this answer











$endgroup$












  • $begingroup$
    All these help me learn C++ better. Thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:27















4












$begingroup$

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...






share|improve this answer











$endgroup$












  • $begingroup$
    All these help me learn C++ better. Thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:27













4












4








4





$begingroup$

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...






share|improve this answer











$endgroup$



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...







share|improve this answer














share|improve this answer



share|improve this answer








edited Nov 16 '18 at 9:55

























answered Nov 16 '18 at 9:48









AconcaguaAconcagua

31116




31116











  • $begingroup$
    All these help me learn C++ better. Thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:27
















  • $begingroup$
    All these help me learn C++ better. Thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:27















$begingroup$
All these help me learn C++ better. Thanks.
$endgroup$
– ChubakBidpaa
Nov 16 '18 at 10:27




$begingroup$
All these help me learn C++ better. Thanks.
$endgroup$
– ChubakBidpaa
Nov 16 '18 at 10:27













4












$begingroup$

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.






share|improve this answer











$endgroup$












  • $begingroup$
    Although it is against the rules to thank people, I shall thank you for taking your time to correct my code. So thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:26















4












$begingroup$

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.






share|improve this answer











$endgroup$












  • $begingroup$
    Although it is against the rules to thank people, I shall thank you for taking your time to correct my code. So thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:26













4












4








4





$begingroup$

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.






share|improve this answer











$endgroup$



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.







share|improve this answer














share|improve this answer



share|improve this answer








edited Nov 16 '18 at 10:44

























answered Nov 16 '18 at 10:18









SnowhawkSnowhawk

5,56611229




5,56611229











  • $begingroup$
    Although it is against the rules to thank people, I shall thank you for taking your time to correct my code. So thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:26
















  • $begingroup$
    Although it is against the rules to thank people, I shall thank you for taking your time to correct my code. So thanks.
    $endgroup$
    – ChubakBidpaa
    Nov 16 '18 at 10:26















$begingroup$
Although it is against the rules to thank people, I shall thank you for taking your time to correct my code. So thanks.
$endgroup$
– ChubakBidpaa
Nov 16 '18 at 10:26




$begingroup$
Although it is against the rules to thank people, I shall thank you for taking your time to correct my code. So thanks.
$endgroup$
– ChubakBidpaa
Nov 16 '18 at 10:26

















draft saved

draft discarded
















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid


  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.

Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207781%2fa-game-called-bocce-played-on-a-cartesian-grid%23new-answer', 'question_page');

);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Top Tejano songwriter Luis Silva dead of heart attack at 64

ReactJS Fetched API data displays live - need Data displayed static

天下統一