mamarjan / gff3-pltools

A fast parallel GFF3 parser
MIT License
15 stars 5 forks source link

refactor Record.append_to #66

Open lomereiter opened 12 years ago

lomereiter commented 12 years ago

I'll be brief:

http://martinfowler.com/refactoring/catalog/replaceConditionalWithPolymorphism.html

mamarjan commented 12 years ago

Thx for your review :)

On Sun, Jul 29, 2012 at 4:07 PM, lomereiter < reply@reply.github.com

wrote:

I'll be brief:

http://martinfowler.com/refactoring/catalog/replaceConditionalWithPolymorphism.html


Reply to this email directly or view it on GitHub: https://github.com/mamarjan/gff3-pltools/issues/66

pjotrp commented 12 years ago

I think there are people who disagree on polymorphism for all situations. Sometimes a switch is descriptive (more functional in a way).

lomereiter commented 12 years ago

In this particular case polymorphism is better because logic of converting GFF3 to GTF should not be a part of Record class. I'd expect to see it in data_formats.d in a form of an interface IDataFormat and implementations for GFF3 and GTF (sharing a common subclass since only attributes are outputted differently) with a method for printing a record.

Another point is that Marjan is now working on JSON/CSV/table output, and that could also be encapsulated in corresponding classes and make conversion to various formats more uniform, while not keeping all of this in the append_to method.

mamarjan commented 12 years ago

Creating external functions, which can then be used in D as if they were methods seems better to me in this case. The Record class is becoming too large, that is true. But I don't like sub-classing. In this case, it's the same data, only the string representation is different. And then for format conversion to work, I would need to copy the object, which is not good for speed :)

The following could be put in output_formats.d:

void to_gff3(Record record, Appender!string) {...} string to_gff3(Record record) {...} void to_gtf(Record record, Appender!string) {...} string to_gtf(Record record) {...} void to_json(Record record, Appender!string) {...} string to_json(Record record) {...} void to_table(Selected fields, Appender!string) {...} string to_table(Record record) {...}

or something like that. I've seem polymorphism misused, and it's no fun.

I'll think a bit more about this before making changes.

pjotrp commented 12 years ago

Polymorphism is OK - but I agree with Marjan that sub-classing (quickly) gets evil. Seen too many object hierarchies in my life ;). Yes, it is good to think carefully about these choices.