Andre,
first things first: it's a great job you did on date calculations and all the other stuff, so kudos to you. Secondly, I'm going to nag you just a tiny bit---feel free to come after me with a big clown-sized rubber hammer... In random order: a) Calendar::isLastBusinessDayOfMonth seems quite a lot of writing (although I'm sure I came up with names just as long in the past, so I ain't throwing the first stone here). How about Calendar::isEndOfMonth? b) As a matter of personal taste entirely, and therefore fully discardable on your part, I'm not enthusiastic about Date::Date(string). I'd rather keep the class clear from IO (Ditto for Period.) But I can live with it. (Aside: if the constructor was prompted by: string s1, s2; cin >> s1 >> s2; Date d1(s1,fmt); Date d2(s2,fmt); how about we implement operator>> and an IO manipulator instead, so that one can write: cin >> setdateformat(fmt); Date d1, d2; cin >> d1 >> d2; In the other cases, writing "Date d = DateParser::parse(s,fmt)" hurts me less, but that's only me :) end of aside) c) Maybe Period::operator== should be a partial function. There's a subset of the domain where it is well defined ("1 year == 12 months" is ok, and so is "3 days == 3 days" and "14 days == 2 weeks") but how about "31 days == 1 month"? Or "4 weeks == 1 month"? With the current implementation, the first is false, with the disconcerting result that if d = Date(22,7,2003), p1 != p2 but d.plus(p1) == d.plus(p2). The second is true, but for most dates, d.plus(p1) != d.plus(p2). How about throwing an exception if the == is undecidable? d) Date::ascending() is none other than std::less<Date>(). In the same way, Date::descending() is std::greater<Date>() and Date::less_equal(d) is std::bind2nd(std::less_equal<Date>(),d). If those classes are to be used with the STL, probably we better use their STL implementation :) Especially Date::less_equal might be more confusing than useful, since std::less_equal is a binary function. And in general, when one familiar with the STL reads std::less<Date>, he knows what that it. If he reads Date::ascending, he doesn't know. The same goes for TimeBasket::Entry. e) Scheduler. No problem per se---on the contrary, I'm grateful you tackled the thing---but it starts to have too many optional parameters. We could make it a bit prettier with the named parameter idiom, i.e., Scheduler s1 = MakeScheduler(mandatory parameters).withStubDate(d); Scheduler s2 = MakeScheduler(mandatory parameters).backwards(); Scheduler s3 = MakeScheduler(mandatory parameters).withStubDate(d).backwards(); f) CashFlowVectors---a lot of parameters now. Maybe we should pass them a Scheduler? The same goes for SimpleSwap. g) Swap::fairRate seems to work only if the first leg is the fixed one---I wouldn't count on that. Also, Swap is a generic swap class: one could be swapping two floating legs, so that fairRate() might not make sense at all. Thanks again, Luigi |
Andre's latest commit is the right occasion for me to urge everybody to consider adding a unit test when adding new features/classes. It is easier done than said, and it will guarantee the author that next releases will be compliant to the original test-specification. ciao -- Nando |
In reply to this post by Luigi Ballabio-2
At 4:27 PM +0200 7/22/03, Luigi Ballabio wrote:
>In random order: I forgot h) please oh please, 4-spaces tabs... :) Did we find out some way to make this comfortable for you? Maybe some emacs macro that makes the setting local for the buffer? Later, Luigi |
In reply to this post by Luigi Ballabio-2
Luigi,
> a) Calendar::isLastBusinessDayOfMonth seems quite a lot of writing. How about Calendar::isEndOfMonth? No problem, the original reason for the long name is to clarify that it's not just last day of month, but last _business_ day of month, but then that's really superfluous where a calendar is concerned! > b) As a matter of personal taste entirely, and therefore fully discardable on your part, > I'm not enthusiastic about Date::Date(string). I'd rather keep the class clear from IO (Ditto for Period.) I did this solely to make my life in SWIG easier, but I'll put my back into it and clean up. > But I can live with it. There are many things we can live with, that does not necessarily make them right! > how about we implement operator>> and an IO manipulator instead, so that one can write: > cin >> setdateformat(fmt); > Date d1, d2; > cin >> d1 >> d2; Will look into it. > c) Maybe Period::operator== should be a partial function. Not quite sure I understand what you mean? > There's a subset of the domain where it is well defined ("1 year == 12 months" is ok, and so > is "3 days == 3 days" and "14 days == 2 weeks") but how about "31 days == 1 month"? > Or "4 weeks == 1 month"? How about throwing an exception if the == is undecidable? Will do. > d) Date::ascending() is none other than std::less<Date>(). In the same way, > Date::descending() is std::greater<Date>() and Date::less_equal(d) is > std::bind2nd(std::less_equal<Date>(),d). Shows my lack of STL knowledge - alas, peer review is a way of learning! > e) Scheduler. No problem per se---on the contrary, I'm grateful you tackled > the thing---but it starts to have too many optional parameters. We could > make it a bit prettier with the named parameter idiom ... > f) CashFlowVectors---a lot of parameters now. Maybe we should pass them a > Scheduler? The same goes for SimpleSwap. OK, once again I learn... > g) Swap::fairRate seems to work only if the first leg is the fixed one---I > wouldn't count on that. Also, Swap is a generic swap class: one could be > swapping two floating legs, so that fairRate() might not make sense at all. Sorry, I realized this and should have removed it before I checked in. Thanks for your input Luigi |
In reply to this post by Luigi Ballabio-2
Nando,
> Andre's latest commit is the right occasion for me to urge > everybody to consider adding a unit test when adding new features/classes. > It is easier done than said, and it will guarantee the author that next > releases will be compliant to the original test-specification. Will do. Andre |
In reply to this post by Luigi Ballabio-2
Luigi,
Some questions on the so-called "named parameter paradigm" as per your comments on Scheduler. Scheduler s1 = MakeScheduler(mandatory parameters).withStubDate(d); Scheduler s2 = MakeScheduler(mandatory parameters).backwards(); Scheduler s3 = MakeScheduler(mandatory parameters).withStubDate(d).backwards(); Am I correct to assume "withStubDate" would be defined something like > Scheduler withStubDate(const Date& date) { > stubDate_ = date; > return *this; > } On the other hand this could erroneously be assumed to be a constructor, which of course it isn't! Am I missing something? Also, some flag would need to be set to force creation/recreation of the date vector? Thanx Andre |
In reply to this post by Luigi Ballabio-2
Hi there,
one idea here: implement < and > for the cases were it is well defined, and expand == to be "not larger and not smaller", eg. 4w<1m false 4w>1m false 4w==1m expands to ( !(4w<1m) && !(4w>1m) ) which is true 28d<1m false 31d>1m false 30d==1m expands to ( !(30d<1m) && !(30d>1m) ) which is true This could also be done in a Compare() function that returns either -1, 0 or 1 to make sure that absolutely noone uses the operators carelessly. Regards, Jens. c) Maybe Period::operator== should be a partial function. There's a subset of the domain where it is well defined ("1 year == 12 months" is ok, and so is "3 days == 3 days" and "14 days == 2 weeks") but how about "31 days == 1 month"? Or "4 weeks == 1 month"? With the current implementation, the first is false, with the disconcerting result that if d = Date(22,7,2003), p1 != p2 but d.plus(p1) == d.plus(p2). The second is true, but for most dates, d.plus(p1) != d.plus(p2). How about throwing an exception if the == is undecidable? |
In reply to this post by Andre Louw-2
At 12:51 PM 7/23/03 +0200, Andre Louw wrote:
>Some questions on the so-called "named parameter paradigm" as per your >comments on Scheduler. > >Scheduler s1 = MakeScheduler(mandatory parameters).withStubDate(d); >Scheduler s2 = MakeScheduler(mandatory parameters).backwards(); >Scheduler s3 = MakeScheduler(mandatory >parameters).withStubDate(d).backwards(); It would be something like: class MakeScheduler { private: ... (mandatory parameters) Date stub; bool fromEnd; ... (other optional parameters) public: MakeScheduler(mandatory params) : copy mandatory parameters, fromEnd(false), ... {} MakeScheduler& withStubDate(const Date& d) { stub = d; return *this; } MakeScheduler& backwards() { fromEnd = true; return *this; } MakeScheduler& forwards() { fromEnd = false; return *this; } // when we're ready... operator Scheduler() { return Scheduler(mandatory params, stub, fromEnd, ...); } }; i.e., no Scheduler is actually instantiated until the MakeScheduler thing is assigned to one. Bye, Luigi |
In reply to this post by Jens Thiel-2
At 12:37 PM 7/23/03 +0200, Jens Thiel wrote:
>one idea here: implement < and > for the cases were it is well defined, and >expand == to be "not larger and not smaller", eg. > > 4w<1m false > 4w>1m false > > 4w==1m expands to ( !(4w<1m) && !(4w>1m) ) which is true > > 28d<1m false > 31d>1m false > > 30d==1m expands to ( !(30d<1m) && !(30d>1m) ) which is true Hmm, I still have the problem that: Date d(23,7,2003); Period p1(30,Days); Period p2(4,Weeks); Period p3(1,Months); p1 == p3 => true d.plus(p1) == d.plus(p3) => false p2 == p3 => true d.plus(p2) == d.plus(p3) => false which is kind of surprising, and the additional problem that equality is not transitive: 30d == 1m => true 1m == 4w => true 30d == 4w => false Later, Luigi |
In reply to this post by Andre Louw-2
Hi Andre,
At 09:24 AM 7/23/03 +0200, Andre Louw wrote: > > b) As a matter of personal taste entirely, and therefore fully discardable >on your part, > > I'm not enthusiastic about Date::Date(string). I'd rather keep the class >clear from IO (Ditto for Period.) > >I did this solely to make my life in SWIG easier, but I'll put my back into >it and clean up. We can leave the additional constructor in SWIG---it just takes to write %extend Date { Date(const std::string& s, const std::string& fmt) { return new Date(DateParser::parse(s,fmt)); } } > > c) Maybe Period::operator== should be a partial function. > >Not quite sure I understand what you mean? I slipped into math talk--or maybe some other field? I meant "not defined upon the whole domain", since there are pairs of Periods for which it is ambiguous. Bye, Luigi |
In reply to this post by Luigi Ballabio-2
Luigi,
> CashFlowVectors---a lot of parameters now. Maybe we should pass them a Scheduler? > The same goes for SimpleSwap. Would you prefer me to keep the original constructors (before adding the optional parameters)? Andre |
At 03:25 PM 7/23/03 +0200, Andre Louw wrote:
> > CashFlowVectors---a lot of parameters now. Maybe we should pass them a >Scheduler? > > The same goes for SimpleSwap. > >Would you prefer me to keep the original constructors (before adding the >optional parameters)? Yes, I'd keep them for the time being. We can deprecate them in this release and remove them in the next one. Bye, Luigi |
In reply to this post by Luigi Ballabio-2
Luigi,
> class MakeScheduler { > ... > operator Scheduler() { > return Scheduler(mandatory params, stub, fromEnd, ...); > } > }; Is there a nice way of implementing the conversion operator in SWIG? Any examples in QuantLib? Thanx Andre |
At 11:35 AM 7/25/03 +0200, Andre Louw wrote:
> > class MakeScheduler { > > ... > > operator Scheduler() { > > return Scheduler(mandatory params, stub, fromEnd, ...); > > } > > }; > >Is there a nice way of implementing the conversion operator in SWIG? Any >examples in QuantLib? As for Python, there's no need to export MakeScheduler since it has keyword arguments---one can write s = Scheduler(..., fromEnd = 1) s = Scheduler(..., stub = Date(...)) For the other languages, I'll have to think about it... Later, Luigi |
Free forum by Nabble | Edit this page |