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

Trimmed text classes should accept multiple strings #539

Open
DFU398 opened this issue Jun 9, 2023 · 10 comments
Open

Trimmed text classes should accept multiple strings #539

DFU398 opened this issue Jun 9, 2023 · 10 comments
Labels

Comments

@DFU398
Copy link
Member

DFU398 commented Jun 9, 2023

Expected Behavior

Text.Trimmed, Text.TrimmedLeft and Text.TrimmedRight accept multiple strings to remove from the original string.

Update: As discussed in the comments below, it would be better to have new classes for this: Text.TrimmedFirstOf, Text.TrimmedLeftFirstOf and Text.TrimmedRightFirstOf. As the names suggest, they should accept multiple substrings to trim from the original text, but only trim the first matching substring, and only once.

Actual Behavior

These classes can only remove one string at a time, so when you want to remove slight variations of the same string, for example with different path separator chars, you have to nest multiple objects inside each other like this:

            new TrimmedLeft(
                new TrimmedLeft(machine.Id(), "machine/"),
                "machine\\"
            ).AsString()

Mention any other details that might be useful

Steps to reproduce the behavior

The log given by the failure.

@DFU398 DFU398 added the Feature label Jun 9, 2023
@Meerownymous
Copy link
Collaborator

To me, the usage with multiple classes is more clear: If I have a ctor with multiple strings, I have to remember which input string will be trimmed first. Is it the first or the last one? I could also get confused, whether every string I input into the ctor has the same weight: Is any of the strings removed, or only one of them? What if after removal of the first, another string which was put in is present? Will it also be removed?

With a composed object, it is obvious that the inner gets trimmed first, then the next layer and so on.

@DFU398
Copy link
Member Author

DFU398 commented Jun 29, 2023

I would expect all input strings to be removed regardless of their order:

new TrimmedLeft("abc", "a", "b").AsString() == "c"
new TrimmedLeft("bac", "a", "b").AsString() == "c"
new TrimmedLeft("ababc", "a", "b").AsString() == "c"

This way, if multiple of the input strings were present, there would be no order or weight to think about, so i don't think that would be unintuitive or unclear.
Also, in most cases where i've used these classes, only one of the input strings would be present at a time (like the example with the path separators in my original comment). In that case, there would be even less reason to think about the input strings possibly having a certain order or weight:

new TrimmedLeft("bc", "a", "b").AsString() == "c"
new TrimmedLeft("ac", "a", "b").AsString() == "c"

@Meerownymous
Copy link
Collaborator

Meerownymous commented Jul 2, 2023

What do you think about the unclear outcome of this example:

var result = new TrimmedRight(„abcde“, „bc“, „cd“, „de“, „ab“).AsText()?

@DFU398
Copy link
Member Author

DFU398 commented Jul 6, 2023

I didn't think about the input strings overlapping like that, that would be unintuitive, but i think that is rare enough that it doesn't outweigh the usefullness of this feature.

@Meerownymous
Copy link
Collaborator

Well, I get it, it sacrifices code readibility to save typing time. I personally prioritize readability.

@DFU398
Copy link
Member Author

DFU398 commented Jul 10, 2023

I don't think this sacrifices readability at all. You make it sound like i'm just being lazy. Of course convenience plays a role, but i also think sometimes more concise code is more readable. I'll get some other opinions and if they agree with you, i'll close the issue. I still think 1. this doesn't make the code less readable and 2. the cases in which this would make the results less predictable are edge cases.

@DFU398
Copy link
Member Author

DFU398 commented Jul 10, 2023

I just realized you seem to equate readability to predictable results, so for clarification:

  • I don't agree that this makes the code less readable. Maybe it even makes it more readable, i'm unsure about that.
  • But: I do agree that this makes the results less predictable in some cases. I just think those cases are not relevant to the normal use of this class.

So it doesn't sacrifice code readability to save typing time. If those were the factors at play here, i would also prioritize readability, i just don't think this is bad for readability.

If anything, it sacrifices the predictability of the results in some edge cases for typing time (and maybe even for improved readability, but you seem to have a different understanding of readability).

@DFU398
Copy link
Member Author

DFU398 commented Jul 10, 2023

Also, in those edge cases where the results would be hard to predict, you could still use multiple objects nested inside of each other. You wouldn't be forced to use this feature, it would just give you more options.

It's like with natural languages: There are many different ways to form a correct sentence. What matters is that people understand what you are trying to say. Sometimes you get your point across better if you go into more detail. Sometimes you get your point across better if you omit distracting details. Therefore, it's good to have options.

@MSE1188
Copy link
Contributor

MSE1188 commented Jul 10, 2023

I think we definitely need something with multiple strings, because we currently lack the functionality to trim a set of chars/strings.
"*/^**^hello there".Trim('*','/','^') would result in "hello there", but this would not work with nested object calls...
However, the previous examples showed, that this topic becomes super hard to overlook when working with strings instead of chars - suddenly the removal order can become important.

Maybe, we can just add a ctor that works with char-arrays?

For the problem described in the issue, you are using the candidates as alternatives, which is a different usecase. What do you think about a new class TrimmedOneOf for this, where the string candidates are or-handled?

@Meerownymous
Copy link
Collaborator

Meerownymous commented Jul 11, 2023

There is absolutely nothing wrong with being lazy. The EO concept of code being the documentation probably is motivated by being lazy or accepting people being lazy - it is simply saving time to write a documentation.

That being said, I realize I need to explain more why I think it sacrifices readability. When using a decorator pattern, it is clear to the reader which string is removed and in which order multiple strings would be removed. The overlapping confusion would not exist. That is based on the fact that the reader does only need to know the pattern itself, and that it works from inside out. The pattern is used throughout the whole library/coding style, so it does not require special knowledge about the object in question.

With the suggested change, this is lost. When I read the example, it raised the question in my head. To raise that question, what-would-happen-if, it does not matter how rare the cases are. It introduces a mental hurdle - for at least some readers - that previously was not there. To answer that question, the reader can either not care (thinking it will be rare, which might be true) or go one level deeper and look at the ctor documentation. Then the reader can remember that special thing about that object. Cost is brain memory.

So I still think, comfort is taken on one side (reader) and shifted to the other (writer). It can be valid to do so and I just wanted to raise the concern that came into my head. It should of course be up to the more frequent users of the library to reason about it. I just like to only do this shift in code that is more specialised than a basic library.

I like Max' suggestion, although I think "TrimmedFirstOf" better because otherwise it raises the same question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants