10
\$\begingroup\$

This will compile, but I'm thinking that this implementation of read/write is very hard coded and would not be easily adapted to larger situations where large XML documents are being processed or if they are in a different format structure.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Xml.Linq;
using System.Text;
using System.IO;
using System.Threading.Tasks;

namespace Testing_LINQ_to_XML
{
    class Program
    {
        static string playerName;
        static int playerWin = 10;
        static int playerLoss = 1;
        static int playerTie = 0;

        #region Main
        static void Main()
        {
            Console.WriteLine("Enter player Name...");
            playerName = Console.ReadLine();

            readXML();
            writeXML();
            readXML();
            Exit();
        }
        #endregion
        #region Write
        static void writeXML()
        {
            var path = @"C:\Users\Viper45\Documents\Visual Studio 2013\Projects\Testing LINQ to XML\Testing LINQ to XML\XML Saves\" + playerName + ".xml";

            if (System.IO.File.Exists(path)) //Decides if the player has a xml file already
            {
                //Get data from existing
                XDocument file = XDocument.Load(path);
                XElement winTemp = new XElement("playerWin", playerWin);
                XElement lossTemp = new XElement("playerLoss", playerLoss);
                XElement tieTemp = new XElement("playerTie", playerTie);

                ////delete existing file
                File.Delete(Path.Combine(path));

                //combine save data with new game
                playerWin = playerWin + (int)winTemp;
                playerLoss = playerLoss + (int)lossTemp;
                playerTie = playerTie + (int)tieTemp;

                ////Creates new file using last game played.
                XDeclaration _obj = new XDeclaration("1.0", "utf-8", "");
                XNamespace gameSaves = "gameSaves";
                XElement fileNew = new XElement("Root",
                                    new XElement("Player",
                                        new XElement("playerName", playerName),
                                        new XElement("Stats",
                                            new XElement("playerWin", (int)winTemp + playerWin),
                                            new XElement("playerLoss", (int)lossTemp + playerLoss),
                                            new XElement("playerTie", (int)tieTemp + playerTie))));

                file.Save(path);
            }

            else //if the player doesn't have a txt file it creates one here
            {
                XDeclaration _obj = new XDeclaration("1.0", "utf-8", "");
                XNamespace gameSaves = "gameSaves";
                XElement file = new XElement("Root",
                                    new XElement("Player",
                                        new XElement("playerName", playerName),
                                        new XElement("Stats",
                                            new XElement ("playerWin", playerWin),
                                            new XElement("playerLoss", playerLoss),
                                            new XElement("playerTie", playerTie))));

                file.Save(path);
                Console.WriteLine("Save created: " + path);
            }

        }
        #endregion
        #region Read
        static void readXML()
        {
            var path = @"C:\Users\Viper45\Documents\Visual Studio 2013\Projects\Testing LINQ to XML\Testing LINQ to XML\XML Saves\" + playerName + ".xml";

            if (System.IO.File.Exists(path))
            {
                XDocument file = XDocument.Load(path);
                XElement winTemp = new XElement("playerWin", playerWin);
                XElement lossTemp = new XElement("playerLoss", playerLoss);
                XElement tieTemp = new XElement("playerTie", playerTie);


                Console.WriteLine("\nYour Record Is:\n");
                Console.WriteLine("Wins:     " + (int)winTemp);
                Console.WriteLine("Losses:   " + (int)lossTemp);
                Console.WriteLine("Ties:     " + (int)tieTemp);
            }
            else
            {
                Console.WriteLine("\nYou don't have any stats to show yet.  Get playing!!!");
            }

        }
        #endregion
        #region Exit
        static void Exit()
        {
            Console.WriteLine("Press any key to exit.");
            string temp = Console.ReadLine();
        }
        #endregion
    }
}

Here is a sample of what my XML source output looks like:

<Root>
<Player>
 <playerName>name</playerName>
<Stats>
 <playerWin>10</playerWin>
 <playerLoss>1</playerLoss>
 <playerTie>0</playerTie>
 </Stats>
 </Player>
 </Root> 

Output if compiled:

Enter player Name...
name

Your Record Is:

Wins:     10
Losses:   1
Ties:     0

Your Record Is:

Wins:     20
Losses:   2
Ties:     0
Press any key to exit.

I'm looking for input on what I could have done better here, what I could have done to make this more modular for future projects, and what general practices I should be focusing on in the future.

I'm currently trying to learn and focus on a more object oriented approach as I run through these new concepts. Take a look at my GitHub for things I've worked on in the past. My forward plan is to eventually create a static 2D game using VS 2013 & MonoGame. Much further down the road... is to build on that and create a small 2D platformer.

\$\endgroup\$
3
  • \$\begingroup\$ I'll try to come back later for a review, but if you're interested in turning the concept of a "Player Record" into a more object oriented endeavor, XML Serialization might be of interest to you. \$\endgroup\$
    – RubberDuck
    Commented Dec 19, 2014 at 21:51
  • 1
    \$\begingroup\$ Are you stuck with XML specifically? You might take a look at JSON which is more light-weight. Other that that you might look into serialization and deserializtion of XML (or JSON) objects as stated above as you have a known type structure \$\endgroup\$ Commented Dec 19, 2014 at 22:33
  • \$\begingroup\$ @VsevolodGoloviznin - It came down to me wanting to learn XML. Once I found out I could pair it easily with the LINQ namespace in Visual Studio, without adding more libraries, I ran with it. After reading RubberDuck's link I think XML serialization is what I want to pursue. \$\endgroup\$ Commented Dec 19, 2014 at 23:10

3 Answers 3

6
\$\begingroup\$

this implementation of read/write is very hard coded and would not be easily adapted to larger situations where large XML documents are being processed or if they are in a different format structure.

Those are both different situations than what you have now, so those might require different solutions. But it seems that you're not in such situation, you have small amount of simple data, so you probably shouldn't worry about that (this is called You aren't gonna need it, or YAGNI for short).

If you have data that easily maps directly to classes, XML serialization is going to be useful.

If you have huge amount of data that won't fit into memory at once, consider using the overloads of XElemement.Load() and XElement.Save() that transform between XmlReader/XmlWriter and XElement. (E.g. you would use XmlReader to read the topmost parts of the XML, but then read each of the inner elements using XElement.)


static string playerName;
static int playerWin = 10;
static int playerLoss = 1;
static int playerTie = 0;

This should be enclosed into a Player class, not static fields on Program.


#region Main

I don't see any reason for enclosing each method into its own #region, it just adds noise to your code.


var path = @"C:\Users\Viper45\Documents\Visual Studio 2013\Projects\Testing LINQ to XML\Testing LINQ to XML\XML Saves\" + playerName + ".xml";

This sounds like you want a path relative to the location of the executable in the project directory, something like @"..\XML Saves\" + playerName + ".xml".

In a production application, consider using a subdirectory of AppData (you can get that using Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData)).


System.IO.File.Exists(path)

You're already using System.IO, no need to repeat the namespace here.


Path.Combine(path)

You're not combining the path with anything, just path would work the same.


XElement winTemp = new XElement("playerWin", playerWin);
XElement lossTemp = new XElement("playerLoss", playerLoss);
XElement tieTemp = new XElement("playerTie", playerTie);

…

playerWin = playerWin + (int)winTemp;
playerLoss = playerLoss + (int)lossTemp;
playerTie = playerTie + (int)tieTemp;

I don't understand this code. This is basically just a complicated way to double each of the fields.


XDeclaration _obj = new XDeclaration("1.0", "utf-8", "");
XNamespace gameSaves = "gameSaves";
XElement file = new XElement("Root",
                    new XElement("Player",
                        new XElement("playerName", playerName),
                        new XElement("Stats",
                            new XElement ("playerWin", playerWin),
                            new XElement("playerLoss", playerLoss),
                            new XElement("playerTie", playerTie))));

file.Save(path);

This code is repeated twice, only with different values for playerWin, playerLoss and playerTie. You should extract it into a method that takes those values as parameters, to make your code more DRY.


XElement winTemp = new XElement("playerWin", playerWin);

Consider using var here. It's clear from the right side of the assignment what the type is, so there is no reason to repeat the type on the left side.

Some people (me included) think that var makes sense even when the type isn't clear, but that's a matter of opinion.


var path = @"C:\Users\Viper45\Documents\Visual Studio 2013\Projects\Testing LINQ to XML\Testing LINQ to XML\XML Saves\" + playerName + ".xml";

Another code that's repeated, try to avoid that.


string temp = Console.ReadLine();

You don't need the variable here, just Console.ReadLine(); is enough. (Pressing Ctrl+F5 instead of just F5 in Visual Studio also works.)

\$\endgroup\$
1
  • \$\begingroup\$ In regards to the code you don't understand. This was just my quick solution into fixing the following line: new XElement("playerWin", (int)winTemp + playerWin), It compiles, but it doesn't not properly sum the playerWin and (int)winTemp values. My work-around was that I assigned the values before writing to the file so that I could verify the other code I was focusing on for this project. Some of the other issues you pointed out were from me reusing, and not properly cleaning up, the old code. Looks like I need to pay closer attention to detail. \$\endgroup\$ Commented Dec 20, 2014 at 3:26
3
\$\begingroup\$
namespace Testing_LINQ_to_XML

Namespaces should be PascalCase, and I wouldn't use underscores in them. Something like Sandbox.Xml.Linq would be more idiomatic.

#region

You're abusing #region directives, big time - wrapping every method in a region serves no purpose, and clutters up your code. Remove them, remove them all.

Your IDE should make procedures collapsible (VS does), so folding the methods isn't a reason to use regions here. You would typically use regions to regroup related methods in one - but then, that's also what a class does. Hence, regions aren't really needed; whenever you feel like you need a region, you probably actually need to extract some code out of a method, or out of a class.

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

It's already been mentioned that you're in need of a player class and you seem to be interested in my XML Serialization suggestion, so I'll forgo a traditional review and show you how I would do that.

Let's start with looking at your XML. (You should properly indent your XML by the way.)

<Root>
    <Player>
        <playerName>name</playerName>
        <Stats>
            <playerWin>10</playerWin>
            <playerLoss>1</playerLoss>
            <playerTie>0</playerTie>
        </Stats>
     </Player>
 </Root> 

Okay, so we have two distinct objects here. A Player and PlayerStats. Stats has three properties, Win, Loss, and Tie. Let's make a small adjustment to your XML to make serialization/deserialization a bit easier. First, we'll make Player the root object. Then we'll rename some things to make sure our resulting classes have good names. The element (and attribute) names must exactly match our class names and properties.

<Player>
    <Name>name</playerName>
    <Stats>
        <Wins>10</playerWin>
        <Losses>1</playerLoss>
        <Ties>0</playerTie>
    </Stats>
 </Player>

Next, we'll create an actual class for Player.

public class Player
{
    string Name { get; set; }
    PlayerStats Stats { get; set; }
}

And a PlayerStats class.

public class PlayerStats
{
    int Wins { get; set; }
    int Losses { get; set; }
    int Ties { get; set; }
}

Dead dumb simple right? These classes really aren't much more than data structures. (At least right now. They can, and might, actually have methods in the future. Serialization does not prevent it.) We're not quite ready to serialize/deserialize them though. We need to add some attributes to these classes.

To keep things clean, we'll use the XML Serialization namespace.

using System.Xml.Serialization;

Since Player is our root object, it will get the XmlRootAttriubte ermmm..... attribute.

[XmlRootAttribute(Namespace = "", IsNullable = false)]
public class Player
{
    string Name { get; set; }
    PlayerStats Stats { get; set; }
}

PlayerStats is a class (or type) so it gets the type attribute.

[XmlTypeAttribute(AnonymousType = true)]
public class PlayerStats
{
    int Wins { get; set; }
    int Losses { get; set; }
    int Ties { get; set; }
}

And now we're ready to serialize/deserialize this information.

First, an example of reading (deserializing) this information from the XML. I keep this in a static ConfigurationLoader class. You may want to do something different depending on your needs.

    public static Player LoadPlayer(string configFile)
    {
        try
        {
            using (StreamReader reader = new StreamReader(configFile))
            {
                var deserializer = new XmlSerializer(typeof(Configuration));
                return (Configuration)deserializer.Deserialize(reader);
            }
        }
        catch (IOException)
        {
            // it's good to catch IOException in case the file is unavailable. 
            // Optionally write some code to get a default player or otherwise
            // handle the exception

            return GetDefaultPlayer();
        }
    }

Saving back (serializing) is almost as simple.

    public static void SaveConfiguration<T>(T toSerialize, string configFile)
    {
        XmlSerializer xmlSerializer = new XmlSerializer(toSerialize.GetType());
        using (TextWriter textWriter = new StreamWriter(configFile))
        {
            xmlSerializer.Serialize(textWriter, toSerialize);
        }
    }

Note that I use this SaveConfiguration method for serializing many different classes, so it requires that you tell it what type to save back to XML. You could modify it to be specific to your Player class, but I'm not sure there's a benefit there.

And finally, the client code can load/save classes (and their state) to XML pretty simply.

Player player1 = ConfigurationLoader.LoadPlayer("\\path\to\file.xml")

//do something with player

// and save it back

ConfigurationLoader.SaveConfiguration<Player>(player1);
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.