Retrying...

BR,

Jukka Zitting

---------- Forwarded message ----------
From: Jukka Zitting <jukka.zitting@gmai...>
Date: Feb 27, 2006 8:02 PM
Subject: Re: [midgard-cvs] CVS update: /midgard/src/core/midgard/midgard/
To: dev@midg...


Hi,

On 26 Feb 2006 14:33:31 -0000, piotras@tigr... <piotras@tigr...> wrote:
> Log:
>  Initial

Thanks, great work! There are however a number of points I'd like to
raise. See below.

First a general point about method documentation. While the following
doc comment is better than nothing, it actually mostly just duplicates
the knowledge that already exists in the method signature:

/**
* \ingroup midgard
*
* Sets sitegroup's value.
*
* \param[in] mgd MidgardConnection object
* \param[in] value  sitegroup's value container
*
* Sets internal sitegroup's value. GValue can hold string or integer type.
*/
extern void midgard_connection_sitegroup_set(MidgardConnection *mgd,
GValue *value);

See:

    "Sets sitegroup's value."                <=
midgard_connection_sitegroup_set()
    "mgd MidgardConnection object"           <= MidgardConnection *mgd
    "value sitegroup's value container"      <= GValue *value
    "Sets internal sitegroup's value"        <=
midgard_connection_sitegroup_set()
    "GValue can hold string or integer type" <= GValue

An improved doc comment would be something like this (see also my
other comments about this method signature):

/**
* \ingroup midgard_connection
* Sets the sitegroup of the given Midgard connection. The sitegroup
of a connection
* affects all database accesses as described in the Midgard Sitegroup
documentation.
*
* The given \c sitegroup value should contain either a sitegroup id,
guid, or name.
* The connection sitegroup is not changed if the given value is invalid.
*
* \param     mgd   the Midgard connection whose sitegroup value is changed
* \param[in] value the new sitegroup value (sitegroup id, guid, or name)
* \see http://www.midgard-project.org/documentation/concepts-sitegroups/
* \todo needs error reporting
*/
extern void midgard_connection_sitegroup_set(MidgardConnection *mgd,
GValue *value);

Use \param[in] when the parameter is only read by the function,
\param[out] when the value is only written, and \param[inout] when the
value is both read and written. Plain \param is best used when the
access type is not known or when the parameter is a $this object
instance

> /* convention macros */
> #define MIDGARD_TYPE_CONNECTION (midgard_connection_get_type())
> [...]

Why would MidgardConnection need to be a GObject? I see no use case for that.

IMO a simple opaque struct would be much better:

    typedef struct _MidgardConnection MidgardConnection;

Less code, less bugs... :-)

> typedef struct MidgardObjectProperty MidgardObjectProperty;
> typedef struct MidgardObjectPropertyClass MidgardObjectPropertyClass;

What are these doing here?

> extern MidgardConnection *midgard_connection_new(void);

What can I do with an empty connection? IMO the
midgard_connection_open() methods are good enough.

> extern void midgard_connection_reset(MidgardConnection *mgd);

Why is this needed?

> extern void midgard_connection_shutdown(MidgardConnection *mgd, gboolean free_config);

I'd drop the free_config argument and rename the method to
midgard_connection_close() (it rhymes better with
midgard_connection_open()).

> extern void midgard_connection_sitegroup_set(MidgardConnection *mgd, GValue *value);
> extern GValue *midgard_connection_sitegroup_get(MidgardConnection *mgd);
> extern void midgard_connection_lang_set(MidgardConnection *mgd, GValue *value);
> extern GValue *midgard_connection_lang_get(MidgardConnection *mgd);

I believe the naming convention module_class_{set,get}_property() is
more standard than ..._property_{set,get}().

Why GValue? The more specific the API is the better. How about
something like this:

    /**
     * \ingroup midgard_connection
     * Sets the language of the given Midgard connection. The
connection language
     * affects all MultiLang database accesses as described in the
Midgard MultiLang
     * documentation.
     *
     * The given \c language parameter is the two-letter language code
stored in the
     * \c language table in the Midgard database. This method returns
\c FALSE if
     * the given language is not found.
     *
     * \param     connection the Midgard connection whose language is changed
     * \param[in] language the new language code
     * \return \c TRUE if the connection language was changed, \c
FALSE otherwise
     * \see http://www.midgard-project.org/documentation/midgard-and-multilingual-content/
     */
    extern gboolean midgard_connection_set_lang(
            MidgardConnection *connection, const char *language);

Where the lang parameter is the language code to be used.

> extern MidgardConfig *midgard_connection_config_get(MidgardConnection *mgd);

I'd rather not have this. Is there any use case where the connection
configuration is needed after the connection has been opened?

> extern void midgard_connection_loglevel_set(MidgardConnection *mgd, const gchar *level);
> extern guint midgard_connection_loglevel_get(MidgardConnection *mgd);
> extern void midgard_connection_loghandler_set(
>                 MidgardConnection *mgd, guint loghandler);
> extern guint midgard_connection_loghandler_get(MidgardConnection *mgd);

This functionality is already provided by GLib. The most we should
provide is something like the following so clients wouldn't need to
know the midgard-core log domain.

    guint midgard_set_log_handler(
           GLogLevelFlags levels, GLogFunc handler, gpointer data) {
           return g_log_set_handler(G_LOG_DOMAIN, levels, handler, data);
    }

All the other log functions are better implemented by the client application.

BR,

Jukka Zitting

--
Yukatan - http://yukatan.fi/ - info@yuka...
Software craftsmanship, JCR consulting, and Java development