5
\$\begingroup\$

I have a file that is written DOS format and I need to replace some characters into others. I created a dictionary for that purposes, and now I have to read everyline from my stream and replace my values as I go.

I created this piece of code in C#, however, I highly doubt that this is the way to go, since I figure that C# dictionary class must have a simpler way to this probably common operation. Any feedback is greatly appreciated !

public Dictionary<char, string> Chars { get; set; } = new Dictionary<char, string>();

    // Called in my constructor
    public void CreateDictionnary()
    {
        Chars.Add('\u001b', " ");
        Chars.Add('\u0000', " ");
        // Multiple other characters that I have to replace
    }

public List<string> ReplaceSpecialCharacters(List<string> lines)
    {
        var result = new List<string>();

        foreach (var line in lines)
        {
            string newLine = string.Empty;

            foreach (var @char in line)
            {
                string replacedChar = @char.ToString();
                bool keyFound = false;

                foreach (var entry in Chars)
                {
                    if (keyFound)
                        continue;

                    if (@char == entry.Key)
                    {
                        replacedChar = entry.Value;
                        keyFound = true;
                    }
                }

                newLine += replacedChar;
            }

            result.Add(newLine);
        }

        return result;
    }
\$\endgroup\$
7
  • \$\begingroup\$ Welcome to code review, I suggest you look at this stackoverflow question and answer. It's really not clear that this question belongs on code review. \$\endgroup\$
    – pacmaninbw
    Jun 2, 2020 at 18:57
  • 1
    \$\begingroup\$ I understand how a dictionary works, I'm more interested if the way I did it is the correct way (my solution works, however a bit of code review would help :)) \$\endgroup\$ Jun 2, 2020 at 18:59
  • \$\begingroup\$ I understand how a dictionary works if Chars is your directory, the code presented suggests otherwise. \$\endgroup\$
    – greybeard
    Jun 2, 2020 at 20:20
  • 1
    \$\begingroup\$ Also without seeing the actual dictionary, it's hard to come up with a viable review. @greybeard is probably talking about looping through the entries in the dictionary instead of using indexing. Which does show an obvious lack of knowledge about how a dictionary works. \$\endgroup\$
    – user33306
    Jun 2, 2020 at 20:39
  • 1
    \$\begingroup\$ I added my dictionary definition. I used a dictionary because I thought that I want to change one value for another (key to find for value to replace) and I wanted to group it in the same function (where I defined my dict). I'm aware I could've done a simple class with 2 properties, but that's why I posted the question. I want to see what is the better way to replace my characters encountered in my list of strings. \$\endgroup\$ Jun 2, 2020 at 20:47

2 Answers 2

5
\$\begingroup\$

The main suggestion I'd make is to use Dictionary<char,char>. This allows to work with char collections and convert them to strings rather than continuously concatenating strings.

This also allows you to leverage LINQ to do the replacement and shorten your code tremendously.

It could look like this:

public Dictionary<char, char> Chars { get; set; } = new Dictionary<char, char>();

// Called in my constructor
public void CreateDictionnary()
{
    Chars.Add('\u001b', ' ');
    Chars.Add('\u0000', ' ');
    // Multiple other characters that I have to replace
}
public List<string> ReplaceSpecialCharacters(List<string> lines)
{
    for(int i = 0; i < lines.Count; ++i)
    {
        lines[i] = new string(lines[i].Select(x => Chars.ContainsKey(x) ? Chars[x] : x).ToArray());
    }
    return lines;
}

Incidentally this lowers the complexity from O(n²m),the total number of characters squared times the number of entries in the dictionary, to O(n²). Which since you're dealing with strings is probably about the best you can get. If you want it better, storing the data as char arrays instead of strings allows the replacement to be in-place instead of creating new strings. This could be done in O(n) complexity.

\$\endgroup\$
3
  • \$\begingroup\$ OK, I see thanks a lot ! \$\endgroup\$ Jun 3, 2020 at 12:23
  • 1
    \$\begingroup\$ You can simplify the Select-statement to: x => CharsT.TryGetValue(x, out char y) ? y : x which in fact saves you a ContainsKey()-call because the dictionary's indexer in fact calls ContainsKey() itself. \$\endgroup\$
    – user73941
    Jun 3, 2020 at 17:47
  • \$\begingroup\$ BtW: I think I would change the return type to IEnumerable<string> and then yield return the new string directly. IMO it is bad design if you both change the argument list in place and return that list as the result as if it is a new list. At least you should describe the behavior in a comment. \$\endgroup\$
    – user73941
    Jun 3, 2020 at 17:53
2
\$\begingroup\$

why not using Replace directly ? not sure why no one suggested though. It would be much faster and simpler.

public static IEnumerable<string> ReplaceSpecialCharacters(IEnumerable<string> lines)
{
    if(lines == null) { throw new ArgumentNullException(nameof(lines)); }

    StringBuilder sb = new StringBuilder();

    foreach(var line in lines)
    {   
        sb.Clear();
        sb.Append(line);

        foreach(var character in Chars)
        {
           sb.Replace(character.Key, character.Value);       
        }

        yield return sb.ToString();
    }
}

using Replace would replace the old character with the new one, if there is no match, it would return the original string. So, this would eliminate the need of checking the key, as the Replace will handle that for you.

Using StringBuilder would minimize the string allocation overhead, especially with large amount of strings. Also, it would give you more performance.

since we used yield return we also need to clear the stringbuilder then append the new line sb.Clear();

Finally, return the results as IEnumerable<string>, this would give you more compatibility with other collections. So, instead of just return or accept List<string> the method can accept any type of collection that implements IEnumerable<string> such as List<string> , Collection<string>, string[] and many others.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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