Parsing a file for a game

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
{
///
/// Returns a populated Game objects that has a list of players and other information.
///
/// Path to the .log file.
/// A Game object.
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";
}
}

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

StackExchange Code Review Q#164, answer score: 24