do-plugins team mailing list archive

[Karol Będkowski <karol.bedkowski+lp@xxxxxxxxx>]

> Review: Needs Fixing none
> Please review mono style guidelines, do not use _foo, m_foo, or any
> syntax like that for member fields.  

Done.
 
> when checking if a string has contents, you should use
> string.IsNullOrEmpty ()  

Fixed

> Services.cs:102; you pass a list with the ref keyword, it seems a bit
> cleaner just to return a List object. What you have it looks like if
> LoadServices is called more than once you'll get duplicates. I see
> that you clear this before calling it in SystemItemSource, but that's
> bad encapsulation, do the work in one place, don't split it up across
> classes.  

Fixed

> Services.cs:132; can you check if the string is in a blacklist before
> you even add it to result? That should save you some work.
> 
> You do a lot looping through lists to check if an element exists in
> it, List has a Contains member function which will save you some
> lines of code.  

Yes, I can. 
But I have many scripts in /etc/init.d/. If I use Contains (probably)
all items in blacklist will be checked for each script.
My method is much faster - I iterate over blacklist only once. 

> Do string.Format, not String.Format. Small style thing, this is C#
> not Java  

Fixed
-- 
https://code.launchpad.net/~karol-bedkowski/do-plugins/systemservices/+merge/2317
Your team GNOME Do Plugins Team is subscribed to branch lp:do-plugins/community-future.