5
\$\begingroup\$

I'm working with the XMLSOCCER.COM API. I'm a little confused with how to best design the application. The problem I'm facing is that teams do not have a league ID so I couldn't associate a team with a league by that.

In order to associate them my league has a List<Team> and Team has List<Player>.

I'm trying to reduce the number of calls to the API but I can't find a way around this.

Classes

   class Leagues
    {

      public  List<League> _Leagues { get; set; }
        public Leagues()
        {

            XmlDocument xdoc = Program.MakeRequest("http://www.xmlsoccer.com/FootballDataDemo.asmx/GetAllLeagues", Program.API_KEY);

            StringBuilder output = new StringBuilder();

            string json = JsonConvert.SerializeXmlNode(xdoc);

            JObject o = JObject.Parse(json);

            IList<JToken> results = o["XMLSOCCER.COM"]["League"].Children().ToList();

            IList<League> leagues = new List<League>();

            _Leagues = new List<League>();
            foreach (JToken result in results)
            {
                League league = JsonConvert.DeserializeObject<League>(result.ToString());
                _Leagues.Add(league);
            }


        }
    }
    class League
    {
        public int id { get; set; }
        public string Name { get; set; }
        public string Country { get; set; }
        public string Historical_Data { get; set; }
        public string Fixtures { get; set; }
        public string Livescore { get; set; }
        public int NumberOfMatches { get; set; }
        public DateTime LatestMatch { get; set; }

        public List<Team> Teams { get; set; }

        public League()
        {

        }

        public void GetLeagueTeamsByID(int league, string seasondateString){
            var url = String.Format("http://www.xmlsoccer.com/FootballDataDemo.asmx/GetAllTeamsByLeagueAndSeason?ApiKey={0}&league={1}&seasonDateString={2}", Program.API_KEY, id, seasondateString);

            try
            {

                XmlDocument xdoc = Program.MakeRequest(url);

                StringBuilder output = new StringBuilder();

                string json = JsonConvert.SerializeXmlNode(xdoc);

                JObject o = JObject.Parse(json);

                IList<JToken> results = o["XMLSOCCER.COM"]["Team"].Children().ToList();

                IList<Team> teams = new List<Team>();

                Teams = new List<Team>();
                foreach (JToken result in results)
                {
                    Team team = JsonConvert.DeserializeObject<Team>(result.ToString());
                    Teams.Add(team);
                }

            }
            catch (Exception ex)
            {
                Console.WriteLine("------------ERROR----------------");
                Console.WriteLine(ex.ToString());
            }

        }
    }

    class Team
    {
        public int Team_Id { get; set; }
        public string Name { get; set; }
        public string Country { get; set; }
        public string Stadium { get; set; }
        public string HomePageURL { get; set; }
        public string WIKILink { get; set; }

       public List<Player> Players { get; set; }

        public Team()
        {
            GetPlayersByTeam(Team_Id);
        }


        public void GetPlayersByTeam(int team)
        {
            var url = String.Format("http://www.xmlsoccer.com/FootballDataDemo.asmx/GetPlayersByTeam?ApiKey={0}&team_id={1}", Program.API_KEY, team);

            try
            {

                XmlDocument xdoc = Program.MakeRequest(url);

                StringBuilder output = new StringBuilder();

                string json = JsonConvert.SerializeXmlNode(xdoc);

                JObject o = JObject.Parse(json);

                IList<JToken> results = o["XMLSOCCER.COM"]["Player"].Children().ToList();

                IList<Player> players = new List<Player>();

                Players = new List<Player>();
                foreach (JToken result in results)
                {
                    Player player = JsonConvert.DeserializeObject<Player>(result.ToString());
                    Players.Add(player);
                }

            }
            catch (Exception ex)
            {
                Console.WriteLine("------------ERROR----------------");
                Console.WriteLine(ex.ToString());
            }

        }

    }

    class Player{
        public int Id { get; set; }
        public string Name { get; set; }
        public double Height { get; set; }
        public double Weight { get; set; }
        public string Nationality { get; set; }

        public string Position { get; set; }
        public int Team_Id { get; set; }
        public int PlayerNumber { get; set; }
        public DateTime DateOfBirth { get; set; }
        public DateTime DateOfSigning { get; set; }
        public string Signing { get; set; }
    }

Program

 static void Main(string[] args)
        {
            Leagues leagues = new Leagues();

            foreach (var league in leagues._Leagues)
            {
                //Just get Scottish Premier League
                if (league.id == 3)
                {
                    Console.WriteLine(league.id + " " + league.Name);
                    league.GetLeagueTeamsByID(league.id, "1415");
                    foreach (var team in league.Teams)
                    {
                        Console.WriteLine(team.Team_Id + " " + team.Name);

                        foreach (var player in team.Players)
                        {
                            Console.WriteLine("\t\t Player Name:" + player.Name);
                        }
                    }
                }

            }

            Console.ReadKey();

        }

        public static XmlDocument MakeRequest(string requestUrl, string API_KEY)
        {
            try
            {

                requestUrl = requestUrl + "?ApiKey=" + API_KEY;

                HttpWebRequest request = WebRequest.Create(requestUrl) as HttpWebRequest;
                HttpWebResponse response = request.GetResponse() as HttpWebResponse;

                XmlDocument xmlDoc = new XmlDocument();
                xmlDoc.Load(response.GetResponseStream());
                return (xmlDoc);

            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);

                Console.Read();
                return null;
            }
        }

        public static XmlDocument MakeRequest(string requestUrl)
        {
            try
            {
                HttpWebRequest request = WebRequest.Create(requestUrl) as HttpWebRequest;
                HttpWebResponse response = request.GetResponse() as HttpWebResponse;

                XmlDocument xmlDoc = new XmlDocument();
                xmlDoc.Load(response.GetResponseStream());
                return (xmlDoc);

            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);

                Console.Read();
                return null;
            }
        }

Example Team XML

<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>45</Team_Id>
<Name>Aberdeen</Name>
<Country>Scotland</Country>
<Stadium>Pittodrie Stadium</Stadium>
<HomePageURL>http://www.afc.co.uk</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Aberdeen_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>46</Team_Id>
<Name>St Johnstone</Name>
<Country>Scotland</Country>
<Stadium>McDiarmid Park</Stadium>
<HomePageURL>http://www.perthstjohnstonefc.co.uk</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/St._Johnstone_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>47</Team_Id>
<Name>Motherwell</Name>
<Country>Scotland</Country>
<Stadium>Fir Park Stadium</Stadium>
<HomePageURL>http://www.motherwellfc.co.uk</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Motherwell_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>48</Team_Id>
<Name>Inverness C</Name>
<Country>Scotland</Country>
<Stadium>Caledonian Stadium</Stadium>
<HomePageURL>http://ictfc.com</HomePageURL>
<WIKILink>
http://en.wikipedia.org/wiki/Inverness_Caledonian_Thistle_F.C.
</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>50</Team_Id>
<Name>Hearts</Name>
<Country>Scotland</Country>
<Stadium>Tynecastle Stadium</Stadium>
<HomePageURL>http://www.heartsfc.co.uk/page/Home</HomePageURL>
<WIKILink>
http://en.wikipedia.org/wiki/Heart_of_Midlothian_F.C.
</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>51</Team_Id>
<Name>Dundee United</Name>
<Country>Scotland</Country>
<Stadium>Tannadice Park</Stadium>
<HomePageURL>http://www.dundeeunitedfc.co.uk</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Dundee_United_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>52</Team_Id>
<Name>Kilmarnock</Name>
<Country>Scotland</Country>
<Stadium>Rugby Park</Stadium>
<HomePageURL>http://www.kilmarnockfc.co.uk/page/Home</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Kilmarnock_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>53</Team_Id>
<Name>Hibernian</Name>
<Country>Scotland</Country>
<Stadium>Easter Road</Stadium>
<HomePageURL>http://www.hibernianfc.co.uk/page/Home</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Hibernian_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>54</Team_Id>
<Name>Celtic</Name>
<Country>Scotland</Country>
<Stadium>Celtic Park</Stadium>
<HomePageURL>http://www.celticfc.net</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Celtic_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>56</Team_Id>
<Name>St Mirren</Name>
<Country>Scotland</Country>
<Stadium>St. Mirren Park</Stadium>
<HomePageURL>http://www.saintmirren.net/pages/</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/St._Mirren_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>360</Team_Id>
<Name>Ross County</Name>
<Country>Scotland</Country>
<Stadium>Victoria Park</Stadium>
<HomePageURL>http://www.rosscountyfootballclub.co.uk/</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Ross_County_F.C.</WIKILink>
</Team>
<Team xmlns="http://xmlsoccer.com/Team">
<Team_Id>561</Team_Id>
<Name>Partick</Name>
<Country>Scotland</Country>
<Stadium>Firhill Stadium</Stadium>
<HomePageURL>http://www.ptfc.co.uk/</HomePageURL>
<WIKILink>http://en.wikipedia.org/wiki/Partick_Thistle_F.C.</WIKILink>
</Team>
\$\endgroup\$
  • \$\begingroup\$ Just to clarify, your code is currently working, you're just unhappy with it? \$\endgroup\$ – RubberDuck May 2 '15 at 16:52
  • 1
    \$\begingroup\$ @RubberDuck, yes it's all working fine. I'm starting out in OOP and I'm not happy with my design. \$\endgroup\$ – ZeroBased_IX May 2 '15 at 16:53
  • 1
    \$\begingroup\$ Great! Welcome to Code Review! \$\endgroup\$ – RubberDuck May 2 '15 at 16:55
  • \$\begingroup\$ Crazy question perhaps, but why don't you use the official .NET library? \$\endgroup\$ – BCdotWEB Sep 28 '15 at 7:43
5
\$\begingroup\$

What @RubberDuck said

plus... I want to be clear that your Leagues / Teams / Players collections etc. - your domain / business objects, should work amongst themselves only. That DAL will create these. Then methods like FindByTeamName() are trivial because we're past all that XML stuff.

public class League {
    protected List<Team> Teams { get; set; }

    ///<summary>
    /// Case Sensitive search by Name. Returns null if not found
    ///<summary> 
    public Team FindByName(string teamName) {
        return Teams.Find(x -> x.Name == teamName);
    }
}

I'm trying to reduce the number of calls to the API but I can't find a way around this.

I think the main problem is that everything is public. So client code can do anything anywhere at any time - a free-for-all of code calls.


The problem I'm facing is that teams do not have a league ID so I couldn't associate a team with a league by that.

In order to associate them my league has a List and Team has List.

Do you mean that if you had that ID then a League would not have a List<Team>? But of course it should! You are not building a normalized relational database here.


@RubberDuck said: "I really don't like that there's sooo much logic in your constructors. "

I want to emphasize that you should not dump raw XML into your constructors and parse that out - I mean for example a single team's XML into the Team constructor. Instead your "DAL/Factory" should parse it all and:

  • create Player objects
  • create a List<Player>
  • create a Team and pass that List<Player> in the constructor
  • create a List<Team>
  • create a League and pass List<Team> in the constructor.

Well, you can certainly have Team.Add(Player aPlayer), League.Add(Team aTeam), etc. to build up a Team one player at a time, for example - as the DAL/factory builds them.


... I have to call the GetTeamsByLeagueAndSeason in the league construction and vice versa for teams -> players.

No. No you don't. You'll have methods for that. Called after everything is constructed.

 public class Leagues {
    protected List<League> Leagues { get; set; }

    public ??? GetLeagueBySeason (string LeagueName, string season) {
        League thisLeague = GetByName (sring LeagueName);

        if(thisLeague != null)
            thisLeague.GetSeason(season);
    }

    public League GetByName(string leagueName) {
        return Leagues.Find(x -> x.Name == leagueName);
    }
}

public class League {
    protected List<Team> Teams { get; set; }

    public ????? GetSeason (string thisSeason){
        // I have no idea what a season is. I expect you need
        // a Season class. Perhaps it has a complete list of games,
        // which is yet another class. A Game: date, opponents, score.
        // then a season: year, List<Game>
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks @radarbob! That's just what I needed, you make a lot of sense. I'll have a play around with these suggestions when I get home from work. Just as an FYI a season is a period of time that a league runs for i.e. English Barclay's Premier League starts in August 2015 ends in May 2016 so the season is 15/16 \$\endgroup\$ – ZeroBased_IX Sep 28 '15 at 7:54
3
\$\begingroup\$

I think you're right to seek some advice.

  1. I really don't like that there's sooo much logic in your constructors. Constructors should do the bare minimum required to make sure your class is ready to use. They also should have next to zero possibility of throwing an exception. Move as much of this logic as possible somewhere else. Probably into some kind of class factory, but I'll get to that in a second.

  2. I don't think these classes should know how to query the data from the website. In my mind these classes should be pretty "dumb", consisting mostly of {get; set;} properties.

    In order to dumb these down, you'll need to introduce a Data Access Layer, or DAL. Your DAL will have the responsibility of querying the data from the website and creating new instances of your models. This is the class factory I mentioned earlier.

Unfortunately, I don't really have time to draft up an example at the moment. I hope this still helps.

\$\endgroup\$
  • \$\begingroup\$ Thank you Rubberduck, you're right. It's fine that you don't have time to draft up an example, you've put me on the right track. I just have to do some more reading up on N-Tier design and the factory pattern. \$\endgroup\$ – ZeroBased_IX May 2 '15 at 18:12
  • \$\begingroup\$ Factory might not be exactly the right word/pattern, but it's the closest I have in my vocabulary. \$\endgroup\$ – RubberDuck May 2 '15 at 19:40
  • \$\begingroup\$ I'm looking into the Builder pattern too. Though your advice was correct and I do need to separate the application into layers. Can you think of a better way of constructing them? There are leagues, teams, and players. I have to call the GetTeamsByLeagueAndSeason in the league construction and vice versa for teams -> players. This seems really inefficient, note the nested loops in Program. Any ideas? \$\endgroup\$ – ZeroBased_IX May 3 '15 at 9:48

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.