Hi Jens, Thanks again.
> Variable names shouldn't start with an uppercase letter — > the convention is to lowercase the entire acronym that > starts a name, so "xmlCharacterSet". I'm aware of that and it's not a habit I'm generally in - a year ago I would always have used "xmlCharacterSet" rather than "XMLCharacterSet", but of late I've been picking up bad habits from the AppKit itself, based on accessor methods (which generally follow the same naming conventions as variables) beginning with initials. :) For instance, -(PDFDocument *)PDFDocument rather than - (PDFDocument *)pdfDocument, and -(NSURL *)URL rather than -(NSURL *)url. So I've recently picked up the habit of using uppercase for "PDF", "XML" etc by osmosis. > > > range = > [cleanedString > rangeOfCharacterFromSet:invalidXMLCharacterSet]; > > FYI: If you ever find this method is a performance > bottleneck, you can make it run faster on long strings by > using the variant of this method that takes a range, and > passing in the range from the last deletion point to the end > of the string. That will avoid scanning the already-cleaned > prefix again. -rangeOfCharactersFromSet:options:range: - makes sense, and will do! Thanks. > > > return (NSString *)[cleanedString > autorelease]; > > The cast isn't necessary; an NSMutableString is an > NSString. A habit from my fear that the compiler will get even fussier (for instance it is these days fussier about conditional expressions). > I find it more maintainable to do the autorelease at the > same time I create the object. I do the same in long methods for the same reason, but in very short methods like this one, where the object is created before a loop and then returned immediately afterwards, I often autorelease it at the end. Point taken though! Thanks again and all the best, Keith --- On Fri, 1/29/10, Jens Alfke <[email protected]> wrote: > From: Jens Alfke <[email protected]> > Subject: Re: NSXML and invalid UTF8 characters > To: "Keith Blount" <[email protected]> > Cc: [email protected] > Date: Friday, January 29, 2010, 5:05 PM > This code looks good. Just a few > possible improvements, in the spirit of code-review: > > On Jan 29, 2010, at 4:00 AM, Keith Blount wrote: > > > > NSMutableCharacterSet *XMLCharacterSet = > [[NSMutableCharacterSet alloc] init]; > > Variable names shouldn't start with an uppercase letter — > the convention is to lowercase the entire acronym that > starts a name, so "xmlCharacterSet". > > > range = > [cleanedString > rangeOfCharacterFromSet:invalidXMLCharacterSet]; > > FYI: If you ever find this method is a performance > bottleneck, you can make it run faster on long strings by > using the variant of this method that takes a range, and > passing in the range from the last deletion point to the end > of the string. That will avoid scanning the already-cleaned > prefix again. > > > return (NSString *)[cleanedString > autorelease]; > > The cast isn't necessary; an NSMutableString is an > NSString. > > I find it more maintainable to do the autorelease at the > same time I create the object, i.e. change the declaration > to > NSMutableString *cleanedString = [[self > mutableCopy] autorelease]; > and then just "return cleanedString" at the end. That way > if you (or someone else) every modifies the code later to > add an earlier 'return' statement, they won't accidentally > create a memory leak if they forget to autorelease it. > > —Jens _______________________________________________ Cocoa-dev mailing list ([email protected]) Please do not post admin requests or moderator comments to the list. Contact the moderators at cocoa-dev-admins(at)lists.apple.com Help/Unsubscribe/Update your Subscription: http://lists.apple.com/mailman/options/cocoa-dev/archive%40mail-archive.com This email sent to [email protected]
