gamecreature / QtAwesome

QtAwesome - Font Awesome for Qt Applications
Other
773 stars 147 forks source link

Finish intialization in the constructor #16

Closed gsauthof closed 8 years ago

gsauthof commented 8 years ago

I think that having the possibility to construct a QtAwesome object that is only in a half-initialized state isn't very useful.

Having to construct and call an init function directly after that isn't C++ idiomatic.

Also, eliminating the explicit init() function makes the API harder to misuse.

See also:

http://stackoverflow.com/a/3786967/427158

gamecreature commented 8 years ago

I agree the API it's a little bit harder to call the init function.

But my opinion is to not a nice thing to have, exceptions in constructors... Exceptions for such a simple library, make the code more complex and result in needlessly try-catch constructs. Also I don't want to force users of this library to make use of exception constructs. A simple initialization that succeeds or fails .

Btw you always have the possibility to inherit from QtAwesome and create your own all-wrapping exception throwing constructor-subclass. ;)

The main focus on this library is to keep it simple clean and lean. That's also the reason I throw overboard c++11.

As a side-note and also requested earlier, is my opinion about making a singleton class from this library (similar consideration):

I’ve been thinking about singleton when I created the library. I choose not to use it, because (in theory) you can use other fonts with this library. Also this current construct makes it possible to control exactly when the font is loaded. Also singletons are very limited, which prevents descent inheritance and specialization in subclasses. So that’s the reason I didn’t make a singleton :)

gsauthof commented 8 years ago

Ok, then we have different opinions. :)

I think that exceptions really simplify error handling code and make it more robust. For example, if I initialize 10 things that may throw I just have to write one catch block. It is arguably less tedious and error prone in many use cases. See for example John Kalb's series for a good overview and comparison of different error handling methods.

Anyhow, if you really don't want to introduce exception throwing code you can apply the named constructor idiom instead. For example like this:

class QtAwesome : public QObject
{
  Q_OBJECT
  protected:
    QtAwesome(QObject *parent);
  public:
    static *QtAwesome create(const QString &fontname, QObject *parent = 0);
    static *QtAwesome create(QObject *parent = 0);

    // ..
};

Instead of:

QtAwesome* awesome = new QtAwesome( qApp );
bool r = awesome->initFontAwesome(); 
if (!r) {
   // report error and exit somehow
}

One then just needs to write:

QtAwesome* awesome = QtAwesome::create(qApp);
if (!awesome) {
   // report error and exit somehow
}

That would also be an improvement of the API (i.e. no possibility of an half initialized object, harder to misuse it) and the 'named constructor' doesn't throw.

gamecreature commented 8 years ago

I don't disagree with you. They have their place... In a project exceptions are a good tool. But they are only a tool. The focus on this library is to keep is as simple a possible. I don't want to force people to use exceptions. You can customize QtAwesome in the way that fits your project coding style by wrapping it in your own enforced constructs.

The main idea behind the project is to keep the code as open as possible, and give the developer as much freedom as possible without enforcing a style or method.

The named constructor methods are possible, but aren't inheritable... You can create you own MyCustomQtAwesome::create method if you like.

The same reason I choose to use in integer for the 'icon' method in stead of the fa::icon enum type. This gives people more flexibility for adding custom icons etc.

I appreciate your work and your input, but I'm not incorporating this pull request, because I don't want to enforce exception handling and I don't want to break the code of everybody who's using this library.