3
\$\begingroup\$

The task:

You will be given several lines of messages containing data. You have to check for the validity of the lines. A valid line should be in the format: ""s:{sender};r:{receiver};m--"{message}"

  • sender – could contain any ASCII character except for ;
  • receiver – could contain any ASCII character except for ;
  • message – should contain only letters and spaces

In each valid message there is a hidden size of data transfer. The size of the data transfer is calculated by the sum of all digits in the names of the sender and receiver. After each valid message print a line in the format: "{senderName} says "{currentMessage}" to {recieverName}". The printed names should contain only letters and spaces.

Example: sender "P@e$5sh#o Go^4sh5ov" is valid and matches, but when printing his name, we only print "Pesho Goshov". At the end print a line in the format "Total data transferred: {totalData}MB".

Example 1

=== Input ===
3
s:P5%es4#h@o;r:G3#o!!s2h#2o;m--"Attack"
s:G3er%6g43i;r:Kak€$in2% re3p5ab3lic%an;m--"I can sing"
s:BABAr:Ali;m-No cave for you

=== Output ===
Pesho says "Attack" to Gosho
Gergi says "I can sing" to Kakin repablican
Total data transferred: 45MB

Example 2

=== Input ===
5
s:B^%4i35454l#$l;r:Mo5l#$34l%y;m--"Run"
s:Ray;r:To^^5m;m--"Hidden Message"
bla;r:1234a;m--Hello
s:M#$%$#^6767687654545e;r:Yo54$#@#u5;m--"$$$"
s:M#$@545e;r:You241$@#23;m"Hello"

=== Output ===
Bill says "Run" to Molly
Ray says "Hidden Message" to Tom
Total data transferred: 42MB

My Solution

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
class Program
{
    class MessageInfo
    {
        public bool isOk = true;
        public int messageSize;
        string sender;
        string reciver;
        public string Sender
        {
            get
            {
                return Regex.Replace(sender, @"\s+", " ");
            }
            set
            {
                StringBuilder temp = new StringBuilder();
                foreach (var c in value)
                {
                    if (Char.IsDigit(c))
                    {
                        messageSize += (c - '0');
                    }
                    if (Char.IsLetter(c) || Char.IsWhiteSpace(c))
                    {
                        temp.Append(c);
                    }
                }
                sender = temp.ToString();
            }
        }
        public string Reciver
        {
            get
            {
                return Regex.Replace(reciver, @"\s+", " ");
            }
            set
            {
                StringBuilder temp = new StringBuilder();
                foreach (var c in value)
                {
                    if (Char.IsDigit(c))
                    {
                        messageSize += (c - '0');
                    }
                    if (Char.IsLetter(c) || Char.IsWhiteSpace(c))
                    {
                        temp.Append(c);
                    }
                }
                reciver = temp.ToString();
            }
        }
        public string Message { get; set; }

    }
    static void Main()
    {
        int n = int.Parse(Console.ReadLine());
        int totalMessageSize = 0;
        MessageInfo[] chatLog = new MessageInfo[n];
        for (int i = 0; i < n; i++)
        {
            string line = Console.ReadLine();
            chatLog[i] = CoreAction(line);
            if (chatLog[i].isOk)
            {
                totalMessageSize += chatLog[i].messageSize;
            }
        }
        for (int i = 0; i < chatLog.Length; i++)
        {
            if (chatLog[i].isOk)
            {
                Console.WriteLine("{0} says \"{1}\" to {2}", chatLog[i].Sender, chatLog[i].Message, chatLog[i].Reciver);
            }
        }
        Console.WriteLine("Total data transferred: {0}MB",totalMessageSize);
    }

    private static MessageInfo CoreAction(string line1)
    {
        MessageInfo a = new MessageInfo();
        int indexer = 0;
        StringBuilder sender = new StringBuilder();
        StringBuilder reciver = new StringBuilder();
        StringBuilder message = new StringBuilder();
        Regex matchLine = new Regex("^s:[^;]+;r:[^;]+;m--\"[\\w\\s]+\"$");
        if (matchLine.IsMatch(line1))
        {
            lineSplitter(line1, ref indexer, ref sender, ref reciver, ref message);
        }
        else
        {
            a.isOk = false;
        }
        a.Sender = sender.ToString();
        a.Reciver = reciver.ToString();
        a.Message = message.ToString();
        return a;
    }

    private static void lineSplitter(string line1, ref int indexer, ref StringBuilder sender, ref StringBuilder reciver, ref StringBuilder message)
    {
        while (true)
        {
            if (line1[indexer + 1] == ':')
            {
                if (line1[indexer] == 's')
                {
                    sender = stringFill(line1, ref indexer, 2, ';');
                }
                if (line1[indexer] == 'r')
                {
                    reciver = stringFill(line1, ref indexer, 2, ';');
                }
            }
            if (line1[indexer] == 'm' && line1[indexer + 1] == '-' &&
                line1[indexer + 2] == '-' && line1[indexer + 3] == '"')
            {
                message = stringFill(line1, ref indexer, 4, '"');
            }
            if (indexer >= line1.Length - 1)
            {
                break;
            }
            indexer++;
        }
    }

    private static StringBuilder stringFill(string line1, ref int indexer, int offSet, char charBreak)
    {
        StringBuilder temp = new StringBuilder();
        indexer += offSet;
        while (line1[indexer] != charBreak)
        {
            temp.Append(line1[indexer]);
            indexer++;
        }
        return temp;
    }
}
\$\endgroup\$
  • \$\begingroup\$ This looks like it was copied directly from an exam published by a school in Bulgaria. Is that where you found it, @Cecobent? \$\endgroup\$ – benj2240 Dec 14 '18 at 23:03
  • \$\begingroup\$ Yes,all the credit for the task condition goes to "SoftUni", but the solution is solely mine. \$\endgroup\$ – Mountain Gold Dec 15 '18 at 23:17
3
\$\begingroup\$

I can tell you're concerned about performance (and not just because of the tag on the question). This is a good impulse, because it helps you avoid selecting terribly inefficient algorithms. But it can also hurt, when it leads you to micromanage details that would be better left to (for example) a regular expression engine.

You have some good habits. I love to see foreach loops. I'm very glad that you used a StringBuilder rather than just the + operator. Putting sanitization logic in the public setter for a property so that you can maintain a "clean" private backing field is a great idea.

Passing ref parameters generally makes code more difficult to maintain. In fact, I'll go farther than that: Anything that increases the scope of a variable usually makes code more difficult to maintain. Imagine I'm reading your lineSplitter function, I've never seen it before, and I'm in a big hurry because there's a bug in the company's legacy chat log parsing program that's costing us thousands of dollars for every minute it's down. I can't tell what I am and am not allowed to change with how the indexer variable is treated, without reading other functions. I can't tell what it will be when it's passed in, I can't tell how it will change when I pass it to stringFill, and I don't know if I'll break anything in CoreAction if I change its value within lineSplitter.

Now, the fact that you've made all three of those functions private is a big help - I can be sure that there are no other places in the codebase that I might be breaking with edits to lineSplitter. But it would still be vastly preferable to find an approach that doesn't require ref variables at all.

An object should always be valid. Take this contrived example:

public class Foo
{
    public string Value;
    public int Length()
    {
        return value.Length;
    }
}

// elsewhere
var a = new Foo();
a.Value = "hello";
Console.WriteLine(a.Length()); // Prints "5"

var b = new Foo();
Console.WriteLine(b.Length()); // Error!

I've heard this called "temporal coupling". It's unfriendly because there's no warning that I can't call Length() on an invalid object - just the error when I try. My favorite approach to provide that warning is by required constructor arguments; simply don't allow invalid objects to exist.

public class Foo
{
    public string Value { get; } // no "set" -> must be assigned at construction

    /// <param name="value">
    /// The value used for <see cref="Length()"/>. Cannot be null.
    /// </param>
    public Foo(string value)
    {
        Value = value ?? throw new ArgumentNullException("Foo requires a non-null value");
    }

    public int Length()
    {
        return Value.Length;
    }
}

// elsewhere
var a = new Foo("hello");
Console.WriteLine(a.Length()); // Prints "5"

var b = new Foo(); // Doesn't compile

var c = new Foo(null); // *Immediate* feedback

Why am I ranting about this? It applies to your strategy of creating a list of MessageInfo objects, only some of which are valid. That works, so long as you always remember to check. I would much rather see the invalid input lines completely filtered out.

String.Join is often nicer than StringBuilder, in the same way Linq is often nicer than a for- or foreach-loop. A StringBuilder generally requires four lines of code:

var builder = new StringBuilder();
foreach (var w in words)
    builder.Append(w + " ");
return builder.ToString()

Whereas string.Join (which has similar performance characteristics), often requires only one (or perhaps two for readability):

return string.Join(" ", words);

Regex Capture Groups are your friends, as Alan T pointed out. Here's a demo you can play around with and see live results (That site uses the Ruby regex engine rather than the .NET regex engine, but the differences are minor enough that you can ignore them here).

Linq is a beautiful library in C# for transforming sequences, with extension functions on IEnumerable like Select(mapper), Where(filter), and ToList(). To me, this task is begging for the application of Linq. My solution uses foreach in 1 place, Linq in 5 places, and 0 fors or whiles. I'll show you more below.

Separate classes to hold separate logic. You've already done some of this; I would take it even further. Here's how I sliced it all up:

  • A Program class to hold Main (and almost nothing else)
  • A ChatLog class to hold the messages and drive the validation/parsing
  • A LineParser class to validate each line, and construct a ChatMessage
  • A ChatMessage class to hold each message's info
  • A NameParser class to calculate payload sizes, and sanitize names

An outline of my solution - brace yourself, because I may have gone a little too far (but I'll let you be the judge):

// Program - "build these objects, then ask them questions"
public class Program
{
    static void Main()
    {
        var log = new ChatLog(Console.ReadLine);
        foreach (var message in log.Messages)
        {
            Console.WriteLine($@"{message.Sender} says ""{message.Message}"" to {message.Receiver}");
        }
        Console.WriteLine($"Total data transferred: {log.TotalPayloadSize}MB");
    }
}
using System.Collections.Generic;
using System.Linq;

// ChatLog - "Filter bad lines, build ChatMessages, and summarize"
public class ChatLog
{
    public IEnumerable<ChatMessage> Messages { get; }
    public int TotalPayloadSize { get; }

    public ChatLog(Func<string> readLine)
    {
        var numberOfLines = int.Parse(readLine())
        var lines = Enumerable       // My Linq approach to avoid a foreach loop:
            .Range(1, numberOfLines) // Generate a sequence of N integers
            .Select(_ => readLine()) // Replace each integer with a string from the input

        Messages = Parse(lines);
        TotalPayloadSize = Messages.Sum(message => message.PayloadSize);
    }

    private List<ChatMessages> Parse(IEnumerable<string> lines)
    {
        // Here I used Linq to
        //     transform each line to a LineParser object (Select)
        //     filter out invalid lines (Where)
        //     transform each remaining LineParser to a ChatMessage (Select)
        //     return a list, to avoid the multiple enumeration trap (ToList)
    }
}
// LineParser - "Filter out junk and calculate size"
public class LineParser
{
    public bool Success { get; }

    private ChatMessage _message;
    public ChatMessage Message => _message ?? throw new InvalidOperationException("Line could not be parsed");

    private static Regex _format { get; } = new Regex("...");
    public LineParser(string line)
    {
        Success = match.Success;

        if (Success)
        {
            // set the _message variable to a new ChatMessage from on match.Groups
        }
    }
}
// ChatMessage - "Hold"
public class ChatMessage
{
    public int PayloadSize { get; }
    public string Sender { get; }
    public string Receiver { get; }
    public string Message { get; }

    public ChatMessage(string rawSender, string rawReceiver, string message)
    {
        // Construct two NameParsers, and set field values
    }
}
// NameParser - "Filter out junk and calculate size"
public class NameParser
{
    public string CleanName { get; }
    public int PayloadSize { get; }

    public NameParser(string rawName)
    {
        // set field values
    }

    private int SumOfDigitCharacters(string rawName)
    {
        // Here I used Linq to
        //     transform rawName to an array of characters
        //     filter out non-digit characters
        //     transform each digit character to an int
        //     sum them
    }

    private static Regex _letterOrSpace { get; } = new Regex("...");
    private string StripNonAlphaCharacters(string rawName)
    {
        var lettersAndSpaces = // Here I used Linq to
        //     transform rawName to an array of chars
        //     filter out invalid chars

        return string.Join(
            separator: string.Empty,
            values: lettersAndSpaces);
    }
}

Why so many classes? It might look awkward, but it's hugely valuable in terms of testability. The same goes for the Func<string> parameter to ChatLog - it feels strange but it allows ChatLog to be completely independent of Console, which is a big win. This means that I can create a suite of unit tests automatically verifying the behavior of the program without manually inspecting what was printed to the console after each run of the program. For example:

[TestClass]
public class NameParserTests
{
    [TestMethod]
    public void NameParser_CalculatesSizeCorrectly()
    {
        var testCases = new List<(string rawName, int expectedSize)>
        {
            ("s1ender", 1),
            ("sen2d3r", 5),
            ("s123der", 6),
            ("s10nd11", 3),
        };

        foreach (var (rawName, expectedSize) in testCases)
        {
            var parser = new NameParser(rawName);
            Assert.AreEqual(expectedSize, parser.PayloadSize, $"'{rawName}' should have a payload of size {expectedSize}, not {parser.PayloadSize}");
        }
    }
}

Now that I've verified the behavior of NameParser directly, I don't need to worry about it in any of my other tests. If I make any changes, I can run all of my tests again in a matter of seconds. If there are any problems, the tests immediately show what went wrong.

A final note about performance. The three situations where you should be concerned about code performance, in my opinion, are these:

  • You know your code that will be called thousands of times per second in a tight loop
  • You are entering a coding competition where you will be judged on speed
  • You have noticed performance problems in your application

Short of those situations, the maintainability of your code is far more important than the speed. I don't know whether my solution is faster than yours, but

  • If it is, I'll bet it's too small of a difference for a human to notice
  • If there's a bug, I'd rather be troubleshooting the one with unit tests
  • When the requirements change, I'd rather be expanding the one with unit tests

I've gone on long enough, so I'll spare you any more preaching about Separation of Concerns and Unit Testing. Let me just include some snippets from my other tests:

// NameParser should sanitize like this
("sender", "sender"),
("SENDER", "SENDER"),
("S12end3456er", "Sender"),
("@$s e^&*", "s e"),

// LineParser should reject these
"",
"asdf",
"s:sender;r:receiver;m:\"message\"",
"s:sender;r:receiver;m-\"message\"",
"s:sender;r:receiver;m--\"message",
"s:sender; r:receiver;m--\"message\"",
"s:sender;r:receiver; m--\"message\"",
"s:sen;er;r:receiver;m--\"message\"",
"s:sender;r:rece;iver;m--\"message\"",
"s:sender;r:receiver;m--\"me55age\"",
"s:sender;r:receiver;m--\"mess@ge\"",
"s:sender;r:receiver;m--\"mess;ge\"",
"s:sender;r:receiver;m--\"mes\"ge\"",
" s:sender;r:receiver;m--\"message\"",
"s:sender;r:receiver;m--\"message\" ",

// LineParser should accept these
"s:sender;r:receiver;m--\"message\"",
"s:SENDER;r:receiver;m--\"message\"",
"s:123456;r:receiver;m--\"message\"",
"s:@$ ^&*;r:receiver;m--\"message\"",
"s:sender;r:receiver;m--\"message\"",
"s:sender;r:RECEIVER;m--\"message\"",
"s:sender;r:12345678;m--\"message\"",
"s:sender;r:!@#$ ^&*;m--\"message\"",
"s:sender;r:receiver;m--\"THIS is a MESSAGE\"",

// Helper function to test ChatLog
private Func<string> Enumerate(params string[] lines)
{
    var enumerator = lines.GetEnumerator();

    return () => enumerator.MoveNext()
        ? (string)enumerator.Current
        : throw new InvalidOperationException($"There were only {lines.Length} lines of input!");
}

// Helper function usage:
var log = new ChatLog(Enumerate(
    "3",
    "s:P5%es4#h@o;r:G3#o!!s2h#2o;m--\"Attack\"",
    "s:G3er%6g43i;r:Kak€$in2% re3p5ab3lic%an;m--\"I can sing\"",
    "s:BABAr:Ali;m-No cave for you"));
| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

We are using regular expressions to recognise the valid message but then manually extracting the values.

If we change the regex pattern to ^s:([^;]+);r:([^;]+);m--\"([\\w\\s]+)\"$ we can use the groups with the regex (the parentheses indicate the groups)

  • The sender is match.Groups[1].Value
  • The receiver is match.Groups[2].Value
  • The message is match.Groups[3].Value

We can be a bit fancier if desired and use named groups ^s:(?<sender>[^;]+);r:(?<receiver>[^;]+);m--\"(?<message>[\\w\\s]+)\"$

  • The sender is match.Groups["sender"].Value
  • The receiver is match.Groups["receiver"].Value
  • The message is match.Groups["message"].Value

We can simplify things an amount by placing all the active code in the validation/parsing piece and using the MessageInfo so hold the results. If we ever need to make changes, we now have them all localized to ProcessLine not split between CoreAction/lineSplitter and the MessageInfo class

e.g.

private class MessageInfo
{
    public MessageInfo(string sender, string receiver, string message, int score)
    {
        Sender = sender;
        Receiver = receiver;
        Message = message;
        Score = score;
    }
    public string Sender { get; }
    public string Receiver { get; }
    public string Message { get; }
    public int Score { get; }
}



static void Main(string[] args)
{
    var count = int.Parse(System.Console.ReadLine());
    var total = 0;
    for(var index = 0; index < count; index++)
    {
        var info = ProcessLine(System.Console.ReadLine());
        if (info == null) continue;
        total += info.Score;
        System.Console.WriteLine($@"{info.Sender} says ""{info.Message}"" to {info.Receiver}");
    }
    System.Console.WriteLine($"Total data transferred: {total}MB");

}

private const string Pattern = @"^s:(?<sender>[^;]+);r:(?<receiver>[^;]+);m--""(?<message>[\w\s]+)""$";
private readonly static Regex Regex = new Regex(Pattern);

private static MessageInfo ProcessLine(string line)
{
    var match = Regex.Match(line);
    if (!match.Success) return null;

    var score = 0;
    var sender = CleanName(match.Groups["sender"].Value, ref score);
    var receiver = CleanName(match.Groups["receiver"].Value, ref score);

    return new MessageInfo(sender, receiver, match.Groups["message"].Value, score);
}

private static string CleanName(string input, ref int score)
{
    var builder = new StringBuilder();
    foreach (var ch in input)
    {
        if (char.IsDigit(ch))
        {
            score += ch - '0';
            continue;
        }
        if (char.IsLetter(ch) || ch == ' ')
        {
            builder.Append(ch);
        }
    }
    return builder.ToString();
}
| improve this answer | |
\$\endgroup\$

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.