5
\$\begingroup\$

The assignment is to mask (or rather unmask) certain indexes from a string. Both the input string and the demasker are strings. An example shows how it's supposed to work.

Input: "abcdefgh"
Mask: "2-4,6"
Output: "bcdf"

The index is one-based and we need not to worry about malicious users' entries. The entries are always separated by a comma and the limits (if not atomary) by a dash.

My current solution is this.

string input = "abcdefghijklmnopqrstuvwxyz";
string mask = "3-5,8,9-12,13,14,18-26";

Dictionary<int, int> intervals = mask.Split(',').ToDictionary(
  element => Convert.ToInt32(element.Split('-').First()),
  element => Convert.ToInt32(element.Split('-').Last()));
string substring = "";
foreach (KeyValuePair<int, int> entry in intervals)
  substring += input.Substring(entry.Key - 1, entry.Value - entry.Key + 1);

I'm unhappy with the way it looks right now, because I'd like it to be demasking using other method than foreach. Given that the strings are relatively short, there's no option of deploying a third party library.

Can the above be made smoother? All creative critique is welcome.

\$\endgroup\$
3
\$\begingroup\$

If usage of the foreach is an issue, you could replace it with the IEnumerable<T>.Aggregate method call:

string substring = intervals.Aggregate(String.Empty, 
    (current, entry) => 
        current + input.Substring(entry.Key - 1, entry.Value - entry.Key + 1));

If performance is a root, you could try to use the StringBuilder class:

StringBuilder sBuilder = new StringBuilder(input.Length);
foreach (var entry in intervals)
    sBuilder.Append(input, entry.Key - 1, entry.Value - entry.Key + 1);
string substring = sBuilder.ToString();

And there is no reason to use the Dictionary<int, int>. It could be easily replaced with the List<Tuple<int, int>> or with the IEnumerable<Tuple<int, int>> (but in the last case there is no ForEach extension method):

List<Tuple<int, int>> intervals = mask.Split(',').Select(part =>
    {
        var range = part.Split('-').Select(val => Convert.ToInt32(val));
        return new Tuple<int, int>(range.First(), range.Last());
    }).ToList();

StringBuilder sBuilder = new StringBuilder(input.Length);
intervals.ForEach(entry => sBuilder.Append(input, entry.Item1 - 1, entry.Item2 - entry.Item1 + 1));
string substring = sBuilder.ToString();
\$\endgroup\$
4
\$\begingroup\$

Do not use Dictionary when order matters.

Your code assumes that iterating over Dictionary will yield elements in the order in which they were added, but that's not guaranteed at all (emphasis mine):

For purposes of enumeration, each item in the dictionary is treated as a KeyValuePair<TKey, TValue> structure representing a value and its key. The order in which the items are returned is undefined.

And even if this wasn't an issue, the intervals logically are not keys and values, so that's another reason not to use dictionary.

In simple cases like this, consider using anonymous type:

var intervals = mask.Split(',').Select(
  element => new 
  {
      from = Convert.ToInt32(element.Split('-').First()),
      to = Convert.ToInt32(element.Split('-').Last())
  });
\$\endgroup\$
5
  • \$\begingroup\$ Interesting point. Is it relatively easy to describe a scenario where the operation suggested would produce a different order of elements? \$\endgroup\$ – Konrad Viltersten Oct 29 '14 at 12:51
  • 2
    \$\begingroup\$ @KonradViltersten With the current implementation of Dictionary, I belive that requires removing items from the dictionary. But saying "I don't remove anything, so I should be fine" is relying on undocumented implementation details, and you shouldn't do that. \$\endgroup\$ – svick Oct 29 '14 at 12:55
  • \$\begingroup\$ @KonradViltersten For example var dict = new Dictionary<int, int>(); dict.Add(1, 2); dict.Add(3, 4); dict.Remove(1); dict.Add(5, 6); will return (5,6); (3,4). \$\endgroup\$ – svick Oct 29 '14 at 12:58
  • \$\begingroup\$ Oh, right. Let me rephrase the question - is there a relatively simple example that shows how a newly initialized dictionary that's only written to (addition, by addition, without any deletions, moves nor sortings) still can get an unintended order of elements? \$\endgroup\$ – Konrad Viltersten Oct 29 '14 at 14:10
  • 3
    \$\begingroup\$ @KonradViltersten: No, there most likely isn't an example of that. However, there are many variants of the CLR, and because this is undefined, you cannot know that it will work the same way on all of them. Even if you stick to just the one, relying on implementation details is a bad idea, because they could change in the next version. Most likely, you'll be fine. But it's a very bad pattern to get into. \$\endgroup\$ – Magus Oct 29 '14 at 14:44
1
\$\begingroup\$

Using a dictionary is effective and provides you with the desired result. A Dictionary does, however provide a little overhead.

More inefficient is building a string in a loop using +=. Use a StringBuilder when concatenating strings in a loop.

To avoid using a dictionary just split the array on ',' and iterate over that array. The following code differs from your functionality in that given a mask like "1-3-4" your algorithm will result in the range of 1-4 and mine will ignore it entirely. In your description you indicated that the user input is assumed correct so I will assume this is acceptable behavior :).

StringBuilder substring = new StringBuilder();
string[] intervals = mask.Split(',');
string[] range;

foreach (var interval in intervals) {
  range = interval.Split('-');
  if (range.Length == 1) {
    var index = int.Parse(range[0]) - 1;
    substring.Append(input[index]);
  }
  else if (range.Length == 2) {
    var start = int.Parse(range[0]) - 1;
    var length = int.Parse(range[1]) - start;
    substring.Append(input.Substring(start, length));
  }
}
\$\endgroup\$
1
\$\begingroup\$

One alternative to using foreach is string.Join:

private static string Mask(string input, string mask)
{
    var intervals = ???
    return string.Join(string.Empty, intervals);
}

As has already been pointed out, a Dictionary is not the best match for this situation. Instead we can just enumerate the substrings that we want

private static string Mask(string input, string mask)
{
    var intervals = from interval in mask.Split(',')
                    let split = interval.Split('-').Select(int.Parse).ToArray()
                    select input.Substring(split.First() - 1, split.Last() - split.First() + 1);
    return string.Join(string.Empty, intervals);
}

Or the slightly more verbose but (presumably) more efficient

private static string Mask(string input, string mask)
{
    var intervals = from interval in mask.Split(',')
                    let split = interval.Split('-')
                    let first = int.Parse(split.First())
                    let last = int.Parse(split.Last())
                    select input.Substring(first - 1, last - first + 1);
    return string.Join(string.Empty, intervals);
}
\$\endgroup\$
0
\$\begingroup\$

Strange implementation

I only see a few strange implementation issues:

  • in the ToDictionary() call you are splitting the string twice times
  • string concatenation in the foreach loop

Refactoring to class

With object oriented view we can build a class to solve this problem what kind of problems we have to deal with:

  • parse the mask
  • apply it to the input string

So create a parser class!

public class StringMasker
{
    private readonly IEnumerable<KeyValuePair<int, int>> _parsedMask;

    public StringMasker(string mask)
    {
        if (mask == null) throw new ArgumentNullException("mask");

        _parsedMask = ParseMask(mask);
    }

    public virtual string ApplyTo(string input)
    {
        if (input == null) throw new ArgumentNullException("input");

        return string.Concat(_parsedMask.Select(entry => GetSubstring(input, entry)));
    }

    protected virtual string GetSubstring(string input, KeyValuePair<int, int> entry)
    {
        return input.Substring(entry.Key - 1, entry.Value - entry.Key + 1);
    }

    protected virtual IEnumerable<KeyValuePair<int, int>> ParseMask(string mask)
    {
        foreach (var splitted in mask.Split(','))
        {
            var indexes = splitted.Split('-');

            var start = int.Parse(indexes[0]);
            var end = indexes.Length == 1 ? start : int.Parse(indexes[1]);

            yield return new KeyValuePair<int, int>(start, end);
        }
    }
}

Yes the code has grown a little bit but i think everything is in place now lets see what i did. I have splitted the parse and the apply operations into different method and i have elminated the double split (separator '-') and removed the foreach loop and changed it to a Linq call with string.Concat(IEnumerable).

You can put additional error checks into the method i have only added the null check in the input methods (constructor).

The KeyValuePair is not the best to hold two indexes togatcher becouse it stands for a complete different stuff in the framework as it's name is telling us.

The class bejavior can be overridden in derived classes because of the virtual methods and the separated reponsibilities.

Test

I have created a unit test too to check the results but i have only implemented one trivial test.

[TestClass]
public class NormalBehaviorTest
{
    private const string Input = "abcdefghijklmnopqrstuvwxyz";
    private const string Mask = "3-5,8,9-12,13,14,18-26";
    private const string Result = "cdehijklmnrstuvwxyz";

    [TestMethod]
    public void TestFixedInputAgainstDesiredResultWithCorrectPattern()
    {
        //Arrange
        var masker = new StringMasker(Mask);

        //Act
        var result = masker.ApplyTo(Input);

        //Assert
        Assert.AreEqual(Result, result);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Could you explain why do you think writing 5 times more LOC is a good idea here? What do you gain? Certainly not readability. \$\endgroup\$ – svick Oct 29 '14 at 12:40
  • \$\begingroup\$ It depends what are you trying to achieve. If you are applying one mask to several string my solution would be the better approach. But you gain also readability (yes i can understand every other solution) but put the solutions above beside of mine. Separated understandable logic is one thing that matters the most in every application. \$\endgroup\$ – Peter Kiss Oct 29 '14 at 13:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.