I'm not sure how I feel about API's complexity, especially in complex code. I have been concious of keeping the APIs relatively complex i.e. the more complex the object, the more flexibility; the more simple the object, the more restrictive.
It's normal that complexity increases with functionality. What I was referring to is that the complexity of your API increases beyond that, because there are many methods for convenience. The same functionality can be easily provided with a fraction of the public methods, and I think it would help the user get a quicker overview and know what to use in which situation.
I am assuming you are referring to Console Screen when you mention create()?
Yes, and I assumed it would do the same as the constructor. Based on that:
create() methods that (re-)initialize the object made sense in C++98 where it was expensive to copy-assign another object. With C++11 move semantics, this is almost free, and creating a new object assures that it's in a fresh state. One doesn't need to maintain code duplication in constructor and
create().
Now that I realized it's not the same, I think the method should be renamed. "create" without further specifying the object leads to the assumption that a new object of the same class is created. Why not "resizeGrid"? And is that change actually needed or can the user achieve the same by creating a new object?
I'm not sure if or-ing enumerators is better than explicit booleans.
console.printAt(pos, 'A');
console.printCharAt(pos, 'A', true);
console.printCharAt(pos, 'A', true, true);
console.printCharAt(pos, 'A', sf::Color::White);
console.printCharAt(pos, 'A', sf::Color::White, true);
console.printCharAt(pos, 'A', sf::Color::White, sf::Color::Black);
There are 6 possible argument combinations for this simple method. The difference between those calls, as well as the meaning of the magic
true values is impossible to know without learning the API by heart. Even when seeing the function declaration, it's still unclear what happens exactly when colors are overwritten -- which color is then used?
Expressivity is a core criterion for API quality. In my opinion, function overloads and default parameters are overused here -- interestingly, you did
not use it for one of its main purposes: type inference. There's no harm in treating single
chars and
std::strings the same. By the way, character types should be
char, not
unsigned char.
std::string also uses
char.
It may be overkill in this situation, but since you use a lot of foreground-background color pairs, why not treat them as first-class citizens? For example as a type with named constructor idiom:
struct ColorPair
{
static ColorPair foreground(const sf::Color& color);
static ColorPair background(const sf::Color& color);
static ColorPair pair(const sf::Color& foregroundColor, const sf::Color& backgroundColor);
static ColorPair none(); // maybe find better names
// members and private constructor
};
With that, the method calls are much more expressive. A user actually knows what happens when reading the code, without even knowing the exact declarations, or even worse, the implementation.
console.printAt(pos, 'A', ColorPair::foreground(...));
console.printAt(pos, 'A', ColorPair::background(...));
console.printAt(pos, 'A', ColorPair::pair(..., ...));
console.printAt(pos, 'A', ColorPair::none());
I suppose that the reference and pointer parameter overloads can be confusing. The main reason behind the null pointer overloads is to allow specifying parameters via reference instead of pointer but also allow nullifying such object.
But what do you gain by that? The "advantage" of omitting & at call site is shadowed by the confusion arising from the possibility to pass
nullptr, but not
T*. I've never seen an API that does that.
What you're actually looking for are optional types. They will soon be introduced as
std::optional in C++, and
my library Aurora already provides them. They're very easy to use:
aurora::Optional<int> x = 32; // contains a value
aurora::Optional<int> y; // is empty
aurora::Optional<int> z = aurora::nullopt; // is empty
if (x) // check if not empty
output(*x); // access stored int value
The default values for "enabled" boolean setters, in my view, make more sense grammatically. I think I would personally prefer enable and disable methods, or the method not having "Enabled" in its name, but I used this naming scheme to be closer to how SFML names its methods.
But SFML does not allow
setXyEnabled() without a parameter.
I think it's a fundamental design question: do you want to make all users happy and leave it up to them what style to use, or is your focus on a concise API that precisely says "in order to achieve that, you must do this". I personally prefer the latter option by far, because:
- Users can get a quick overview over the functionality when there's no redundancy.
- It's clear that two different methods are actually different and don't just seem so.
- Different users of my API understand each other, because there's only one style.
- There's a consistency throughout my API. Users knowing my API style can use new classes directly, as they're used to the API and have expectations that are fulfilled.
- I have less maintenance overhead as a developer, and potentially fewer bugs.
I don't say you should follow this style, I merely want to give you some insights in how I design interfaces in my libraries and what criteria are important to me personally.
