Do

Merge lp:~jassmith/do/docky-random-polish-and-breakage into lp:do

Proposed by Jason Smith
Status: Merged
Merged at revision: not available
Proposed branch: lp:~jassmith/do/docky-random-polish-and-breakage
Merge into: lp:do
To merge this branch: bzr merge lp:~jassmith/do/docky-random-polish-and-breakage
Reviewer Review Type Date Requested Status
David Siegel (community) Approve
Review via email: mp+3319@code.launchpad.net
To post a comment you must log in.
924. By Jason Smith

Minor cleanup

Revision history for this message
David Siegel (djsiegel-deactivatedaccount) wrote :
Download full text (4.9 KiB)

DockServices.cs
 Does this class need to be static? Have you thought about using the existing
  services class instead of duplicating it here? You can just use IService,
  IInitializedService instead of reinventing the wheel.
 Use a static constructor to initialize fields.
 Please don't use the E1 ?? (E1 = E2) form; use if (E1 == null)...
 For items_service, drawing_service, do_interop_service, just new those in the
  static constructor and use auto-properties.
 For LoadService, you should use OfType<TService>.FirstOrDefault (), then check
 for null and use the Activator if you need it. Or get fancy with ??

DoInteropService.cs
 You may want to use EventHandler and pass (this, EventArgs.Empty) instead of
 using Action.

Default.DrawingService.cs
 Looks good.

Default.ItemsService.cs
 Can you make your fields more generic -- IDictionary instead of Dictionary,
  ICollection instead of List.
 s/Dragable/Draggable

 ItemCanBeMoved (int item)
  return DockItems [item] is DockItem && ItemCanBeMoved (DockItems [item]);
 ItemCanBeMoved (DockItem item)
  return DraggableItems.Contains (item);
 It's very weird that you case DockItems [item] to DockItem.

 DropItemOnPosition, MoveItemToPosition:
  Don't repeat yourself. Make the following its own method:
  if (DockItems.Contains (item) && 0 <= position && position < DockItems.Count)
 MoveItemToPostion (int, int) doesn't do upper bounds checking.

 DockPreferences.AutomaticIcons--
  is a bit scary

 Enumerable.ToDictionary throws a not implemented exception on mono < 2.0

 You should writegeneric Serialize<T> and Deserialize<T> methods instead of
 repeating yourself. You can overload them with Funcs if you want to alter the
 datastructure before/after serialization, which you seem to be doing.

 UpdateWindowItems
  first foreach, you have a line ending with '.'

 SimplifyPositions:
  s/i=0/i = 0
  Why do you call ToArray?

Default.PainterService.cs:
 Bad file name in header comment.
 Use Log<PainterService>

IItemsService.cs
 Don't use List, use the thinnest interface you can -- IEnumerable is best.
 It's very weird to have a readonly property for a mutable type.

 You should not have overloads in your interface. Do calling classes really use
 both versions of MoveItemToPosition and RemoveItem? That may be a sign of a
 poor design decisison.

 A few comments would be nice!

IPainterService.cs
 Wrong filename in heaer comment

ApplicationDockItem.cs
 DesktopFilesDirectories should be static readonly IEnumerable<string>, not a
 property.

BaseDockItem.cs
 MakeIconSurface
  if (icon_surface == null)
   icon_surface = ...
  return icon_surface;

DoDockItem.cs
 const "Summon GNOME Do" at the top of the file

DoInteropService
 Why do you have IDoInteropService and DoInteropService?

DockArea.cs:
 BaseDockItem needs to be public?
 Magic numbers 22, 300, 500?

 Your AnimationStateConditions could hav better indentation (I still think the
 string-based criteria are a bit wierd, would prefer enum). Now that I read
 further and see you writing down "PainterAnimation" over and over again, I
 *really* think you should be using an enum.

 You frequently place an unneeded ( ) around the predicate in your ternary
 e...

Read more...

Revision history for this message
David Siegel (djsiegel-deactivatedaccount) wrote :

Address my comments, then go ahead and merge.

review: Approve
925. By Jason Smith

Cleanup everything in the comments from the merge review (sorry, didn't mean to do this in one big chunk)