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

[Roman Goj]

On 7/25/07, David García Garzón <dgarcia at iua.upf.edu> wrote:
> 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.

true, this will disappear soon :)
maybe I'll keep the additional assert though? It will hopefully make
things easier to debug when I start making changes to the segmentation
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

I should keep a more detailed TODO :( that was to be one of my next steps :)

> * 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

I think closeLastSegment() will still need to be called from within
the dumper's destructor... I can see no way for
ChordExtractorSegmentation to know that it's the end of the file and
any still opened segments need to be closed... (so closeLastSegment()
still needs to be called from the dumper at the end of the wav/mp3
file) or did I just miss something in your explanation?

Anyway, I'm just getting round to doing all the refactoring, so in
some time we should have cleaner code to discuss :)

roman

> 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
>
>
>
> _______________________________________________
> Clam-devel mailing list
> Clam-devel at llistes.projectes.lafarga.org
> https://llistes.projectes.lafarga.org/cgi-bin/mailman/listinfo/clam-devel
>