3
\$\begingroup\$

Could use some help with my presentation in Software Development. I'm presenting how I reworked a game's parser (SpaceTaxi). Basically we take in a .txt file with ASCII characters and convert them into entities that can be rendered in our game.

Input, Output

In between the input and output I've written a parser. I've tried to follow the Facade Design Pattern with the SOLID principles in mind.

I'm not sure how much code I should show here as I don't want to flood you, so I am going to upload some of it but tell me if it's sufficient.

What my code is intended to do

The parser should be a single instance and when called (Parser.StartParse(string txtFile)) the parser should go through a txt file comparing ascii characters with image files.
The image files are given to our project.

IParse.cs

public interface IParse {
    void Parse(string[] txtFile);
    void Add(List<char> map);
}

public class ParseObstacles : IParse {

        public Dictionary<char, string> Dict;
        public List<Entity> Entities;

        public ParseObstacles() {
            Dict = new Dictionary<char, string>();
            Entities = new List<Entity>();
        }

        /// <summary>
        ///     Extracts the txt information about what each character represents.
        ///     Example from the-beach.txt: "A) aspargus-edge-left.png"
        ///     Here we split ')' to first get the character, A, and it's
        ///     correspondent filename, 'aspargus-edge-left.png'.
        /// </summary>
        public void Parse(string[] txtFile) {
            foreach (var i in txtFile) {
                if (i.Contains(")")) {
                    var tile = i.Split(')')[0].ToCharArray()[0];
                    var name = i.Split(')')[1].Trim();
                    Dict.Add(tile, name);
                }
            }
        }

        public void Add(List<char> map) {
            ParseHelper.AddEntities(map, Dict, Entities);
        }
    }

I intended for the parser to follow the facade pattern.
Does this code in fact implement the facade pattern?

\$\endgroup\$
3
\$\begingroup\$

First, we encounter interface IParse.

public interface IParse {
    void Parse(string[] txtFile);
    void Add(List<char> map);
}

You should use C# conventions.

  • interface name should be a noun or noun phrase IParse -> IParser
  • use a plural name for a variable that represents a sequence of items txtFile -> segments??
  • use a meaningful name to describe structures map -> ?? -> doesn't say anything
  • use the correct types, preferrably interfaces, when dealing with sequences; For instance, IEnumerable to iterate, IList to manipulate.
  • Note that your interface does not have any state or return values. We are never able to interact with it bi-directionally. Too much encapsulation perhaps?

Refactored:

public interface IParser {
    void Parse(IEnumerable<string> segments); // or whatever we are talking about ?
    void Add(IList<char> map); // what is map ?
}

Let's move on to ParseObstacles.

Your state is public, is this as designed or a code smell? Again, use conventions.

public Dictionary<char, string> Dict;
public List<Entity> Entities;
private IDictionary<char, string> values; // they are values I suppose
private IList<Entity> entities;

Next, we find method Parse.

  • the signature is already changed by interface implementation
  • Only use i as an index in an iterator
  • perform i.Split(')') once
  • Dict is already renamed previously
public void Parse(string[] txtFile) {
    foreach (var i in txtFile) {
        if (i.Contains(")")) {
            var tile = i.Split(')')[0].ToCharArray()[0];
            var name = i.Split(')')[1].Trim();
            Dict.Add(tile, name);
        }
    }
}

After changing the method:

 public void Parse(IEnumerable<string> segments) {
      foreach (var segment in segments) {
          if (segment.Contains(")")) {
              var tokens = segment.Split(')');
              var tile = tokens[0].ToCharArray()[0];
              var name = tokens[1].Trim();
              values.Add(tile, name);
          }
      }
 }

Next up, method Add. This is a black-box for us. Consider the fact ParseHelper adds cyclomatic complexity to your API. Perhaps its code should be divided into the other classes.

public void Add(List<char> map) {
     ParseHelper.AddEntities(map, Dict, Entities);
}
public void Add(IList<char> map) {
       // not convinced about ParseHelper ..
       ParseHelper.AddEntities(map, values, entities);
}

And finally, ParserFacade.

Let's have a look at the singleton.

  • A private constructor, well done.
  • A nested static class holding the singleton, good design.
  • all caps -> take it easy: INSTANCE -> Instance
  • a getter method is best substituted with a property getter: getInstance() -> Instance
private ParserFacade() { }

private static class ParserFacadeHolder {
    public static readonly ParserFacade INSTANCE = new ParserFacade();
}

public static ParserFacade getInstance() {
    return ParserFacadeHolder.INSTANCE;
}

After changes:

private static class ParserFacadeHolder {
    public static readonly ParserFacade Instance = new ParserFacade();
}

public static ParserFacade Instance => ParserFacadeHolder.Instance;

I don't like this helper class, but I did expect it to be static. Here you used an instance.

private ParseHelper parseHelper;

Public state detected. Code smell?

 public ParseExits exits;
 public ParseObstacles obstacles;
 public ParsePlatforms platforms;
 public ParseCustomers customers;

Method StartParse:

What bothers me here is the inconsistent and convoluted design of parsing behavior. ParseExits (and sibling classes) can parse the file. But then the facade calls the infamous helper to parse additional data, to then forward this data to ParseExits. This design is too complex in behavioral complexity.

 exits = new ParseExits();
 exits.Parse(file);
 exits.Add(ParseHelper.GetAllTiles(file));
\$\endgroup\$
  • \$\begingroup\$ The thing is about ParseHelper.cs I have some code which all the sibling classes uses. So was not sure if it was a good idea to add some sort of class which could hold these methods they all use. Wanted to avoid copy+pasting \$\endgroup\$ – user10829235 Jun 15 at 21:11
  • \$\begingroup\$ @user10829235 There is a bit of everything in that class :-) \$\endgroup\$ – dfhwze Jun 15 at 21:12
  • 1
    \$\begingroup\$ Code smell I guess. Damn. I thought a lot about the public/private scenario. I wanted my GameLoop in another file to be able to call Parser.exit.Entities or Parser.exit.Dict so I left it public purposely. But thank you for being aware of the small details. This is what really helps me \$\endgroup\$ – user10829235 Jun 15 at 21:30

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.