17
\$\begingroup\$

Is this code good enough, or is it stinky?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.IO;

namespace DotNetLegends
{
    public class LogParser
    {
        /// <summary>
        /// Returns a populated Game objects that has a list of players and other information.
        /// </summary>
        /// <param name="pathToLog">Path to the .log file.</param>
        /// <returns>A Game object.</returns>
        public Game Parse(string pathToLog)
        {
            Game game = new Game();

            //On actual deployment of this code, I will use the pathToLog parameter.
            StreamReader reader = new StreamReader(@"D:\Games\Riot Games\League of Legends\air\logs\LolClient.20110121.213758.log");
            var content = reader.ReadToEnd();

            game.Id = GetGameID(content);
            game.Length = GetGameLength(content);
            game.Map = GetGameMap(content);
            game.MaximumPlayers = GetGameMaximumPlayers(content);
            game.Date = GetGameDate(content);

            return game;
        }

        internal string GetGameID(string content)
        {
            var location = content.IndexOf("gameId");
            var gameID = content.Substring(location + 8, 10);
            gameID = gameID.Trim();
            return gameID;
        }

        internal string GetGameLength(string content)
        {
            var location = content.IndexOf("gameLength");
            var gamelength = content.Substring(location + 13, 6);
            gamelength = gamelength.Trim();
            var time = Convert.ToInt32(gamelength) / 60;
            return time.ToString();
        }

        internal string GetGameMap(string content)
        {
            var location = content.IndexOf("mapId");
            var gameMap = content.Substring(location + 8, 1);
            switch (gameMap)
            {
                case "2":
                    return "Summoner's Rift";
                default:
                    return "nul";
            }
        }

        internal string GetGameMaximumPlayers(string content)
        {
            var location = content.IndexOf("maxNumPlayers");
            var maxPlayers = content.Substring(location + 16, 2);
            maxPlayers = maxPlayers.Trim();
            return maxPlayers;
        }

        internal string GetGameDate(string content)
        {
            var location = content.IndexOf("creationTime");
            var creationDate = content.Substring(location + 14, 34);
            creationDate = creationDate.Trim();
            return creationDate;
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ You should close the file stream, and probably declare it with a using statement so it gets disposed. \$\endgroup\$
    – Sean Lynch
    Jan 23, 2011 at 2:39
  • \$\begingroup\$ Totally, but I'm asking about the way I'm parsing itself, and how I'm passing the parameters. (Upvote either way!) \$\endgroup\$
    – Sergio Tapia
    Jan 23, 2011 at 2:41
  • \$\begingroup\$ Ahh, hadn't noticed the rest on my phone, it didn't have scrollbars. \$\endgroup\$
    – Sean Lynch
    Jan 23, 2011 at 3:00
  • 1
    \$\begingroup\$ Is there a reason that you are converting the length back to a string? \$\endgroup\$
    – Sean Lynch
    Jan 23, 2011 at 3:12

7 Answers 7

24
\$\begingroup\$

You have a lot of undescriptive magic numbers and code repetition whilst retrieving the contents of a field. You could eliminate the repetition and make those numbers a little more meaningful by introducing a single method:

protected string GetFieldContent(string content, string field,
    int padding, int length)
{
    var location = content.indexOf(field);
    padding += field.Length;

    var fieldVal = content.Substring(location + padding, length);
    fieldVal = fieldVal.Trim();
    return fieldVal;
}

Use it like so:

internal string GetGameMaximumPlayers(string content)
{
    var maxPlayers = GetFieldContent(content, "maxNumPlayers", 3, 2);
    return maxPlayers;
}

Something to note here is the padding value has changed. You no longer need to include the length of the field name itself and can just describe the number of junk characters afterwards.

Padding length

Upon examining your code I noticed one peculiarity - the fields have inconsistent, magical padding lengths:

  • gameID padding: 2
  • gameLength padding: 3
  • mapId padding: 3
  • maxNumPlayers padding: 3
  • creationTime padding: 2

As a symptom of these being magic numbers I have no idea why this is the case. This is one of the many reasons to avoid magic numbers like the plague: it's difficult to understand their meaning. I'll trust you to evaluate whether varying padding lengths is necessary, or whether you can just assume a constant padding for all fields.

If we can assume a constant padding amount for all fields then we can change the code further a little bit to make your life easier. There are two steps to this change.

First, give your LogParser class a private field:

private const var defaultPadding = 2

Second, GetFieldContent can be refactored to produce this:

protected string GetFieldContent(string content, string field, int length)
{
    var location = content.indexOf(field);
    var padding = defaultPadding + field.Length;

    var fieldVal = content.Substring(location + padding, length);
    fieldVal = fieldVal.Trim();
    return fieldVal;
}

Then getting the contents of a field becomes simpler:

var maxPlayers = GetFieldContent(content, "maxNumPlayers", 2);

\$\endgroup\$
0
6
\$\begingroup\$
  1. Remove the code responsible for retrieving the log file contents. If this is a type that is just responsible for parsing log files than that is all it should do. Think about either passing in the file contents to this method or using Dependency Injection so that you can mock out the StreamReader call in your tests. You do have tests right?
  2. Return the appropriate types from these methods. GetGameLength should return an int and Game.Length should likewise be an int. Same thing for Maximum Players and Game Date.
  3. The 'Get' methods should be private or protected unless there is some specific reason to make them internal.
  4. If there are a finite number of maps (and they are all known) you may want to consider using an enum rather than a string. If an enum is not appropriate you may want to create a GameMap (or some such type to encapsulate the information you care about with regards to a map) and return that instead.
  5. Each 'Get' method needs to check that the location variable is set to something >= 0 when you search for the specified text.
  6. Throw helpful exceptions if the log file does not contain the expected data (or the data isn't the expected type). A NullReferenceException is not a helpful exception.
  7. There are too many magic numbers and strings here. Move these into constants with helpful names.
  8. Add comments with examples of the log file text that you will be searching for in each 'Get' method. This makes maintenance significantly easier as you know the type of text you were expecting to be parsing.
  9. Wrap the StreamReader in a using block.
  10. Check that the file exists before attempting to load it.
\$\endgroup\$
3
  • \$\begingroup\$ My reasoning for using string instead of ints for Length is that I won't be calculating anything with it. Why would I cast it to string when outputting it to the browser? Just leaving it as string from the get go seems the best course of action. Thanks for the other tips. \$\endgroup\$
    – Sergio Tapia
    Jan 23, 2011 at 5:40
  • \$\begingroup\$ for point 3 don't you mean external since the idea is to make it only accessible within this class? agreed with all the other points. \$\endgroup\$
    – greatwolf
    Jan 23, 2011 at 11:59
  • \$\begingroup\$ @victor The method accessibility is set to internal, that's what I was referring to. \$\endgroup\$
    – akmad
    Jan 24, 2011 at 3:47
3
\$\begingroup\$

You have basically created a procedural implementation of your own mechanism to serialize a game object to and from a file wich is ok if you want to be in full controll of the file format.

Have you looked at the concept of dotnet-serialisation? There you can see how declarative style could minimize the code to write by attaching Attributes to the Game-Class and/or its properties.

There are implementatins for binary, xml and json

\$\endgroup\$
2
  • \$\begingroup\$ I don't think it's such a great idea. The game is going to be evolving a lot, and if the class evolves, the serializer will probably be unable to compute it. The custom deserializer is more robust. \$\endgroup\$ Jan 31, 2011 at 9:16
  • \$\begingroup\$ DonkeyMaster: Are you saying that you and a custom serializer because you want to be able to deserialise logs written by and old version of of the program? I think that might be an antipattern. If nothing else you might consider a custom Xml Serialser, over your own flat file format. In any case the Poster has said inanother comment that he isn't incontrol of the fileformat, so this disucssion is irellervent \$\endgroup\$ Jun 9, 2012 at 7:51
2
\$\begingroup\$

You could also replace:

StreamReader reader = new StreamReader(@"D:\Games\Riot Games\League of Legends\air\logs\LolClient.20110121.213758.log");
var content = reader.ReadToEnd();

with:

var content = File.ReadAllText(@"D:\Games...log");

ReadAllText method, as MSDN says: Opens a text file, reads all lines of the file into a string, and then closes the file.

Which is even shorter than using block.

\$\endgroup\$
1
\$\begingroup\$

Seems like you're doing a lot of unecessary and standard managing of a simple logfile. If you were saving the log in binary perhaps it could be excused but seeing as you're working with text files, why not go with xml? or rather, why not go with XML Fragments? Linq to XML has excellent support for managing those kinds of files, both creating, parsing and querying.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ I'm parsing a log file that is completely out of my hands. Trust me, if I had a choice, I'd be parsing json. :P \$\endgroup\$
    – Sergio Tapia
    Jan 23, 2011 at 12:48
  • \$\begingroup\$ Based on the filepath Sergio Tapia appears to be parsing League of Legends' logfiles, presumably those generated just after completion of a match which describe the events of that match. \$\endgroup\$ Jan 25, 2011 at 1:48
0
\$\begingroup\$

I would rename the methods GetGameID(), GetGameLength(), GetGameMap()... because they look like getters, that could be called in any order, while they are actually performing parsing and must be called in a given order. This is confusing at first glance.

I suggest replacing 'Get' with 'Read' or 'Parse':

  • ReadGameID()
  • ReadGameLength()
  • ReadGameMap()
  • ...
\$\endgroup\$
1
  • \$\begingroup\$ They can be called in any order any number of times. This wouldn't be the case if the methods were operating on the reader file stream, but the entire file is read into content right after it's opened. Since the methods operate on a string there's no need to read them in any particular order. \$\endgroup\$ Jan 23, 2011 at 16:07
0
\$\begingroup\$
  1. You should put a comment with the reference to the log format if it exists .
  2. There is no error checking at all.
  3. There is nothing in your code which reflects the structure of the file. If I were in your place I would use regex.
\$\endgroup\$

Your Answer

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