4
\$\begingroup\$

This code works as intended, however, I am sure that there is a way to condense the xmldataLoad method. If someone could maybe explain a better approach than what I have done with the method, I would be extremely grateful. Essentially, what I am saying is I am rather new to using LINQ and don't fully understand the various ways of using it, and I am quite sure there are many people in the same boat as I am.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;
using System.Xml.Linq;
using System.Xml.Serialization;
//using XMLControlLibrary;

namespace EJCTest
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {

        public MainWindow()
        {
            InitializeComponent();
        }

        private void xmlDataLoad()
        {
            string xmlPath = @"C:\Users\Jesse\Source\Repos\AsyncSpeechSynth\EJCTest\XMLControlLibrary\newDictionary.xml";
            XDocument xdoc = XDocument.Load(xmlPath);

            var eng = from wordList in xdoc.Root.Elements("Word")
                      select wordList.Element("English").Value;
            engList.ItemsSource = eng.ToList();

            var rom = from wordList in xdoc.Root.Elements("Word")
                      select wordList.Element("Romaji").Value;
            romList.ItemsSource = rom.ToList();

            var jpn = from wordList in xdoc.Root.Elements("Word")
                      select wordList.Element("Japanese").Value;
            jpnList.ItemsSource = jpn.ToList();
        }

        private void Button_Click(object sender, RoutedEventArgs e)
        {
            xmlDataLoad();
        }

        public class WordList
        {
            string english;
            string romaji;
            string japanese;

            public string English
            {
                get { return english; }
                set { english = value; }
            }

            public string Romaji
            {
                get { return romaji; }
                set { romaji = value; }
            }

            public string Japanese
            {
                get { return japanese; }
                set { japanese = value; }
            }
        }
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You're using a lot. I can't imagine you need all of those. \$\endgroup\$
    – Mast
    Jul 4, 2015 at 8:43

2 Answers 2

4
\$\begingroup\$

Your LINQ is fine. All you're doing is simple navigation of an XDocument. There's isn't any more you can do with LINQ there. However, there are still some improvements that can be made.

Borrowing from janos' answer, you can create a helper method GetWords that takes XDocument xdoc and string name as parameters:

private List<string> GetWords(XDocument xdoc, string name)
{
    var words = from wordList in xdoc.Root.Elements("Word")
                select wordList.Element(name).Value;
    return words.ToList();
}

You can store the XML path as a constant at the class level:

private const string _cXmlPath = @"C:\Users\Jesse\Source\Repos\AsyncSpeechSynth\EJCTest\XMLControlLibrary\newDictionary.xml";

You can get rid of magic strings.

You can use auto properties.

Edit: You can rename the xmlDataLoad method using UpperCamelCase and to start with a verb. Thanks @BCdotWEB and @RobH.

The new version of your class looks like this:

/// <summary>
/// Interaction logic for MainWindow.xaml
/// </summary>
public partial class MainWindow : Window
{
    private const string _cXmlPath = @"C:\Users\Jesse\Source\Repos\AsyncSpeechSynth\EJCTest\XMLControlLibrary\newDictionary.xml";
    private const string _cWordName = "Word";
    private const string _cEnglishName = "English";
    private const string _cRomajiName = "Romaji";
    private const string _cJapaneseName = "Japanese";

    public MainWindow()
    {
        InitializeComponent();
    }

    private void LoadXmlData()
    {            
        XDocument xdoc = XDocument.Load(_cXmlPath);

        engList.ItemsSource = GetWords(xdoc, _cEnglishName);
        romList.ItemsSource = GetWords(xdoc, _cRomajiName);
        jpnList.ItemsSource = GetWords(xdoc, _cJapaneseName);
    }

    private List<string> GetWords(XDocument xdoc, string name)
    {
        var words = from wordList in xdoc.Root.Elements(_cWordName)
                    select wordList.Element(name).Value;
        return words.ToList();
    }

    private void Button_Click(object sender, RoutedEventArgs e)
    {
        LoadXmlData();
    }

    public class WordList
    {
        public string English { get; set; }
        public string Romaji { get; set; }
        public string Japanese { get; set; }
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ This answer is the most in depth and really explains a great alternative to what I have. \$\endgroup\$ Jul 4, 2015 at 18:17
  • \$\begingroup\$ Although there is one slight issue with the code sample. In the GetWords method, it should be... Private List<string> GetWords(Xdocument xdoc, string name) \$\endgroup\$ Jul 4, 2015 at 23:17
  • \$\begingroup\$ @JesseGlover: Good catch! Fixed. \$\endgroup\$ Jul 6, 2015 at 4:03
  • \$\begingroup\$ Prefixing const with "_c"? The Microsoft standard says const should be PascalCase. Methods -- xmlDataLoad() -- should also be PascalCase. \$\endgroup\$
    – BCdotWEB
    Jul 6, 2015 at 9:19
  • \$\begingroup\$ It's more common to name methods starting with the verb - e.g. XmlDataLoad sounds a lot better as LoadXmlData \$\endgroup\$
    – RobH
    Jul 6, 2015 at 10:40
3
\$\begingroup\$

Sure. Create a helper method GetWords that takes XDocument xdoc and string name as parameter:

private List GetWords(XDocument xdoc, string name)
{
    var words = from wordList in xdoc.Root.Elements("Word")
                select wordList.Element(name).Value;
    return words.ToList();
}

Then xmlDataLoad can become:

    private void xmlDataLoad(XDocument xdoc)
    {
        string xmlPath = @"C:\Users\Jesse\Source\Repos\AsyncSpeechSynth\EJCTest\XMLControlLibrary\newDictionary.xml";
        XDocument xdoc = XDocument.Load(xmlPath);

        engList.ItemsSource = GetWords(xdoc, "English");
        romList.ItemsSource = GetWords(xdoc, "Romaji");
        jpnList.ItemsSource = GetWords(xdoc, "Japanese");
    }
\$\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.