The issues you are interested in are deep, deep issues of software design and idiomatic C++. There’s no “right” answer, and different C++ experts will have different opinions about what the “best” solution would look like.
So, there’s just no possible way I can give an answer that isn’t at least partially coloured by my own biases. Please keep that in mind.
I’d start a design review by suggesting a top-down approach: start with what you think a really good solution should look like, and then figure out the details.
So, okay, what would a really good solution look like. Would it look like File::solve();? I’d say no.
What’s wrong with File::solve();? Well:
- It’s utterly meaningless. If you saw
File::solve(); in someone else’s program, what would you think? What is that line doing? What does it mean? 🤷🏼
- It’s completely inflexible. Whatever
File::solve(); does, it clearly doesn’t give the user any say in how, when, or where it’s doing it. As a user, I chafe at the idea that the person who wrote that function thinks they know more than I do about what I want or need.
- It’s literal nonsense: How do you “solve” a file? 🤨
The name solve() isn’t really the problem; the issue is that there is no good explanation of what is being solved, or how. The real problem here is File. This class does not describe a file, and even if it did, that is not what any of this is really about.
So File is a terrible name. What would be a good name? Well, what is the class actually about? It’s about some kind of challenge, it seems. So, maybe a name like array_modification_problem (or ArrayModificationProblem, if you prefer; I just find snake case easier to read). That would give you array_modification_problem::solve()… which, to me, is a lot clearer, and even reads better. If I saw that in code, I would immediately understand that there is some problem involving modifying arrays, and this line is solving it. Which, while vague, is correct.
There’s still a problem, though… well, half of a problem. The solver is completely inflexible. You can’t control anything about it. What if you don’t want to read the data from std::cin? What if you want to read it from a file instead? And what if you don’t want to write the results to std::cout?
There’s another important reason to give this flexibility: testing. You should ALWAYS test your code. In fact, you should write tests for your code, before you write the code. Code without tests is garbage code; I won’t accept code without tests in any projects I’m in charge of. The best thing you can learn as a coder, is how to PROPERLY test your code.
If you hard-code std::cin and std::cout into your solver, you make it extremely difficult to test. If you take the input and output streams as arguments, then you make it easy to test. For example:
class array_modification_problem
{
public:
static auto solve(std::istream& in, std::ostream& out)
{
// actual solution here
}
static auto solve()
{
return solve(std::cin, std::cout);
}
// ...
};
// Example of how to test, using Catch2:
TEST_CASE("example case")
{
constexpr auto example_input = R"(6
1 4 6 2 8 9
2
2 4
)";
constexpr auto example_output = R"(3
1 8 9
)";
auto in = std::istringstream{example_input};
auto out = std::ostringstream{};
array_modification_problem::solve(in, out);
REQUIRE(out.str() == example_output);
}
Note that I’ve included an overload that defaults the input and output streams to std::cin and std::cout, so you can still do array_modification_problem::solve(); as a shorthand for array_modification_problem::solve(std::cin, std::cout);.
See how being able to provide your own streams as input and output makes testing easy? We simply create a string stream, fill it with the example input, run the solver, and then check that the expected results were written to the output string stream. Easy peasy. If you had no way to specify the streams, this would become incredibly hard.
But let’s go a step further. Because maybe we don’t want to just print the results. Maybe we might want the final vector. We might want to do a more complex problem of which this problem is only a sub-step.
So in addition to the two solve() functions above, I’d also suggest one that takes an input stream, and just returns the final vector:
class array_modification_problem
{
public:
static auto solve(std::istream& in)
{
// read the data from in
// solve the problem
// return the final vector
}
static auto solve(std::istream& in, std::ostream& out)
{
auto result = solve(in);
// write result to out
}
static auto solve()
{
return solve(std::cin, std::cout);
}
// ...
};
This makes the solver much more flexible, and as a bonus, it makes testing even easier.
But we can go even further!
Consider the problem. The input data to the problem is a vector, a single number, then a pair of numbers. So… why not actually code a function that just takes that directly?
static auto solve(std::vector<int> data, int position, std::pair<int, int> range)
{
data.erase(data.begin() + (position - 1));
data.erase(data.begin() + (std::get<0>(range) - 1), data.begin() + (std::get<1>(range) - 1));
return data;
}
All other functions can now be written in terms of this function. You just need two helpers:
static auto read(std::istream& in)
{
// this is basically your processData() function
// read the data vector, the position, and then the two range numbers
return std::tuple{data, position, std::tuple{start, end}};
}
static auto write(std::vector<int> const& data, std::ostream& out)
{
// this is basically your print() function
}
And now everything else is easy:
static auto solve(std::istream& in)
{
auto [data, position, range] = read(in);
return solve(std::move(data), position, range);
}
static auto solve(std::istream& in, std::ostream& out)
{
write(solve(in), out);
}
static auto solve()
{
return solve(std::cin, std::cout);
}
So, so far, the interface of the class looks like this:
class array_modification_problem
{
public:
// Input/output functions ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// Read problem data from stream.
static auto read(std::istream& in) -> std::tuple<std::vector<int>, int, std::tuple<int, int>>;
// Write solution to stream.
static auto write(std::vector<int> const& data, std::ostream& out) -> void;
// Actual solution function ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
static auto solve(std::tuple<std::vector<int>, int, std::tuple<int, int>> problem_data) -> std::vector<int>;
// Nicer interface functions to simplify the above ~~~~~~~~~~~~~~~~~
static auto solve(std::istream& in) -> std::vector<int>;
static auto solve(std::istream& in, std::ostream& out) -> void;
static auto solve() -> void;
};
I’d say that’s a pretty decent interface. It’s easy to use. And so long as you pass legal values, it’s pretty darn hard to misuse.
But now we get to the part that you’re really interested in. Note that in the interface about, I use this large tuple type twice:
std::tuple<std::vector<int>, int, std::tuple<int, int>>
That tuple type is the problem data: the vector, the position for the single removal, and then the range for the range removal. While using a tuple for this works, it’s kind of ugly. So what if we instead use the actual problem class to store the problem data (as you did):
class array_modification_problem
{
public:
// Input/output functions ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// Read problem data from stream.
static auto read(std::istream& in) -> array_modification_problem;
// Write solution to stream.
static auto write(std::vector<int> const& data, std::ostream& out) -> void;
// Actual solution function ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
static auto solve(array_modification_problem const& problem_data) -> std::vector<int>;
// Nicer interface functions to simplify the above ~~~~~~~~~~~~~~~~~
static auto solve(std::istream& in) -> std::vector<int>;
static auto solve(std::istream& in, std::ostream& out) -> void;
static auto solve() -> void;
};
We can still use a bare vector for the result… because that’s all the result is: just a vector.
Now we can implement the problem data like so:
class array_modification_problem
{
std::vector<int> _data;
int _pos;
std::tuple<int, int> _rng;
public:
// Input/output functions ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// Read problem data from stream.
static auto read(std::istream& in) -> array_modification_problem;
// Write solution to stream.
static auto write(std::vector<int> const& data, std::ostream& out) -> void;
// Actual solution function ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
static auto solve(array_modification_problem const& problem_data) -> std::vector<int>;
// Nicer interface functions to simplify the above ~~~~~~~~~~~~~~~~~
static auto solve(std::istream& in) -> std::vector<int>;
static auto solve(std::istream& in, std::ostream& out) -> void;
static auto solve() -> void;
// Constructor for problem data ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// this doesn't *need* to be public; you *could* make it private, but
// since it is safe (assuming you do the checks mentioned below), it's
// okay to make it public; it will make testing easier (because you can
// create problem test data directly, without needing to put it in a
// stream and parse it)
array_modification_problem(std::vector<int> data, int position, std::tuple<int, int> range)
: _data{std::move(data)}
, _pos{position}
, _rng{range}
{
// here we could check that the position and range values are okay
// (like, that all the values are in range for the data vector),
// and if not, throw an exception
}
};
Now, what you’re concerned about—instantiating an object of the class within one of the static functions—is not a problem at all. That’s basically what all of the “nicer interface” functions above are doing. It is possible to instantiate an object of the class outside of those static functions either using read() or the constructor… but it doesn’t have to be; we could make those functions private, and everything would still be just fine (the class would just become a little less flexible).
(Performance-wise, there’s one small problem: The vector of numbers doesn’t get reused, so a whole new vector has to be created for the result. This is an easy fix, though; just add an overload of solve() that takes the problem data by rvalue reference, and then you can steal the vector out of it. But really getting into the performance weeds here will just add complications unrelated to the questions you’re interested in.)
So the above is basically what I would do if I were writing this. As you can see, it’s pretty darn close to what you did! But there are some very important differences, that I really think need to be pointed out. So let’s look at your version.
The first problem I have with your version—other than that I think the name File is bad—is that the class is default-constructible… and the default constructed data is illegal. That’s… pretty bad. In a well-written class, it should never be possible to create an illegal state (at least not without doing something incredibly stupid). Your class creates an illegal state by default.
And why is it illegal? Well, in the default constructed class, eraseEle, the position for the single element erasure, is 0. But… the numbers in the problem are 1-based. 0 is just plain illegal.
It turns out, though, that this isn’t actually a problem, because, for example, the erase1() function doesn’t even use eraseEle; it takes the position to erase as an argument.
Except… that creates an entirely new problem. Because a user of the class can pass literally any integer to erase1(). They can pass 0. Or −1. Literally anything.
So the second problem I have with your version is: all of your intermediate operation functions should be private. There is no reason to give J Rando User the power to call erase1() directly. That’s just inviting them to screw something up.
Note that the problem isn’t that you’ve made all the intermediate steps into functions. That’s fine; I know I didn’t do that above, but maybe I should have. I could have done something more like this:
private:
auto erase_1()
{
_data.erase(_data.begin() + (_pos - 1));
}
auto erase_range()
{
_data.erase(_data.begin() + (std::get<0>(_rng) - 1), _data.begin() + (std::get<1>(_rng) - 1));
}
public:
static auto solve(array_modification_problem problem_data)
{
problem_data.erase_1();
problem_data.erase_range();
return problem_data._data;
}
That’s fine because erase_1() and erase_range() are private, so I don’t need to check that the arguments are valid. I do need to check that they’re valid in the public function, though… except that’s already handled in the constructor. So everything is safe. It’s impossible to screw up. array_modification_problem is always created with valid data (or the constructor will throw), and so you can never call solve() with bad data, and thus it should always return valid results.
This probably isn’t the best way to do this, though, because after the first step, the problem data is now messed up. It’s okay here, because the messed up data is isolated within the solve() function. But you need to be careful if you ever refactor the class. It’s a bad idea to write functions that leave your class in an illegal state, even if they’re only used internally. That’s why I’d prefer to just do the two steps inline in solve(), with no help from other functions. Your mileage may vary.
There are plenty of other issues that arise from making these intermediate step functions public, too. For example, consider what would happen if someone called processData() twice. (This isn’t such a crazy idea. Someone might be solving these problems in a loop, and they might want to reuse the object.)
There are only a few other small things to mention:
size is pointless. v knows what size it is. All the effort you go through to keep size in sync with v.size() is error-prone and wasteful.- All four of the accessor functions are pointless. There is no reason to give users access to the internals of the class.
- Also regarding the accessors: Returning
const values is (almost) always wrong. It doesn’t really help anything—the user can strip away the const effortlessly.
- In all the other functions except the accessors: The final
return; statement is pointless.
- In
processData(): temp should not be declared at the top of the function. It should be declared right where you need it. (Which is right before std::cin >> temp;.)
- Also in
processData(): After reading size, you know how big the vector is supposed to be. At that point, you should call v.reserve(size);. This will prevent repeated re-allocations as you push back the elements, which could be a massive speedup.
So in summary, I think your concern is unwarranted: your solve() function is implemented just fine. It’s everything else that I think is problematic:
- Never allow objects to be constructible in an illegal state. If there is a sane default state, use that. If there is no sensible default state—as is the case here—then don’t allow default construction.
- Never allow objects to be left in an illegal state. Every function that can change the object should leave it in a legal state.
- Don’t make functions public if the user should never be allowed to call them. In your case, most of the functions in your class are dangerous, because they either should only be called in certain situations (like you can only call
erase1() directly after processData()), and/or they can only be called exactly once. Your public interface should be idiot-proof, as much as possible… so giving random users access to functions that are so finicky and dangerous is a recipe for disaster.
- Test your code. Learn how to test properly, and write your classes so they can be easily tested.
