Helpers refactoring #1416

Merged
merged 3 commits into from Dec 4, 2013

Conversation

Projects
None yet
7 participants
@samdark
Member

samdark commented Dec 4, 2013

Moved file and path related methods from StringHelper to FileHelper, renamed StringHelper byte methods not to be misused as string methods

Moved file and path related methods from StringHelper to FileHelper, …
…renamed StringHelper byte methods not to be misused as string methods
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 4, 2013

Member

I think we should keep basename and dirname in StringHelper. They are about string manipulation, not file system manipulation. They are often used for strings not related with file paths. We have discussed about this before.

Member

qiangxue commented Dec 4, 2013

I think we should keep basename and dirname in StringHelper. They are about string manipulation, not file system manipulation. They are often used for strings not related with file paths. We have discussed about this before.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 4, 2013

Member

I've got at least three people complaining about FS-related stuff is in StringHelper and I feel bad about these methods being in this helper as well. Maybe another helper called "path" or something?

Member

samdark commented Dec 4, 2013

I've got at least three people complaining about FS-related stuff is in StringHelper and I feel bad about these methods being in this helper as well. Maybe another helper called "path" or something?

@@ -26,7 +26,7 @@ class BaseStringHelper
* @param string $string the string being measured for length
* @return integer the number of bytes in the given string.
*/
- public static function strlen($string)
+ public static function byteLen($string)

This comment has been minimized.

@qiangxue

qiangxue Dec 4, 2013

Member

Suggest naming it as byteLength(). We always use full words in names other than those inherited from existing PHP function names.

@qiangxue

qiangxue Dec 4, 2013

Member

Suggest naming it as byteLength(). We always use full words in names other than those inherited from existing PHP function names.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Dec 4, 2013

Member

The functions are provided really just for string manipulation. As you can see, they are used to extract part of the string from class names, etc., which has nothing to do with file systems. I don't think we should create a new class to hold them. The class FileHelper only deals with file systems.

Member

qiangxue commented Dec 4, 2013

The functions are provided really just for string manipulation. As you can see, they are used to extract part of the string from class names, etc., which has nothing to do with file systems. I don't think we should create a new class to hold them. The class FileHelper only deals with file systems.

@armpogart

This comment has been minimized.

Show comment
Hide comment
@armpogart

armpogart Dec 4, 2013

I often use basename for string manipulation that has nothing to do with files directly. So it's not very logical to place it in FileHelper. Though in native PHP it is in Filesystem functions, so the first place to search for that function is File related class.

I often use basename for string manipulation that has nothing to do with files directly. So it's not very logical to place it in FileHelper. Though in native PHP it is in Filesystem functions, so the first place to search for that function is File related class.

@egorpromo

This comment has been minimized.

Show comment
Hide comment
@egorpromo

egorpromo Dec 4, 2013

Contributor

I think basename() and dirname() should be in Yii class. They are special functions and could be both in StringHelper and in FileHelper at the same time.

Contributor

egorpromo commented Dec 4, 2013

I think basename() and dirname() should be in Yii class. They are special functions and could be both in StringHelper and in FileHelper at the same time.

qiangxue added a commit that referenced this pull request Dec 4, 2013

@qiangxue qiangxue merged commit 4df9b6c into master Dec 4, 2013

1 check passed

default The Travis CI build passed
Details

@qiangxue qiangxue deleted the helpers-renames branch Dec 4, 2013

@mikehaertl

This comment has been minimized.

Show comment
Hide comment
@mikehaertl

mikehaertl Apr 1, 2014

Contributor

I have to second this: I'd never have expected these methods in StringHelper. Even if it's only string manipulation: The manipulation performed is clearly filesystem related.

Following this logic, the dirname() method from PHP would also be in the string section of the PHP manual instead of the filesystem section, as it's also just performing some string manipulation. This is not the case.

Contributor

mikehaertl commented Apr 1, 2014

I have to second this: I'd never have expected these methods in StringHelper. Even if it's only string manipulation: The manipulation performed is clearly filesystem related.

Following this logic, the dirname() method from PHP would also be in the string section of the PHP manual instead of the filesystem section, as it's also just performing some string manipulation. This is not the case.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Apr 1, 2014

Member

StringHelper::dirname and StringHelper::basename are served for the namespace manipulation mainly.

Member

klimov-paul commented Apr 1, 2014

StringHelper::dirname and StringHelper::basename are served for the namespace manipulation mainly.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Apr 1, 2014

Member

@mikehaertl for file system you can use the PHP functions basename() and dirname(). The helper is there because the PHP functions do not work on namespaces.

Member

cebe commented Apr 1, 2014

@mikehaertl for file system you can use the PHP functions basename() and dirname(). The helper is there because the PHP functions do not work on namespaces.

@mikehaertl

This comment has been minimized.

Show comment
Hide comment
@mikehaertl

mikehaertl Apr 1, 2014

Contributor

I understand that. Still I don't really see these methods as generic string related methods. So StringHelper really feels wrong. A class like yii\helpers\Namespace would be way more clean IMO.

Contributor

mikehaertl commented Apr 1, 2014

I understand that. Still I don't really see these methods as generic string related methods. So StringHelper really feels wrong. A class like yii\helpers\Namespace would be way more clean IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment