Feature #3237: dividend yoc

[Marfir]

see issue #3237
I will add a dividend yield per cost for each security in the portfolio.

[Marfir]

@buchen
@OnkelDok
I need to extend the message files. But I can only German and English. What should I do with the other ones? Add english to any others?

[Marfir]

@Nirus2000
@buchen
I'm finish. All unit tests are green and I tested it manually in the UI with my test portfolio. All is working as I expected.
Please review it.
Did I need to add this new feature to an changelog? I found no changelog.md in the repository.

[chirlu]

Generally, and without having looked at the changes themselves, it would be good if you could clean up/rebase your branch.

• No merge commits.
• Corrections should not be separate commits, but be squashed into the original commit they are fixing.
• Personally, I would prefer to have separate commits for programming and for tests.

The changelog is the commit history. A summary is given on the forums with each release.

[chirlu]

I haven’t looked at the actual calculation.

[buchen]

Thanks @Marfir for the contribution. Generally it looks good (although personally I am very skeptical what YoC actually tells you as investor, I know there are many folks asking for this)

Before discussing the algorithm, let me make a couple remarks:

• Please create one branch per feature; it makes it easier to review. And rebase the branch instead of merging the master into your branch. This particular PR is based on a non-merged pull request that makes it even harder
• I like the builder pattern, but it is used only for tests anyway and there is already a builder pattern - so I think it does not make sense to introduce yet another builder

Then about the algorithm:

• Keep in mind that the "reporting period" can have arbitrary intervals - from a couple of days to multiple years. Inside the DividendCalculation, you will get only the payments that fall into this period. Therefore, you cannot make the assumption that you see all payments of the last year (the use of "now" - 365 days)
• If the user uses the "time machine", then the reporting periods are not calculated relative to "now" (it can be another date). If you want to have a sub-period, then it would be the "minus 1 year from the end of the interval" (and use minus 1 year to not have to think about leap years).
• The FIFO cost in the security performance record is relative to the selected reporting period. I know this is not intuitive, but maybe we should use the "absolute" FIFO costs instead?
• At the moment, you pick the first FIFO value as the basis for the calculation. Two thoughts: Shouldn't we pick the FIFO values at the beginning of the year? What happens if the user has additional purchases / sales in the past year? Wouldn't we want to adjust the divider of the calculation?
• If the YoC is always relative for one year, then maybe the best approach is to take the end the interval as the reference point in time (and then minus 1 year). Which means for me not plugging the calculation into the Security Record at all but creating a separate calculation (which I wanted to do for a long time anyway).

My proposal:

• Let's first agree on the algorithm (and ignore the comments on the more secondary code).
• From my understanding, the key points are: always 1 year? how to handle purchase/sales in the period?

[chirlu]

A bit of algorithm discussion had happened on the forums when @tquellenberg was working on this, starting from here and going on for about fifteen posts.

The FIFO cost in the security performance record is relative to the selected reporting period. I know this is not intuitive, but maybe we should use the "absolute" FIFO costs instead?

I think (as I already stated in that forums discussion) that we should stay consistent and honour the selected reporting period. Otherwise it will be doubly confusing. ;-)

[Marfir]

Generally, and without having looked at the changes themselves, it would be good if you could clean up/rebase your branch.

• No merge commits.
• Corrections should not be separate commits, but be squashed into the original commit they are fixing.
• Personally, I would prefer to have separate commits for programming and for tests.

The changelog is the commit history. A summary is given on the forums with each release.

If this are requirements then it should be part of the readme/contributors.md! It's my first contribution to this project. I don't know the workflow of this project. And I don't know how I can change all my past commits to fulfill your proposals.

[Marfir]

Thanks @Marfir for the contribution. Generally it looks good (although personally I am very skeptical what YoC actually tells you as investor, I know there are many folks asking for this)

Before discussing the algorithm, let me make a couple remarks:

• Please create one branch per feature; it makes it easier to review. And rebase the branch instead of merging the master into your branch. This particular PR is based on a non-merged pull request that makes it even harder
• I like the builder pattern, but it is used only for tests anyway and there is already a builder pattern - so I think it does not make sense to introduce yet another builder

Then about the algorithm:

• Keep in mind that the "reporting period" can have arbitrary intervals - from a couple of days to multiple years. Inside the DividendCalculation, you will get only the payments that fall into this period. Therefore, you cannot make the assumption that you see all payments of the last year (the use of "now" - 365 days)
• If the user uses the "time machine", then the reporting periods are not calculated relative to "now" (it can be another date). If you want to have a sub-period, then it would be the "minus 1 year from the end of the interval" (and use minus 1 year to not have to think about leap years).
• The FIFO cost in the security performance record is relative to the selected reporting period. I know this is not intuitive, but maybe we should use the "absolute" FIFO costs instead?
• At the moment, you pick the first FIFO value as the basis for the calculation. Two thoughts: Shouldn't we pick the FIFO values at the beginning of the year? What happens if the user has additional purchases / sales in the past year? Wouldn't we want to adjust the divider of the calculation?
• If the YoC is always relative for one year, then maybe the best approach is to take the end the interval as the reference point in time (and then minus 1 year). Which means for me not plugging the calculation into the Security Record at all but creating a separate calculation (which I wanted to do for a long time anyway).

My proposal:

• Let's first agree on the algorithm (and ignore the comments on the more secondary code).
• From my understanding, the key points are: always 1 year? how to handle purchase/sales in the period?

Thx for the review. :-)
Yes this little feature is in this branch. No more. I will create later another branch to do the rest (Div YoC over the complete portfolio).
The old builder was wrong implemented and I see no problem to using the new one, that I created, also in production code.

To the calculation:
I want to calculate the Div YoC for the last 12M. Why? To visible the value of a long period holding the dividend yield on current asset price did not matter. It's only interesting for new investors but not for the old ones. Using the last 365 days/12M/1Y get an more accurate calculation of current yield of the asset position then using an average dividend payment over the holding period. Companies increasing the dividend over time (the good companies) and this should be also visible in the Div YoC number.
I have also concerns to allow other periods in e.g. StatementsOfAssetViewer. If I understand the PP code correct then the original number (e.g. Div YoC in percent) was adjusted by something around in regard to the period. This is in the case of a dividend yield sometimes wrong (e.g. dividend increased or decreased over time) and would show something different what I expected.
Thx for the hint of the FIFO costs. Yes the magic around the calculations is not intuitive and can be dangerous. Please can you maybe document the magic in a readme that everybody can handle it correctly? This could be also reducing the discussions in the PRs if any contributors understand what he doing. ;-) I will check how I can fix it.

[Marfir]

A bit of algorithm discussion had happened on the forums when @tquellenberg was working on this, starting from here and going on for about fifteen posts.

The FIFO cost in the security performance record is relative to the selected reporting period. I know this is not intuitive, but maybe we should use the "absolute" FIFO costs instead?

I think (as I already stated in that forums discussion) that we should stay consistent and honour the selected reporting period. Otherwise it will be doubly confusing. ;-)

Yes there is also an older discussion here:
https://forum.portfolio-performance.info/t/personliche-dividendenrendite/335
as I mentioned in #3237
They are also thinking about using the last 365 days of dividend payment.

Using the selected period then you will get a wrong number in case of last dividend payments are changing big in compare to past payments. I can fix the tool tip to make it more clear that 1 Year was used?

[buchen]

Just FYI - I am on the road and will be able to look into this only next week :-(

[Marfir]

@Nirus2000
@chirlu
I don't know the workflow. Should I close the solved conversations or did it the creator?

[Nirus2000]

Hey @Marfir
Everything is fine so far. You just have to wait a bit until Andreas has time to look at the pull request again. Maybe you merge your individual commits in the meantime.
As soon as Andreas has found time and everything is ok from his side, then this will be transferred to the master.
Please read all our hints again and check the adjustments (e.g. typo).

Greetings
Alex

[Marfir]

Now I squashed all my commits into one. Hopefully I do no mistake. This was my first manual squash...

[chirlu]

Now I squashed all my commits into one.

From one extreme to the opposite … Each logical change (of which here is more than one) should have its own commit. – In any case you should rebase onto current master (which would remove the duplication of changes that happened there) before splitting the commit into logical steps again.

Furthermore, it doesn’t seem you addressed many of the other issues from the comments.

[chirlu]

OK, so I had hoped that adding a comment would cause Github to re-expand the conversations that you have, prematurely, marked as resolved, but unfortunately it doesn’t. I’ll need to repeat here:

AccountBuilder is technical debt. The Builder should be have the same name then the original class and it needs to be a part of the class that the builder construct. Then all people can find and use it. I will mark the AccoutBuilder as deprecated.

It doesn’t work like that. You may not like the current AccountBuilder and PortfolioBuilder, and others may not like it for different reasons, but not everyone can add his/her own, additional version. If you think you can improve them, do so, but then you need to update all current uses as well. And not in this pull request, please.

Further, I absolutely disagree with your opinion that “it needs to be a part of the class that the builder construct”. Stuff that is only used to make tests more readable should not clutter the main code.

No it's not the same! Percent2 do some magic trick with my number and calculates it "* 100". But my number is already a percent number and did not need an additional "* 100". (…) I put a deprecated annotation to this class.

Similar to the above: You are not alone in this project. You may prefer to store “+10%” as 10, and I may prefer to store it as 1.1, but the standard in this project is to use 0.1, and all the helper functions are aligned with that. It doesn’t do to just duplicate code and say “oh, let’s call the original stuff deprecated”.

Regarding Objects.requireNonNull(dateTime, "dateTime is a required field"); //$NON-NLS-1$ (multiple instances), where translations had been requested:
I would recommend leaving out the strings completely, as they are optional. They don’t give any information that isn’t already in the exception and the line number. Perhaps even leave out the tests altogether …

[Marfir]

OK, so I had hoped that adding a comment would cause Github to re-expand the conversations that you have, prematurely, marked as resolved, but unfortunately it doesn’t. I’ll need to repeat here:

Sorry. Because of this I ask for the workflow and got the answer "Everything is fine so far.".

It doesn’t work like that. You may not like the current AccountBuilder and PortfolioBuilder, and others may not like it for different reasons, but not everyone can add his/her own, additional version. If you think you can improve them, do so, but then you need to update all current uses as well. And not in this pull request, please.

Further, I absolutely disagree with your opinion that “it needs to be a part of the class that the builder construct”. Stuff that is only used to make tests more readable should not clutter the main code.

Yes there should be only one builder per class. If it's not allowed to using deprecated annotations in this project then it will be hard to improve the code. I'm sure nobody has the time to refactor the complete project. I create this builders because I see the existing tests that did not using it and I did not see the hidden builder classes. All this project specific things should be part of the contributors.md that every (new) developer can follow it from the beginning.

Similar to the above: You are not alone in this project. You may prefer to store “+10%” as 10, and I may prefer to store it as 1.1, but the standard in this project is to use 0.1, and all the helper functions are aligned with that. It doesn’t do to just duplicate code and say “oh, let’s call the original stuff deprecated”.

Same problem. Hidden magic in questionable classes that is nowhere documented.
I will change it to follow the magic.

Regarding Objects.requireNonNull(dateTime, "dateTime is a required field"); //$NON-NLS-1$ (multiple instances), where translations had been requested: I would recommend leaving out the strings completely, as they are optional. They don’t give any information that isn’t already in the exception and the line number. Perhaps even leave out the tests altogether …

If I should delete my builder classes then this issue does not matter.

[Marfir]

Each logical change (of which here is more than one) should have its own commit. – In any case you should rebase onto current master (which would remove the duplication of changes that happened there) before splitting the commit into logical steps again.

Is this your personal favorite to using git or is this a requirement of this project? If it's a project specific requirement then it should be part of the contributors.md.

[buchen]

Okay, let me first make a meta-comment: When in Rome, do as the Romans do. The code base is 11 years old, some patterns have no right or wrong but are determined by personal preferences (spaces vs. tabs - anyone?), not all technical debt can be fixed in one pull request. As I commented above, I do not plan to add yet another builder pattern or change the way I reflect percentages in the code. And all this discussion drew energy away from the actual point - the yield on cost. So I propose to focus on that one

[buchen]

FYI, here the draft implementation by @tquellenberg mentioned in this forum posting:

https://github.com/tquellenberg/portfolio/commits/dividend_yoc

[buchen]

Let me share my observations from reading the forum thread (which I hadn't done before - time constraints plus not being a dividend investor):

• Already the existing numbers are questionable - I do not think it makes sense to add YoC in a similar fashion
• The numbers definitely need an "explanation" - similar to the "calculation trail" tool tips in the performance calculation
• Dividend indicators seem to collide with arbitrary time intervals - to make sense, one has to look at the last year (or the 12 months before the end of the reporting period if one wants to choose different point in time)
• Today, only the dividend payments recorded are included. That has multiple problems: what if was only bought 1 months ago? That does not allow to compare numbers. How to handle currency changes.
• I still do not understand how the YoC algorithm should handle changes in the holdings during the last 12 months (if one sold 80% a month ago, then FIFO cost is very low compared to the paid dividends in the last 12 months). Is there a proposal how to calculate that?

I have these questions on my mind:

• How to handle time intervals? Always use the last 12 months from the end of the selected reporting interval? Or will this confuse users more than it helps? My gut feeling is to go for the 12 months because otherwise numbers are not meaningful anyway. And also this implementation would be confusing because if 10 years selected, it will also only use 1 year. We should rather think about a way to visualize this in the UI.
• Which dividend payments to use? My gut feeling is we should allow the user to manually maintain "dividend events" for the security. Then we take as a basis the maintained actual dividend payments + events from DivvyDiary + manually maintained events.

What do you think?

(plus: should we move the conceptual discussion back to the forum thread? And: I'll mark this as draft until we have a agreed understanding on the indicator)

[Marfir]

Let me share my observations from reading the forum thread (which I hadn't done before - time constraints plus not being a dividend investor):

• Already the existing numbers are questionable - I do not think it makes sense to add YoC in a similar fashion
• The numbers definitely need an "explanation" - similar to the "calculation trail" tool tips in the performance calculation
• Dividend indicators seem to collide with arbitrary time intervals - to make sense, one has to look at the last year (or the 12 months before the end of the reporting period if one wants to choose different point in time)
• Today, only the dividend payments recorded are included. That has multiple problems: what if was only bought 1 months ago? That does not allow to compare numbers. How to handle currency changes.
• I still do not understand how the YoC algorithm should handle changes in the holdings during the last 12 months (if one sold 80% a month ago, then FIFO cost is very low compared to the paid dividends in the last 12 months). Is there a proposal how to calculate that?

Good question. I did not consider this in my algorithm. I think the algorithm needs to detect if there are buys or sells in the 12M period (to increase the position by 80% would also give a wrong YoC number) and adjust the numbers.
after selling a 80% stake:

• previous dividend payments * 0.2 => using this as adjusted number for the YoC calculation
  after buying a 80% stake:
• previous dividend payments + new shares * current divi yield => using this as adjusted number for the YoC calculation
  For every divi payment we save the number of shares that was effected. So it should be work in that way. The algorithm would only become a little bit more complex. And of course I need to add more test cases for this.

I have these questions on my mind:

• How to handle time intervals? Always use the last 12 months from the end of the selected reporting interval? Or will this confuse users more than it helps? My gut feeling is to go for the 12 months because otherwise numbers are not meaningful anyway. And also this implementation would be confusing because if 10 years selected, it will also only use 1 year. We should rather think about a way to visualize this in the UI.

The interval strongly depends on how the user try to make money. If the user is a long term investor (stock picker, funds or ETF) then the last 12M would make sense. But if the user is a day trader or gambler then the complete feature make no sense for him or a day-based YoC is needed. I prefer the last 12M for myself and maybe the most of the other people too. We can start a survey in the forum about the "right"/expected period?

I already visualized it in the UI. Please take a look into the properties files that I extended. There should be no confusions about the period if the user can read!

• Which dividend payments to use? My gut feeling is we should allow the user to manually maintain "dividend events" for the security. Then we take as a basis the maintained actual dividend payments + events from DivvyDiary + manually maintained events.

I don't understand why the user should do manually this things? If I receive a dividend then I input the amount and the number of shares in the divi dialog. The received divi's of the last 12M are the base of the calculation. We have already what we need to calculate the correct YoC.

What do you think?

(plus: should we move the conceptual discussion back to the forum thread? And: I'll mark this as draft until we have a agreed understanding on the indicator)

Yes we could copy this ideas to the forum and as the others if it should work in that way or not. Also a survey about the used period would be interesting.