Parsing a file for a game

Question

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;
        }
    }
}

You should close the file stream, and probably declare it with a using statement so it gets disposed. — Sean Lynch
– Sean Lynch, Commented Jan 23, 2011 at 2:39
Totally, but I'm asking about the way I'm parsing itself, and how I'm passing the parameters. (Upvote either way!) — Sergio Tapia
– Sergio Tapia, Commented Jan 23, 2011 at 2:41
Ahh, hadn't noticed the rest on my phone, it didn't have scrollbars. — Sean Lynch
– Sean Lynch, Commented Jan 23, 2011 at 3:00
Is there a reason that you are converting the length back to a string? — Sean Lynch
– Sean Lynch, Commented Jan 23, 2011 at 3:12

Community · Accepted Answer · 2020-06-10 13:24:26Z

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);

akmad · Accepted Answer · 2011-01-23 05:03:24Z

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?
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.
The 'Get' methods should be private or protected unless there is some specific reason to make them internal.
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.
Each 'Get' method needs to check that the location variable is set to something >= 0 when you search for the specified text.
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.
There are too many magic numbers and strings here. Move these into constants with helpful names.
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.
Wrap the StreamReader in a using block.
Check that the file exists before attempting to load it.

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. — Sergio Tapia
– Sergio Tapia, Commented Jan 23, 2011 at 5:40
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. — greatwolf
– greatwolf, Commented Jan 23, 2011 at 11:59
@victor The method accessibility is set to internal, that's what I was referring to. — akmad
– akmad, Commented Jan 24, 2011 at 3:47

k3b · Accepted Answer · 2011-01-27 16:57:45Z

3

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

answered Jan 27, 2011 at 16:57

k3b

2311 silver badge4 bronze badges

\$\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\$

DonkeyMaster
– DonkeyMaster

2011-01-31 09:16:32 +00:00
Commented 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\$

Frames Catherine White
– Frames Catherine White

2012-06-09 07:51:44 +00:00
Commented Jun 9, 2012 at 7:51

Add a comment |

frennky · Accepted Answer · 2011-01-27 18:10:10Z

2

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.

answered Jan 27, 2011 at 18:10

frennky

4491 gold badge4 silver badges8 bronze badges

Add a comment |

Homde · Accepted Answer · 2011-01-23 12:18:55Z

1

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.

answered Jan 23, 2011 at 12:18

Homde

2492 silver badges3 bronze badges

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
– Sergio Tapia

2011-01-23 12:48:09 +00:00
Commented 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\$

doppelgreener
– doppelgreener

2011-01-25 01:48:41 +00:00
Commented Jan 25, 2011 at 1:48

Add a comment |

user185 · Accepted Answer · 2011-01-23 14:09:21Z

1

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()
...

answered Jan 23, 2011 at 14:09

user185

\$\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\$

doppelgreener
– doppelgreener

2011-01-23 16:07:03 +00:00
Commented Jan 23, 2011 at 16:07

Add a comment |

Mohammad Tayseer · Accepted Answer · 2011-07-30 01:27:20Z

1

You should put a comment with the reference to the log format if it exists .
There is no error checking at all.
There is nothing in your code which reflects the structure of the file. If I were in your place I would use regex.

answered Jul 30, 2011 at 1:27

Mohammad Tayseer

2261 silver badge2 bronze badges

Add a comment |

Stack Exchange Network

Parsing a file for a game

7 Answers 7

Padding length

You must log in to answer this question.

Hot Network Questions

Parsing a file for a game

7 Answers 7

Padding length

You must log in to answer this question.

Related

Hot Network Questions