I had to do some pretty heavy refactors because most of the bookmark code in the JS topics controller was centered around instances of Post JS models, but the topic level bookmark is not centered around a post. Some refactors were just for readability as well.
The code looks good, although I did notice that from a JS point of view we are only editing existing tests and not adding any new ones. Surely this introduces some new functionality that could use at least one new test?
I’m approving but I wouldn’t mind seeing a follow up PR with more tests!
Looks like _modifyBookmarkWithPost modifies the bookmark + its post, but in fact this function just looks for the post and then pass that post to _modifyBookmark and it’s that function actually modifies both the bookmark and the post. So, looks like the second function should have the name _modifyBookmarkWithPost too?
The names of these function don’t explain clearly what’s the difference between them.
Possibly, we can remove the first function, and get a post in the second function, in the beginning or when we need to (we’re going to need a post only in case it is a post level bookmark as I understand). Then we can delete the post argument from the second function signature (we omit this argument anyway when we call that function directly).
The word “cache” is confusing here. When I see this in Ember code, I think of cache in Ember Data, or probably some custom cache that we may have implemented ourselves. In fact, this function just remove bookmarks from the topic model. Maybe it can be renamed to removeBookmarkFromModel or removeBookmarkFromTopicModel. Even better, looks like this function belongs to the topic model, we call that model in every line, moving it to the model will simplify the function, and it’s name:
The first part of this comment doesn’t say anything that cannot be discovered by reading code:
* Used by toggleBookmark() when called without a post from the
* topic-footer-buttons bookmark button.
If someone adds a new call to this function or change something around of existing call, he\she should go and update this comment or this comment will get outdated.
Also, the second part:
* Has different behaviour
* based on the number of bookmarks in the topic and also whether
* there is a topic-level bookmark for the OP.
is trying to explain what the function does. And again it doesn’t say anything that can’t be found in code, and now we should care about keeping this comment updated.
In general, comments is a bad practice. Not only because they easily get outdated. The more important problem is that they lead to worse code. I understand that you’re trying to make this code simpler to reason about in the future. But it’s better to do it not by adding comments. It’s always better to put more efforts into:
choosing a good explicit descriptive name for the function and its parameters
improving implementation of the function, so it is self-explainable
If you feel like you need to add a comment somewhere, probably you feel like that code is going to be hard to understand without reading history or running debugger. It’s better to improve the code itself, then.
By the way, I, personally, think the name and implementation of this function are good, no need to add the comment.
(Speaking of comments in general, it’s good to add comments to API’s (users not going to read implementation at all, they will be using documentation) or complicated algorithms and fancy optimizations (such places are hard to understand by their nature)).
I understand that it was changed during refactoring, but I fell like we still need to have _tooglePostBookmark function. It is one of top-level operations that we do with bookmarks on topics (we also have _toggleTopicLevelBookmark, deleteBookmark etc.). It’s good to have in code the same actions that we have in mind when we reason about user interactions. We clearly have the togglePostBookmark action, but it disappeared in code in this PR.
@AndrewPrigorshnev thanks for your in-depth review, I will make these changes today. I am hearing you with the confusion around _modifyBookmarkWithPost and _modifyBookmark, I am going to think of a better way to name this functionality and refactor a bit so things are clearer.