Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strings - added method cut() #20

Closed
wants to merge 5 commits into from
Closed

Conversation

icaine
Copy link

@icaine icaine commented Jun 9, 2014

Purpose of this method is to simplify usage when functions like strstr() and strrchr() are need.

@Majkl578
Copy link
Contributor

Majkl578 commented Jun 9, 2014

👎 Toooooo magic.

@icaine
Copy link
Author

icaine commented Jun 9, 2014

What?

@@ -130,6 +130,43 @@ public static function substring($s, $start, $length = NULL)


/**
* Returns cutted string.
Copy link

Choose a reason for hiding this comment

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

@rostenkowski
Copy link
Contributor

I agree with @Majkl578. The name is not much descriptive and the functionality is too complicated.

  • method name does not describe what function does
  • too much method arguments
  • returns different types
  • code itself uses "tooooo magic" constructs

@vojtech-dobes
Copy link

What I like about Nette is that almost every feature is powerful yet very simple to grasp. I am afraid 99% of users will never learn (I mean remember how to use it) such method. 👎

@milo
Copy link
Member

milo commented Jun 10, 2014

@icaine One number and two booleans are really much. E.g. $includeNeedle can be written mostly like:

$sub = Some::cut($path, '/') . '/';

and it is obvious. TRUE or FALSE as 5th parameter is just magic.

If I understand well, you need it to work with filesystem path mostly. Would not be FileSystem class better place? Inside it, method name can be more accurate.

If so, do you really need more than pathinfo() with realpath() provide?

@icaine
Copy link
Author

icaine commented Jun 10, 2014

The name may not be the best and if you give me any better i dont see a problem.
Anyway it cuts a string into two pieces by a needle, which can be a whole word not only a single char, and returns one of pieces, other parameters just say how to do it (at which occurence, which part is needed and if the needle itself should be included). Its useful for working with paths, for stripping pre/suffixes, etc.

The reason why i made this method is that i always was confused with strstr() and strrch() and the fact they dont provide same options (e.g. strrch doesnt have before_needle).

@rostenkowski
Copy link
Contributor

I understand u. What with the Tooooo many arguments problem?

@icaine
Copy link
Author

icaine commented Jun 10, 2014

What with it? Lesser arguments means lesser functionality. If all were mandatory i would understand your cooooomplains, but they are not..

Anyway, the community refused it.. closed..

@icaine icaine closed this Jun 10, 2014
@fprochazka
Copy link
Contributor

@icaine you give up too easily :)

@Majkl578
Copy link
Contributor

Such PRs are about discussion, not about getting anger after first wave of disagreement. ;)

@dg
Copy link
Member

dg commented Jun 10, 2014

@icaine Problem with too many arguments is that they are not understandable. For example, what do you think this is doing? $str = pad($str, TRUE, 3, FALSE)

@MartinMystikJonas
Copy link

What about split it to multiple methods with better names. Something like:

  • Strings::before = part before needle, without needle
  • Strings::until = part bafore needle, with needle
  • Strings::past = part after needle, with needle
  • Strings::after = part after needle, without needle

Internaly it still can call same function with many parameters parameters but it will be private.

Example:

echo Strings::before("some long string", "long"); // -> "some"
echo Strings::until("some long string", "long"); // -> "some long"
echo Strings::past("some long string", "long"); // -> "long string"
echo Strings::after("some long string", "long"); // -> "string"

@rostenkowski
Copy link
Contributor

👍

Good idea...

Maybe the before and after methods could take second argument boolean $inclusive = FALSE or boolean $exclusive = TRUE to prevent API bloating and avoid until and past which are not much clear names.

@MartinMystikJonas
Copy link

I personally don't like boolean parameters. From my POV more methods with clear names are better.

@icaine
Copy link
Author

icaine commented Jun 11, 2014

@MartinMystikJonas i like it...

So

before ($haystack, $needle, $nth = 1)
after ($haystack, $needle, $nth = 1)
// and maybe these
until ($haystack, $needle, $nth = 1)
past ($haystack, $needle, $nth = 1)

// OR
before ($haystack, $needle, $nth = 1, $includeNeedle = false)
after ($haystack, $needle, $nth = 1, $includeNeedle = false)

@icaine icaine reopened this Jun 11, 2014
@dg
Copy link
Member

dg commented Jun 23, 2014

Is there any use-case, where $nth is greater than 1?

@MartinMystikJonas
Copy link

Well I can imagine something like:

after("/some/long/absolute/path/to/file", "/", 3); // -> "path/to/file"

@JanTvrdik
Copy link
Contributor

@MartinMystikJonas And how would you know that you should always skip 3 directories?

@MartinMystikJonas
Copy link

On this was not best example. Let my try again :-)

Lets say we have some data storage where each user has his own folder for personal data and you want to show him only part of path he is interested to.

after("datastorage/user123/myfolder/myfile", "/", 3); // -> "myfolder/myfile"

Or you can compute $nth parameter somehow.

@icaine
Copy link
Author

icaine commented Jun 24, 2014

@JanTvrdik it's similar to using dirname(dirname(dirname(...))). With this method you could type before($filepath, '/', -3);

@dg
Copy link
Member

dg commented Jun 24, 2014

And in the case that there are only two slashes, before($filepath, '/', -3) returns whole string? Hmmm…

I think that more expectable is to return FALSE. You can always use before($filepath, '/', -3) ?: $filepath.

@icaine
Copy link
Author

icaine commented Jun 24, 2014

It returns false.

@foxycode
Copy link
Contributor

I'd like this:

after('some.long.file.name.jpg', '.', -1); // jpg

@dg dg force-pushed the master branch 2 times, most recently from 5617d88 to 209844d Compare May 21, 2015 11:51
@dg dg closed this in 8c57c61 Jun 15, 2015
milo pushed a commit to milo/nette-utils that referenced this pull request Feb 25, 2021
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.