New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRY in constructors #5

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@skington

skington commented Jan 2, 2015

I asked to help out on random CPAN modules via the CPAN PR challenge, and I got assigned POE::Component::Client::Keepalive. I haven't ever done anything with POE, so this is me dumped in the deep end.

So while I explore a couple of outstanding issues (one of which I can reproduce partially but I don't understand why, because, well, never used POE), I thought I'd try something simpler. So I thought I'd look at the constructor of one of the main objects, which appears to violate DRY by making the most important information - the associated constant - a comment rather than something that code could do something about.

There are more things I could do on this subject but I thought I'd push a bunch of supposedly-reasonable code and ask first.

Does this look sane?

@rcaputo

This comment has been minimized.

Show comment
Hide comment
@rcaputo

rcaputo Feb 23, 2015

I'm not convinced that this change provides enough benefit to warrant its run time cost.

rcaputo commented on bb8bb42 Feb 23, 2015

I'm not convinced that this change provides enough benefit to warrant its run time cost.

This comment has been minimized.

Show comment
Hide comment
@skington

skington Feb 23, 2015

Owner

Ahah, found the pessimisation I made.

Owner

skington replied Feb 23, 2015

Ahah, found the pessimisation I made.

@rcaputo

This comment has been minimized.

Show comment
Hide comment
@rcaputo

rcaputo Feb 23, 2015

Owner

I've reviewed the changes here. I haven't fetched or tried them yet. They all seem reasonable, but I have a qualm about bb8bb42 that I've mentioned on the change.

Owner

rcaputo commented Feb 23, 2015

I've reviewed the changes here. I haven't fetched or tried them yet. They all seem reasonable, but I have a qualm about bb8bb42 that I've mentioned on the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment