Re: svn commit: r24349 - in trunk/subversion/bindings/java/javahl: native src/org/tigris/subversion/javahl

On Mon, 02 Apr 2007, hwright@tigris.org wrote:
...
> --- (empty file)
> +++ trunk/subversion/bindings/java/javahl/native/InfoCallback.cpp Mon Apr 2 14:53:06 2007
...
> +/**
> + * Destroy a InfoCallback object
> + */
> +InfoCallback::~InfoCallback()
> +{
> + // the m_callback does not need to be destroyed, because it is the passed
> + // in parameter to the java SVNClient.blame method.
> +}

Cut-n-paste of SVNClient.blame.

...
> +/**
> + * Callback called for a single path
> + * @param path the path name
> + * @param pool memory pool for the use of this function
> + */

We don't want tool-formatted doc strings in the .cpp file, only in the
header file. And there, we should probably be using Doxygen markup,
rather than the JavaDoc that's crept in.

> +svn_error_t* InfoCallback::singleInfo(const char *path,
> + const svn_info_t *info,
> + apr_pool_t *pool)
> +{
...
> + info_entry infoEntry;
> + SVN_JNI_ERR(createInfoEntry(infoEntry, path, info, pool), SVN_NO_ERROR);
> + jobject jinfo2 = createJavaInfo2(&infoEntry);

We have two naming conventions going on here:

a) makeJFoo
b) createJavaFoo

I don't mind which we use, but would prefer to make things consistent.

> + env->CallVoidMethod(m_callback, mid, jinfo2);
> + // Return SVN_NO_ERROR here regardless of an exception or not.

What do we do with any Java exception that's thrown?

...
> --- (empty file)
> +++ trunk/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/InfoCallback.java Mon Apr 2 14:53:06 2007
...
> +/**
> + * this interface is used to retrieve info each result in a
> + * SVNClientInterface.info2 call.
> + */
> +public interface InfoCallback
> +{
> + /**
> + * the method will be called for every line in a file.
> + * @param info the Info2 object
> + */
> + public void singleInfo(Info2 info);
> +}

I'm not a big fan of this method name (nor singleLine, FWIW). Better
suggestions?

...
> --- trunk/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java (original)
> +++ trunk/subversion/bindings/java/javahl/src/org/tigris/subversion/javahl/SVNClient.java Mon Apr 2 14:53:06 2007
...
> + public Info2[] info2(String pathOrUrl, Revision revision,
> + Revision pegRevision, boolean recurse)
> + throws ClientException
> + {
> + MyInfoCallback callback = new MyInfoCallback();
> +
> + info2(pathOrUrl, revision, pegRevision, recurse, callback);
> + return callback.getInfoArray();
> + }
...
> +
> + /**
> + */

Missing JavaDoc. Should indicate that instances of this class aren't
thread-safe.

> + private class MyInfoCallback implements InfoCallback
> + {
> + private List infos = new Vector();

As this Collection doesn't need to be thread-safe, you should use an
ArrayList instead of a Vector.

> + public void singleInfo(Info2 info)
> + {
> + infos.add(info);
> + }
> +
> + public Info2[] getInfoArray()
> + {
> + Info2[] infoArray = new Info2[infos.size()];
> + Iterator it = infos.iterator();
> + int i = 0;
> +
> + while (it.hasNext())
> + {
> + infoArray[i] = (Info2) it.next();
> + i++;
> + }
> +
> + return infoArray;

You can replace this entire method body with a single line:

               return (Info2 []) infos.toArray(new Info2[infos.size()]);

http://java.sun.com/j2se/1.3/docs/api/java/util/Collection.html#toArray(java.lang.Object[])

> + }
> + }
> }
...