HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Kattis "Hitting the Targets" utilizing a polymorphic relashionship

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
hittingthepolymorphicutilizingtargetsrelashionshipkattis

Problem

In order to learn C++ I have written a solution to this Kattis "Hitting the Targets" problem

Basically the program is to accept some shapes (circles and rectangles) on std in and a set of points and output the number of shapes that include each point.

Seeing as it is my first C++ program I'm sure I have made significant mistakes.

#include 
#include 
#include 
#include 
class Shape
{
public:
    Shape()
    {
    }
    ~Shape()
    {
    }
    virtual bool contains(int, int) = 0;
};

class Circle : public Shape
{
private:
    int x1, y1, r;

public:
    Circle(const int _x1,const int _y1,const int _r)
    {
        x1 = _x1;
        y1 = _y1;
        r = _r;
    }
    bool contains(const int x,const int y) override
    {
        int x_dist = std::abs(x - x1);
        int y_dist = std::abs(y - y1);
        return x_dist * x_dist + y_dist * y_dist = x1 && x = y1 && y  > shapes;
    std::regex rect_pat{ R"(rectangle (-?\d+) (-?\d+) (-?\d+) (-?\d+))" };
    std::regex circ_pat{ R"(circle (-?\d+) (-?\d+) (-?\d+))" };
    for(int a = 0; a  rect(new Rectangle(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]), std::stoi(matches[4])));
            shapes.push_back(std::move(rect));

        } else if(std::regex_search(line, matches, circ_pat)) {

            std::unique_ptr circ(
                new Circle(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3])));
            shapes.push_back(std::move(circ));
        }
    }
    std::getline(std::cin, line);

    int num_points = std::stoi(line);
    for(int a = 0; a > x >> y;
        int num_containing = 0;

        for(std::vector::size_type i = 0; i != shapes.size(); i++) {
            if((shapes[i]->contains)(x, y)) {
                num_containing++;
            }
        }
        std::cout << num_containing << '\n';
    }
    return 0;
}


I would have preferred to use count_if with a lambda rather than a for loop to count the number of shapes containing the point in order to im

Solution

Virtual destructor

As a simple rule of thumb, if a base class contains any virtual functions, its destructor should also be virtual. The primary reason for it to declare a virtual function is so a derived class can override that virtual function. If you have public derivation, you nearly always want the destructor to be virtual--without that, destroying a derived object via a pointer to the base class gives undefined behavior.

Defining empty functions

As another simple rule of thumb, if you don't have a specific idea of what a function is supposed to do, it's probably better not to define it at all. In your case, the base class' constructor isn't doing anything, so it probably isn't needed at all. The destructor isn't currently doing anything either, but it should be defined as virtual (see previous point).

If the derived classes don't have anything to do in their destructors, you probably don't need to define their destructors.

Factory

You might want to consider moving all the code for reading data from the file and creating an object from that data into a class (or possibly just a function) by itself. This sort of thing is usually called a factory.

Error handling

Right now, if you encounter bad input (neither the "Circle" nor "Rectangle" pattern is matched by a line of input) you simply ignore that line of input and proceed to the next. This is fine for a site like you're dealing with here, but it's rarely good behavior in the real world, where bad input should normally produce some message to the user letting them know what happened.

Standard algorithm

You mentioned using std::count_if to do the counting. You could use it something like this:

std::cout  &s) { return s->contains(x, y); }) << "\n";


Note that in this case, the unique_ptr doesn't really work particularly well, so we end up passing it by reference. Otherwise, the pointer would be moved (transferred) to the algorithm, so the pointer in the vector would have released it; on the second iteration, the vector would only contain null pointers.

This could be handled by using shared_ptrs instead. Another (probably preferable) solution would be to look up (and use) Boost ptr_vector instead.

Adding a Point class

It's probably at least worth considering adding a Point class (or maybe struct) to encapsulate an X/Y coordinate pair.

Prefer member initializer lists over assignment

When you have a constructor that initializes members from arguments, it's usually preferable to do the initialization in a member initializer list. For example, your Circle constructor could look like this:

Circle(const int _x1, const int _y1, const int _r) : x1(_x1), y1(_y1), r(_r) {
}


Note that a ctor with an empty body like this is quite common and expected.

Prefer make_unique over unique_ptr(new T(arguments))

There's rarely much (if any) reason to use an unadorned new to allocate your object(s), and doing so can introduce exception safety issues (which don't arise in this exact code, but I'd advise normally using make_unique as it eliminates the possibility).

Putting most of those together, we could end up with code something like this:

#include 
#include 
#include 
#include 
#include 

class Shape {
public:
    virtual ~Shape() { }
    virtual bool contains(int, int) = 0;
};

class Circle : public Shape {
    int x1, y1, r;
public:
    Circle(const int _x1, const int _y1, const int _r) 
        : x1(_x1), y1(_y1), r(_r) 
    { }

    bool contains(const int x, const int y) override {
        int x_dist = std::abs(x - x1);
        int y_dist = std::abs(y - y1);
        return x_dist * x_dist + y_dist * y_dist = x1 && x = y1 && y  build(std::string const &line) {
        static const std::regex rect_pat{ R"(rectangle (-?\d+) (-?\d+) (-?\d+) (-?\d+))" };
        static const std::regex circ_pat{ R"(circle (-?\d+) (-?\d+) (-?\d+))" };

        std::smatch matches;
        if (std::regex_search(line, matches, rect_pat)) {
            return std::make_unique(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]), std::stoi(matches[4]));
        }
        else if (std::regex_search(line, matches, circ_pat)) {
            return std::make_unique(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]));
        }
        // Probably want to add something here to deal with bad data (e.g., throw an exception).
    }

};

int main(void)
{
    std::string line;
    std::getline(std::cin, line);
    int num_shapes = std::stoi(line);
    std::vector > shapes;

    Factory f;

    for (int a = 0; a > x >> y;

        std::cout  &s) { return s->contains(x, y); }) << "\n";
    }
}

Code Snippets

std::cout << std::count_if(shapes.begin(), shapes.end(),
        [=](std::unique_ptr<Shape> &s) { return s->contains(x, y); }) << "\n";
Circle(const int _x1, const int _y1, const int _r) : x1(_x1), y1(_y1), r(_r) {
}
#include <iostream>
#include <memory>
#include <regex>
#include <string>
#include <vector>

class Shape {
public:
    virtual ~Shape() { }
    virtual bool contains(int, int) = 0;
};

class Circle : public Shape {
    int x1, y1, r;
public:
    Circle(const int _x1, const int _y1, const int _r) 
        : x1(_x1), y1(_y1), r(_r) 
    { }

    bool contains(const int x, const int y) override {
        int x_dist = std::abs(x - x1);
        int y_dist = std::abs(y - y1);
        return x_dist * x_dist + y_dist * y_dist <= r * r;
    }
};

class Rectangle : public Shape {
    int x1, x2, y1, y2;
public:
    Rectangle(const int _x1, const int _y1, const int _x2, const int _y2) 
        : x1(_x1), y1(_y1), x2(_x2), y2(_y2) 
    { }

    bool contains(const int x, const int y) override {
        return x >= x1 && x <= x2 && y >= y1 && y <= y2;
    }
};

struct Factory {
    std::unique_ptr<Shape> build(std::string const &line) {
        static const std::regex rect_pat{ R"(rectangle (-?\d+) (-?\d+) (-?\d+) (-?\d+))" };
        static const std::regex circ_pat{ R"(circle (-?\d+) (-?\d+) (-?\d+))" };

        std::smatch matches;
        if (std::regex_search(line, matches, rect_pat)) {
            return std::make_unique<Rectangle>(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]), std::stoi(matches[4]));
        }
        else if (std::regex_search(line, matches, circ_pat)) {
            return std::make_unique<Circle>(std::stoi(matches[1]), std::stoi(matches[2]), std::stoi(matches[3]));
        }
        // Probably want to add something here to deal with bad data (e.g., throw an exception).
    }

};

int main(void)
{
    std::string line;
    std::getline(std::cin, line);
    int num_shapes = std::stoi(line);
    std::vector<std::unique_ptr<Shape> > shapes;

    Factory f;

    for (int a = 0; a < num_shapes; a++) {
        std::getline(std::cin, line);
        shapes.push_back(f.build(line));
    }
    std::getline(std::cin, line);

    int num_points = std::stoi(line);
    for (int a = 0; a < num_points; a++) {
        int x, y;
        std::cin >> x >> y;

        std::cout << std::count_if(shapes.begin(), shapes.end(),
            [=](std::unique_ptr<Shape> &s) { return s->contains(x, y); }) << "\n";
    }
}

Context

StackExchange Code Review Q#136715, answer score: 6

Revisions (0)

No revisions yet.