nadrad / h-m-m

Hackers Mind Map
GNU General Public License v3.0
1.89k stars 53 forks source link

PHP DocBlock for header and functions #31

Closed llagerlof closed 2 years ago

llagerlof commented 2 years ago

This is a suggested PR. I wrote a header and a DocBlock for each function. No code logic or formatting was changed. This is just the common comment format for PHP code.

And now a documentation for the code can be generated using the phpDocumentor tool. All tests passed, however, isn't mandatory to use this tool to generate docs. It's up to the mantainer.

This is manual work, but I made it. This make the code easier to other devs read.

Some things to consider in case this pull request is accepted:

I read all the functions declarations and set the types manually. Should be reviewed. An example:

/**
 * Magic readline!
 *
 * @param array $mm
 * @param string $title
 * 
 * @return string|bool
 */
function magic_readline(&$mm, $title)
...

About the code above, I am supposing:

  1. $mm is an array. If $mm can be an array and other thing, "other thing" should be specified. e.g: @param array|null $mm
  2. Reading the function I saw it can return the types string or bool. If the function can return nothing (void), should be specified. e.g. * @return string|bool|void

These DocBlocks are good as-is but can be improved adding a comment after each parameter. An example:

/**
 * Magic readline!
 *
 * @param array $mm The mind map tree
 * @param string $title Some text
 * 
 * @return string|bool This is the returned value
 */
function magic_readline(&$mm, $title)
...

Well, that's it.

nadrad commented 2 years ago

Thanks for the pull request and the explanation. I spent a few hours reading about this concept and thinking about its potential role in this application. However, I'm afraid I'm not convinced about it. It adds a lot of comments to the code, and most of its data is already available (implicitly or explicitly), which is also a maintenance burden for the future.

I think most of its benefits can be achieved by a more careful naming of the functions and variables (e.g., "root_id" instead of just "root"), and we also have the option to add type hints to make the functions even clearer.

So, if you don't mind, I won't merge it, but I'll use this inspiration to refactor the code and make it clearer for others who may want to contribute to the project. I'll also add the top header, so that the license information won't be lost outside the repo.

P.S. the "{{{"s and "}}}"s that you've removed are my fold marks in vim that I really depend on when working on this single-file application :)

llagerlof commented 2 years ago

It's ok. 😅