Re: f-spot r4183 - in trunk/dpap-sharp: launcher lib

["Gabriel Burt" <gabriel burt gmail com>]

Hey Andrew,

Glad to see you're alive!  Haven't seen your status report for this
week yet.  Do you have anything you could blog about, ideally w/ a
screenshot?

On Tue, Jul 15, 2008 at 6:56 AM,  <apart svn gnome org> wrote:
> Author: apart
> Date: Tue Jul 15 11:56:32 2008
> New Revision: 4183
> URL: http://svn.gnome.org/viewvc/f-spot?rev=4183&view=rev
>
> Log:
> Client & initial server work

Please add dpap-sharp/ChangeLog and add detailed entries to it
explaining at a somewhat high level what changed in each file when you
commit.  See F-Spot's main ChangeLog for an example.  And copy the
ChangeLog entry into the svn commit messge (not something that F-Spot
seems to do, but something I like, largely because then it shows up on
svn-commits-list).

> Added:
>   trunk/dpap-sharp/lib/Album.cs

What do you think of renaming lib/ to src/ ?  I think we should, it's
more consistent with other source trees and where people expect the
source to be (IMO).

> +                       Console.WriteLine("Starting DPAP server");

F-Spot and Banshee put a space between the method name and the
parenthesis.  See HACKING.  I don't really care if you use Banshee's
or F-Spot's (though I prefer Banshee's atm), but choose one.

> +                       server.Collision += delegate {
> +                               server.Name = "DPAP" + " [" + ++collision_count + "]";
> +                       };

In general, it's better to use String.Format instead of string
concatenation.  So String.Format ("DPAP [{0}]", ++collision_count).
Plus it's easier to read (IMO).

> -                                       if(tr != null)
> +                                       if(ph != null)

I know you're dealing with the daap-code, but whenever possible try to
use more informative variable names.  Don't abbreviate, and do name
private variables like_this.

> +++ trunk/dpap-sharp/lib/Album.cs       Tue Jul 15 11:56:32 2008
> @@ -0,0 +1,204 @@
> +// Album.cs created with MonoDevelop
> +// User: andrzej at 11:41 2008-07-15
> +//
> +// To change standard headers go to Edit->Preferences->Coding->Standard Headers
> +//

Please fix your header settings in MonoDevelop, and get all your
headers accurate/up to date.

> +            ArrayList photoNodes = new ArrayList ();

Please to now use AraryList, ust a List<T>.

> +            ArrayList deletedNodes = null;

Again, name local/private variables like_this.

> +        internal int LookupIndexByContainerId (int id) {
> +            return containerIds.IndexOf (id);
> +        }
> +
> +               public int getId() {
> +                       return Id;
> +               }

Seems like you have an indenting problem, a mix of tabs and spaces.
Again, choose a HACKING to follow, and be consistent.

Thanks!

Gabriel