Skip to content

Conversation

MBoretto
Copy link

Hello! This is a feature/bugfix that I would like to push in the next version.
I build this on top of your entities refactoring to test your work. If you prefer I can create another pr that point to longman/develop. Let me know

Copy link
Owner

@noplanman noplanman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MBoretto Nice work!
I noted a few suggestions above.

class User extends Entity
{
/**
* Try mention
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great if the description were more... descriptive 😉

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

if ($this->username === null) {
if ($this->last_name !== null) {
return $this->first_name . ' ' . $this->last_name;
if (isset($this->username)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using $this->whatever, the way it works no is to use $this->getProperty('whatever'), as that checks if the value exists / is null.

To make it easier, just set $username = $this->getProperty('username');, then use the $username variable. Same applies for other properties.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense

$string = str_replace('[', '\[', $string);
$string = str_replace('`', '\`', $string);
$string = str_replace('*', '\*', $string);
return str_replace('_', '\_', $string);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of calling str_replace multiple times, they can all be combined into 1 call:

return str_replace(
    ['[', '`', '*', '_'],
    ['\[', '\`', '\*', '\_'],
    $string
);

If you want to play with regex, here a 1-liner 😇 :

return preg_replace('/([\[`\*_])/', '\\\\$1', $string);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the same but i am more confident with str replace

*
* @return string
*/
public function prependAt($string)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it makes sense to have an own method for this, as it's being used just 2x and is just prepending a character.

Could you explain why you opted for a separate method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to be more clear but can be removed of course no big deal

*
* @return string
*/
public function stripMarkDown($string)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be static, as it doesn't access any class members. Would also save you from creating a User object in the tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to not create a static class just for this, al least not now. In this pr i keep everything in this file but I think that this method can be put on top, maybe in the entity class in order to share it with all the entities. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea @ putting it in the entities class!

@noplanman noplanman merged commit c35a03f into noplanman:refactor_entities Sep 29, 2016
@MBoretto MBoretto deleted the md_mention branch December 30, 2018 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants