nagyation / Focus

Focus notifies you of a word or task periodically to keep focus
GNU General Public License v2.0
16 stars 1 forks source link

Use inline struct initialization to avoid extra methods. #1

Closed 9define closed 6 years ago

9define commented 6 years ago

As is, new_notification_data is used to create notification_data structs. However, seeing as it's only used in one place and can be replaced with inline struct initialization, I propose the following changes.

nagyation commented 6 years ago

First thanks for your contribution :), I was thinking of making add_new_notification do the initialization as well, to reduce the steps, so we can make new_notification_data private _new_notification_data and use it in add_new_notification instead.

9define commented 6 years ago

I think you're on the right track with integrating new_notification_data's function into add_new_notification, however I don't see the need for a separate method to do the struct initialization, as it's only used in one place right now. Inline intialization would work fine, or a compound literal could work if you need a pointer (which it seems may be the case).

nagyation commented 6 years ago

I think compound literal in add_new_notification is the best solution for now.

9define commented 6 years ago

Ok, I can put up a PR for that if you'd like.

nagyation commented 6 years ago

No need for a new PR, just commit to your branch it will make change here.

9define commented 6 years ago

Here are the changes. I moved the struct as it's not used anywhere else and doesn't need to be globally available. Code is easier to use/understand now, and I did a bit of testing to make sure there weren't any pointer issues.

nagyation commented 6 years ago

Looks great :+1: Thanks for your contribution.

nagyation commented 6 years ago

There's a big mistake I didn't notice, you dynamically allocated the node, and didn't allocate mem for data itself, I think data is better to be changed for static without the need for pointers, anyway I solved that in another branch and will solve merge soon

ben-brown-or commented 6 years ago

I agree that pointers should be eliminated if possible. Glad you're simplifying it.