HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Gotta catch 'em all!

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gottacatchall

Problem

This week's challenge is essentially about fetching Json data from the Web, and deserializing it into objects. I don't have much time to devote to this one, so what I have is very basic: it displays the names of all pokemons in a WPF ListView:

Here's the code for the window:

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        var service = new PokemonApp.Web.Pokedex();
        var pokemons = service.SelectAllPokemonReferences().OrderBy(pokemon => pokemon.Name);
        DataContext = new MainWindowViewModel { References = pokemons };
    }
}


As you've guessed, the interesting code is in the Pokedex class:

```
public class Pokedex
{
public IEnumerable SelectAllPokemonReferences()
{
string jsonResult = GetJsonResult(PokeDexResultBase.BaseUrl + "pokedex/1/");

dynamic pokemonRefs = JsonConvert.DeserializeObject(jsonResult);
ICollection references = new List();

foreach (var pokeRef in pokemonRefs.pokemon)
{
string uri = PokeDexResultBase.BaseUrl + pokeRef.resource_uri.ToString();
references.Add(new ResourceReference
{
Name = pokeRef.name,
ResourceUri = new Uri(uri)
});
}

return references;
}

private static string GetJsonResult(string url)
{
string result;

WebRequest request = HttpWebRequest.Create(url);
request.ContentType = "application/json; charset=utf-8";

try
{
using (var response = request.GetResponse())
using (var stream = response.GetResponseStream())
using (var reader = new StreamReader(stream))
{
result = reader.ReadToEnd();
}
}
catch (Exception exception)
{
result = string.Empty;
// throw?
}

Solution

If a class such as ResourceReference makes sense as far as wrapping the API is concerned, from a UI standpoint, it's like using POCO's in the UI layer: If the data came from a database through Entity Framework, this code would be displaying the entities. This is a basic implementation, but if the goal is to make it an extensible "skeleton" implementation, there should be a ViewModel class for the UI to display, independently of the WebAPI-provided objects; the ViewModel doesn't care about ResourceUri - this property isn't meant to be displayed, it's plumbing for querying the API, doesn't belong in the UI.

As far as the Pokedex service class goes, it looks like it could work, however it should be returning ViewModel objects for the UI to consume, and the static GetJsonResult method could be encapsulated in its own class, which would be an HTTP-based implementation of a helper object whose role is to fetch the data - there could be a SqlServer-based implementation that would do the same thing off a database, idea being to decouple the data from its data source... but it could be overkill.

Usage of Uri in the POCO classes adds unnecessary complexity: they could just as well be kept as normal strings, so this code:

string uri = PokeDexResultBase.BaseUrl + pokeRef.resource_uri.ToString();


Could then look like this, skipping the useless .ToString() call:

string uri = PokeDexResultBase.BaseUrl + pokeRef.resource_uri;


The PokeDexResultBase.BaseUrl static string property would probably be better off as a static property of the Pokedex class, and then it would make PokeDexResultBase tightly coupled with Pokedex, which somehow makes sense.

Also, class PokeDexResultBase should be called PokedexResultBase, for consistency.

Code Snippets

string uri = PokeDexResultBase.BaseUrl + pokeRef.resource_uri.ToString();
string uri = PokeDexResultBase.BaseUrl + pokeRef.resource_uri;

Context

StackExchange Code Review Q#37866, answer score: 3

Revisions (0)

No revisions yet.