8
\$\begingroup\$

This is one of tasks, which I had to do for the recruitment process at some company. Unfortunately they didn't like it so I've decided to share it here for discussion.

CircleArea.h

#ifndef CIRCLEAREA_H_
#define CIRCLEAREA_H_

//PI Number, with max precision for double (8 bytes)
const double PI = 3.141592653589793115997963468544185161590576171875;

double circleArea(double radius);

#endif

CircleArea.cpp

#include "CircleArea.h"
#include <stdexcept>

double circleArea(double radius)
{
    if(radius < 0)
    {
        throw std::invalid_argument("Radius cannot be less than zero");
    }

    return PI * radius * radius;
}

Why could the solution have been disliked by the recruiters?

\$\endgroup\$
12
  • \$\begingroup\$ Were you required to use separate files? \$\endgroup\$
    – Jamal
    Jun 27, 2015 at 22:31
  • \$\begingroup\$ No, there wasn't any requirements for that. I've just prepared a project with a few files (also for other tasks) and a makefile to compile all of them easily. \$\endgroup\$
    – Sean
    Jun 27, 2015 at 22:36
  • 1
    \$\begingroup\$ The constant is only accurate up to 3.141592653589793 and maybe there is a lack of comments, but I don't really think that would be an issue in such a small task. Can you elaborate as to why they didn't like it? \$\endgroup\$
    – spyr03
    Jun 28, 2015 at 0:26
  • \$\begingroup\$ If you have the original problem statement and any other problems they gave you relevant to this one, it would be helpful to include that to see what they were looking for. If you were given a few problems that revolved around calculating area of shapes, perhaps they were looking for OO-based solutions. \$\endgroup\$
    – Snowhawk
    Jun 28, 2015 at 0:42
  • 3
    \$\begingroup\$ Personally I don't like that the constant PI is stored in the .h file (regardless of how it is defined/calculated). Now any file that includes circlearea.h gets a definition of PI for no reason. \$\endgroup\$ Jun 28, 2015 at 5:25

5 Answers 5

11
\$\begingroup\$

I assume one of the complaints was with the constant. There are various ways to calculate pi, using an exact calculation rather than using some number of digits.

Here's one example (in C++14):

constexpr auto pi()
{
    return std::acos(-1);
}

(For C++11, use a non-auto type, such as double or float.)

The constexpr keyword will allow this to be calculated at compile time. You could still assign the return value to a constant or call the function inline where needed. If you don't have C++11, then you can use const in place of constexpr, and just assign it to a constant.

\$\endgroup\$
3
  • \$\begingroup\$ @Sean: Another one might be with the exception-handling, but I'm not too sure of a better alternative. But you were still right to account for such an argument. \$\endgroup\$
    – Jamal
    Jun 27, 2015 at 23:00
  • \$\begingroup\$ I actually had to use C++14 for this piece of code to work. It was because function's return type is auto. I'm not sure, if I had done something wrong. \$\endgroup\$
    – fsacer
    Jun 30, 2015 at 21:24
  • 1
    \$\begingroup\$ @fsacer: Oh yeah, I forgot to mention that returning auto is C++14. You're right. \$\endgroup\$
    – Jamal
    Jun 30, 2015 at 21:28
2
\$\begingroup\$

Based on the various comments, and your other recent questions with the same context (recruiters didn't like the solution), it seems the problem in this question was just a very small piece of the bigger picture. I'm pretty sure the main reason of the rejection lies elsewhere, not in this simple piece of code.

In addition to @Jamal's answer about the value of PI, the remark by @twohundredping in a comment is also noteworthy. The requirement concerns the calculation of the square of a circle, and doesn't say anything about \$\pi\$. Although it's an obvious detail, the use of \$\pi\$ is still an implementation detail, and therefore it doesn't belong in the header file, it should be in the .cpp file of the implementation.

\$\endgroup\$
1
\$\begingroup\$

Name space crowding?

const double PI = 3.141592653589793115997963468544185161590576171875;

Much like coveted 1 and 2-letters URLs, creating a constant named PI can easily collide with other code. Further, PI looks like a #define for my tastes.

#include <cmath>
double circleArea(double radius) {
  static const double pi = std::acos(-1); // Or some variation 
  return pi * radius * radius;
}

BTW OP's PI * radius * radius has a small advantage over radius * radius * PI as radius * radius may underflow for values just less than sqrt(DBL_TRUE_MIN), but not PI * radius * radius

\$\endgroup\$
1
\$\begingroup\$

Two items in the original code can be improved:

  1. Use the predefined constant M_PI in math.h.
  2. There is no special need to check for radius < 0 because (-r * -r) == (r * r).
\$\endgroup\$
2
  • \$\begingroup\$ i cant agree with the second point, the area of a circle with radius less than zero is not defined, so silently responding as though it is OK is wrong \$\endgroup\$
    – pm100
    Aug 1, 2016 at 23:39
  • \$\begingroup\$ M_PI is not in standard C nor C++. Various implementations do define it. \$\endgroup\$ Aug 2, 2016 at 0:03
0
\$\begingroup\$

I think that an important problem is that you didn't check for overflow and underflow of the parameter, that would produce errors at run time.

See, for instance, the first answer to this question: How to detect double precision floating point overflow and underflow?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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