[Clam-devel] [PATCH] segmentation in chord extractor as a seperate class

[David García Garzón]

Related to both topics, commenting inner code and the closeLastSegment method.

// Closes a segment opened in one of the previous iterations
if (lastChord != 0) {
	_timePositions.push_back(currentTime);
}

Doesn't that the same than the closeLastSegment method? Well it does an 
additional check. Let's see how to remove it:

Why does the dumper need to know about lastSegment? It is a inner question of 
the segmentator? We should move it to a private member of the segmentator. 
And once we make it an inner member and we initialize it to 0 (None), we can 
assure that _timePositions.size()==0 implies that lastChord != 0, so the last 
condition checks both conditions, you can remove that, and then you can unify 
the duplicated code.

A very fine grained compile-test-and-commit-after-each-modification list could 
be:
* Adding a segmentator member called _lastChord and initialize it to 0
* Using new member instead of parameter inside doIt and closeLastSegment
* Remove the parameter
* Remove the dumper member
* Remove the additional condition of closeLastSegment
* Do a call from doIt to closeLastSegment instead the inlined code

As always, duplicating, switch use, removing old.


On Wednesday 25 July 2007 12:23:30 David García Garzón wrote:
> On Wednesday 25 July 2007 00:39:17 Roman Goj wrote:
> > Accessors are here :)
> >
> > There's only one problem - the closing of the last segment - I'd like
> > to make _lastChord local for ChordExtractorSegmentation, not passed
> > with doIt (or is this not the right way?). So I'd like to be able to
> > check whether there is need for the closing of the last segment from
> > inside ChordExtractorSegmentation, so I added this:
> >
> >         void closeLastSegment(unsigned & lastChord, CLAM::TData &
> > currentTime ) {
> >                 if (lastChord != 0 & _timePositions.size() != 0)
> >                 {
> >                         CLAM_ASSERT(_timePositions.size()%2==1,
> > "Attempting to close last segment even though all segments have been
> > closed");
> >                         _timePositions.push_back(currentTime);
> >                 }
> >         }
> >
> > to be called from the dumper's destructor... any nicer way of doing this?
>
> Well, this call has no sense at all when we move to a streaming context. So
> in any case we should deal with this case when visualizing segments. Having
> a closing segment point is a pool requirement. But i am not sure that this
> should be a requirement for the streaming representation. Keep it like that
> and let's see later as we move on.
>
> > Also - removed the commented out debugging code I introduced in the
> > first patch... further cleaning up coming... :)
> >
> > Changelog:
> > * added accessors to _timePositions and _chordIndexes in
> > ChordExtractorSegmentation
> > * added closeLastSegment in ChordExtractorSegmentation
> > * removed commented out debugging code in ChordExtractorSegmentation
>
> Nice, committing.
>
> > One more doxygen question though...
> > If I make comments like this
> >
> > // This line does something extra neat
> > Cool.extraNeat( new wicked );
> >
> > Should I make them also doxygen comments (or is doxygen supposed to be
> > used, as I'm guessing, just for variable/function declarations?) I can
> > check I guess somewhere... ok, found it:
>
> No. Inner comments are not parsed by doxygen. You should think that doxygen
> comments are the ones that appear here:
> http://clam.iua.upf.edu/doc/CLAM-doxygen/
>
> Besides that, in CLAM, we consider that commenting inner code a bad smell
> of having obscure code. So we have the rule that instead commenting inner
> code, we should make it clearer, by using proper variable names and
> methods. One of the smarter rules (that you can find in Martin Fowler's
> Refactoring book) are when you comment some code telling that 'it does
> someting', wrap it into a method called DoSomething and get ride of the
> comment.
>
> While commenting inner code is considered a bad smell, commenting method
> with doxygen is a very good thing as they allow a developer to use this
> method as a blackbox.
>
> > "Doxygen allows you to put your documentation blocks practically
> > anywhere (the exception is inside the body of a function or inside a
> > normal C style comment block)."
>
> So you answered your own questions. :-) Anyway this has given me the
> opportunity of advicing you about inner code comments.
>
> > in the manual link on doxygen.org, look up time 5 min. No RTFMs for me
> > this time then I guess ;-)
>
> _______________________________________________
> Clam-devel mailing list
> Clam-devel at llistes.projectes.lafarga.org
> https://llistes.projectes.lafarga.org/cgi-bin/mailman/listinfo/clam-devel