locked
Add list of images to a class RRS feed

  • Question

  • User439975351 posted

    Hi all,

    I'm trying to extract of list of images from a JSON stream and have the following code. I can get at the data but can't work out how I iterate over the images and add them into the class, any help would be greatly appreciated as always :) 

    public class Cabins
            {
                public string Name { getset; }
                public string Description { getset; }        
                public virtual ICollection<CabinImages> Images { getset; }
                public virtual ICollection<FloorPlans> Floorplans { getset; }
            }
     
            public class CabinImages
            {
                public string Href { getset; }
            }
     
            public class FloorPlans
            {
                public string Href { getset; }
            }
     
     ...........................................................................
    Retrieve JSON from API ...........................................................................
                List<Cabins> cabinList = new List<Cabins>();             List<string> images = new List<string>();             foreach (var item in cabins)             {                 Cabins _cabin = new Cabins();                 _cabin.Name = item.name;                 _cabin.Description = item.description;                              foreach (var img in item.images)                 {                     var imgSrc = new CabinImages();                     imgSrc.Href = img.href;                     _cabin.Images.Add(imgSrc);                 }                 foreach (var floorPlan in item.floorplans)                 {                     var imgSrc = new FloorPlans();                     imgSrc.Href = floorPlan.href;                     _cabin.Floorplans.Add(imgSrc);                 }                 cabinList.Add(_cabin);             }
    Thursday, April 21, 2016 11:36 AM

Answers

  • User303363814 posted

    The properties of a class tell you what sort (or type) of thing can be stored using the property.  So the 'Name' property can store a string.  But ... here is the slightly subtle point, the property is not actually a string - think of it as a specially shaped hole that you can only put strings in.  Typically, you would do something like luxuryCanin.Name = "Honeymoon suite".  This takes the string "Honeymoon Suite" and puts it into the special 'string only' hole called Name.

    Similarly, your property called Images is a specially shaped hole which can take a collection (an ICollection<CabinImages> to be exact).  There ae many sorts of collections - List, Dictionary, Map, and so on.  Think of a collection as cardboard box which holds things.  The cardboard box has internal slots that can only take things that are shaped like CabinImages in this case.  Here is the point - the Images property (just like the Name property) is a specially shaped hole that you can only put an ICollection<CabinImmages> in.  It is not a collaction itself but rather a holder for a collection.

    So, what's the problem?  Yo haven't put a collection into the Images hole of your Cabins class.  To put a collection in that particular property you need to create a collection and then assign it (just the same as the way that you put a string into the Name property hole)..  The more common way to do this would be using a constructor in your class.

    Add some code like this to the Cabins class.

    public Cabins()
    {
       this.Images = new List<CabinImages>();
    }

    The 'new List<CabinImages>()' expression creates a suitable container and then it is slotted into the waiting hole with the assignment.

    I hope you can work out what to do with Floorplans.

    (Sorry to harp but the difference between single things and groups of things is quite important - a collection of CabinImages versus a single CabinImage.  Naming single things with a singular noun and multiple things with a plural noun is very important documentation. It helps sorting out when you can just store something and when you have to create some kind of collection to hold multiple somethings.)

    So, when you modify your code using the advice above and your program still does not work then post your new code here and we expect to see better names.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, April 21, 2016 10:25 PM
  • User303363814 posted

    It may be using concepts you haven't learnt yet but linq is a wonderful thing.  I try to avoid all loops in my code, I like the declarative nature of linq.

    So my first suggestion would be to remove the inner loops to get

    var cabins = new List<Cabin>();
    foreach (var item in cabinData)
    {
       var cabin = new Cabin {Name = item.name, Description = item.description, Stats = item.stats };
    
       cabin.images = item.images.Select(i => new CabinImage { ImageUrl = i.href});
    
       cabin.FloorPlans = item.floorplans.Select(i => new FloorPlan { FloorPlanUrl = floorplan.href };
    }
    
    return cabins;

    There is one small problem with this code - it won't compile.  You have declared Cabin.Images to be of type ICollection<T> but the linq query is going to return an IEnumerable<T> and so the assignment will generate an error.  You could add .ToList() to the end of the linq queries to solve the problem but I don't think that is the best way to go.

    A good design principle is to always have instances of a class/type in a valid state.  In your case I would suspect that a Cabin is only valid if it has a Name and a Description.  Immediately after constructing a Cabin it is in an invalid state - no name and no description.  The way to fix this is to remove the parameterless constructor and require the Name and Description be passed to a new Constructor.

    Another popular idea is to use factories to create objects rather than bare constructors.  I like to enforce this by having one project (Assembly) which js has my data model in it.  Any application layer or UI code goes in a different Assembly.  Now, any constructors can be marked 'internal' which makes these invisible to other Assemblies but allows access by the Factories.

    I am a big fan of immutable types, which means that after creation you cannot modify an instance of a type.  This is great for testing and maintenance but can casue some challenges when interacting with a database (which I supect you are doing).

    So, I would have something like this

    // DomainModel project
    public class Cabin
    {
       public string Name {get; private set;}
       public string Description {get; private set;}
       public IEnumerable<CabinImages> Images {get; private set;}
       public IEnumerable<Floorplan> Floorplans {get; private set;}
    
       public static Create(??? rawCabin) //  whatever the type is the your json converter returns
       {
          return new Cabin(rawCabin.name,
                           rawCabin.description,
                           rawCabin.images.Select(i => new CabinImage(i.href),
                           rawCabin.floorplans.Select(f => new Floorplan(f.href));
       }
    
       Cabin(string name, string description, IEnumerable<CabinImages> images, IEnumerable<Floorplan> plans)
       {
          Name = name;
          Description = description;
          Images = images;
          Floorplans = plans;
       }
    }
    
    public class CabinImage
    {
       public string Href {get; private set;}
    
       CabinImage(string href) { Href = href;}
    }
    
    public class Floorplan
    {
       public string Href {get; private set;}
    
       Floorplan(string href) { Href = href; }
    }
    
    //  Some client project
    var cabinList = cabins.Select(c => Cabin.Create(c);
    

    <<<Most of the time I would remove the static method Cabin.Crate() to a separate class of factories but for this example I have followed Martin Fowler's suggestion.  Basically, I don't think it fits the idea of a ubiquitous language to have the factory for a Cabin be one of the services offered by a Cabin.  In the real world a Cabin has a name and images and other things but no real-world cabin can build itself! >>>

    With this arrangement, the knowledge about a Cabin is in the Cabin class.  The knowledge of how to create a Cabin from the json is in one place - the Create factory.  The client code doesn't need to know anything bout the conversion process.  If the json changes then you only need to change the Create factory.

    In the future, if there is a new piece of Cabin information (NumberOfBedrooms, for example) then you modify the Cabin class so that it knows this new property and you modify the factory so that it knows how to get the information from the json.  The client code only needs to be modified to make use of the new information (it doesn't need to know about how to convert from json or how to initialize the new property.

    By the way - look no loops!  Not actuallu central to what we have done but linq helps to reduce the amount of 'ceremony' code that you have to maintain.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Sunday, April 24, 2016 11:30 PM

All replies

  • User303363814 posted

    Is this all of your code?  There seems to be quite a few errors.  The best way to fix errors is one at a time, starting with the first one.

    When you compile your source code, what is the very first error that the compiler reports (give the full text of the error)?  Which line of your source file does the error refer to?

    If you cannot fix that error yourself then give us the details and someone will help you.

    With that error out of the way, compile again and repeat the process.

    When you are down to zero compilation errors then you can run the code and fix the errors as they occur, one at a time.

    But, don't take my advice.  Check out what the guy who wrote the compiler suggests at https://ericlippert.com/2014/03/05/how-to-debug-small-programs/

    By the way, it is generally a good idea to name classes with a singular noun because an instance of a class represents a single thing.  Variables which represent collections should have plural nouns.  It just makes things easier to read and easier to understand.  So your classes should be called - Cabin, CabinImage and FloorPlan.  The class member names are OK - 'Name', 'Images', etc (but I hate Href - ImageUrl and FloorPlanUrl would be more descriptive).  The local variable 'cabinList' would be better called 'cabins' or 'allCabins' or 'newCabins' or 'cabinsForSale' or something that describes the business reason for the variable rather than using 'List' as part of the name (it is obvious that it is a List - and who cares, anyway.  You might change the data structure you use later.).

    Thursday, April 21, 2016 12:25 PM
  • User439975351 posted

    Many thanks for taking the time to reply Paul. I have noted the advice and will correct those items.

    No this isn't all of the code it omits the JSON call. I just noticed though that I pasted in "links" rather than "imgSrc" so sorry for that :( 

    The error that I'm trying to solve is how to add a list of images to the class, the only error that I get is at this point:

    _cabin.Images.Add(imgSrc);
    
    and

    _cabin.Floorplans.Add(imgSrc);
    

    I assume that I am trying to add these incorrectly. The error states:

    An exception of type 'System.NullReferenceException' occurred in TTU-MVC.dll but was not handled in user code

    Additional information: Object reference not set to an instance of an object.

    However when I hover over "links" it has the desired URL populated and it appears to be of type CabinImages

    Thursday, April 21, 2016 1:03 PM
  • User303363814 posted

    You are getting closer.

    Put a breakpoint on the statement which throws the exception.  When the breakpoint is reached hover over each part of the statement.  It should be obvious what is null.

    (Hint:- it is not _cabin or imgSrc)

    Thursday, April 21, 2016 1:27 PM
  • User439975351 posted

    Thanks again Paul.

    Hmmm, it says that _cabin.Images is null but I would expect that as that where I'm trying to add the URL?

    Thursday, April 21, 2016 2:17 PM
  • User-434868552 posted

    @1jus

    Tip:  when you post code snippets, it's frequently a good idea to post your associated data (substitute fake data for any parts of the data the your peers at forums.asp.net should not see such as real bank account numbers, passwords, et cetera).

    have you tried examining your JSON using the FREE Fiddler4 tool, originally authored by Eric Lawrence, now available from Telerik http://www.telerik.com/fiddler?

    GiGo =. Garbage in, Garbage out ... Fiddler4 will let you inspect your JSON data outside of your program.

    you should wrap the two .Add lines that are causing you grief in a try/catch; also you should test for null before your .Add statements and skip if null.

    https://msdn.microsoft.com/en-CA/library/0yd65esw.aspx  "try-catch (C# Reference)"

    https://msdn.microsoft.com/en-us/library/xtd0s8kd(v=vs.110).aspx "How to: Use the try/catch Block to Catch Exceptions"

    Thursday, April 21, 2016 4:14 PM
  • User303363814 posted

    The properties of a class tell you what sort (or type) of thing can be stored using the property.  So the 'Name' property can store a string.  But ... here is the slightly subtle point, the property is not actually a string - think of it as a specially shaped hole that you can only put strings in.  Typically, you would do something like luxuryCanin.Name = "Honeymoon suite".  This takes the string "Honeymoon Suite" and puts it into the special 'string only' hole called Name.

    Similarly, your property called Images is a specially shaped hole which can take a collection (an ICollection<CabinImages> to be exact).  There ae many sorts of collections - List, Dictionary, Map, and so on.  Think of a collection as cardboard box which holds things.  The cardboard box has internal slots that can only take things that are shaped like CabinImages in this case.  Here is the point - the Images property (just like the Name property) is a specially shaped hole that you can only put an ICollection<CabinImmages> in.  It is not a collaction itself but rather a holder for a collection.

    So, what's the problem?  Yo haven't put a collection into the Images hole of your Cabins class.  To put a collection in that particular property you need to create a collection and then assign it (just the same as the way that you put a string into the Name property hole)..  The more common way to do this would be using a constructor in your class.

    Add some code like this to the Cabins class.

    public Cabins()
    {
       this.Images = new List<CabinImages>();
    }

    The 'new List<CabinImages>()' expression creates a suitable container and then it is slotted into the waiting hole with the assignment.

    I hope you can work out what to do with Floorplans.

    (Sorry to harp but the difference between single things and groups of things is quite important - a collection of CabinImages versus a single CabinImage.  Naming single things with a singular noun and multiple things with a plural noun is very important documentation. It helps sorting out when you can just store something and when you have to create some kind of collection to hold multiple somethings.)

    So, when you modify your code using the advice above and your program still does not work then post your new code here and we expect to see better names.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Thursday, April 21, 2016 10:25 PM
  • User439975351 posted

    Hi @gerrylowry,

    Thanks for the tip, I'm still learning so I appreciate the support & advice from more experienced people. 

    I had examined the JSON prior so that I could get out the correct data from a fairly nested result, I am using Postman which is nice and simple, I have used Fiddler before but I'm no expert with it.

    Noted on the try/catch point, I have used these in the solution on db saves & mail etc. so I just need to be more "belt and braces" with the use cases.

    Sunday, April 24, 2016 8:15 AM
  • User439975351 posted

    Many thanks again Paul for pointing me in the right direction. Here is a working version of my code now. If you have any further comments I'd welcome your input:

    List<Cabin> cabins = new List<Cabin>();
     
               foreach (var item in cabinData)
               {
                   Cabin cabin = new Cabin();
     
                   cabin.Name = item.name;
                   cabin.Description = item.description;
                   cabin.Stats = item.stats;
     
                   var cabinImage = new List<CabinImage>();
                   foreach (var img in item.images)
                   {
                       cabinImage.Add(new CabinImage { ImageUrl = img.href });
                   }
     
                   cabin.Images = cabinImage;
     
                   var cabinFloorPlan = new List<FloorPlan>();
     
                   foreach (var floorPlan in item.floorplans)
                   {
                       cabinFloorPlan.Add(new FloorPlan { FloorPlanUrl = floorPlan.href });
                   }
     
                   cabin.FloorPlans = cabinFloorPlan;
     
                   cabins.Add(cabin);
               }
     
               return cabins;
    Sunday, April 24, 2016 8:18 AM
  • User303363814 posted

    It may be using concepts you haven't learnt yet but linq is a wonderful thing.  I try to avoid all loops in my code, I like the declarative nature of linq.

    So my first suggestion would be to remove the inner loops to get

    var cabins = new List<Cabin>();
    foreach (var item in cabinData)
    {
       var cabin = new Cabin {Name = item.name, Description = item.description, Stats = item.stats };
    
       cabin.images = item.images.Select(i => new CabinImage { ImageUrl = i.href});
    
       cabin.FloorPlans = item.floorplans.Select(i => new FloorPlan { FloorPlanUrl = floorplan.href };
    }
    
    return cabins;

    There is one small problem with this code - it won't compile.  You have declared Cabin.Images to be of type ICollection<T> but the linq query is going to return an IEnumerable<T> and so the assignment will generate an error.  You could add .ToList() to the end of the linq queries to solve the problem but I don't think that is the best way to go.

    A good design principle is to always have instances of a class/type in a valid state.  In your case I would suspect that a Cabin is only valid if it has a Name and a Description.  Immediately after constructing a Cabin it is in an invalid state - no name and no description.  The way to fix this is to remove the parameterless constructor and require the Name and Description be passed to a new Constructor.

    Another popular idea is to use factories to create objects rather than bare constructors.  I like to enforce this by having one project (Assembly) which js has my data model in it.  Any application layer or UI code goes in a different Assembly.  Now, any constructors can be marked 'internal' which makes these invisible to other Assemblies but allows access by the Factories.

    I am a big fan of immutable types, which means that after creation you cannot modify an instance of a type.  This is great for testing and maintenance but can casue some challenges when interacting with a database (which I supect you are doing).

    So, I would have something like this

    // DomainModel project
    public class Cabin
    {
       public string Name {get; private set;}
       public string Description {get; private set;}
       public IEnumerable<CabinImages> Images {get; private set;}
       public IEnumerable<Floorplan> Floorplans {get; private set;}
    
       public static Create(??? rawCabin) //  whatever the type is the your json converter returns
       {
          return new Cabin(rawCabin.name,
                           rawCabin.description,
                           rawCabin.images.Select(i => new CabinImage(i.href),
                           rawCabin.floorplans.Select(f => new Floorplan(f.href));
       }
    
       Cabin(string name, string description, IEnumerable<CabinImages> images, IEnumerable<Floorplan> plans)
       {
          Name = name;
          Description = description;
          Images = images;
          Floorplans = plans;
       }
    }
    
    public class CabinImage
    {
       public string Href {get; private set;}
    
       CabinImage(string href) { Href = href;}
    }
    
    public class Floorplan
    {
       public string Href {get; private set;}
    
       Floorplan(string href) { Href = href; }
    }
    
    //  Some client project
    var cabinList = cabins.Select(c => Cabin.Create(c);
    

    <<<Most of the time I would remove the static method Cabin.Crate() to a separate class of factories but for this example I have followed Martin Fowler's suggestion.  Basically, I don't think it fits the idea of a ubiquitous language to have the factory for a Cabin be one of the services offered by a Cabin.  In the real world a Cabin has a name and images and other things but no real-world cabin can build itself! >>>

    With this arrangement, the knowledge about a Cabin is in the Cabin class.  The knowledge of how to create a Cabin from the json is in one place - the Create factory.  The client code doesn't need to know anything bout the conversion process.  If the json changes then you only need to change the Create factory.

    In the future, if there is a new piece of Cabin information (NumberOfBedrooms, for example) then you modify the Cabin class so that it knows this new property and you modify the factory so that it knows how to get the information from the json.  The client code only needs to be modified to make use of the new information (it doesn't need to know about how to convert from json or how to initialize the new property.

    By the way - look no loops!  Not actuallu central to what we have done but linq helps to reduce the amount of 'ceremony' code that you have to maintain.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Sunday, April 24, 2016 11:30 PM
  • User439975351 posted

    Hi Paul,

    Again, I really appreciate you taking the time to reply in such detail.

    Some of the concepts you discuss I am yet to learn but I will be sure to explore these further.

    I have used Linq in other areas of the project but you're correct that I am still learning and am not utilising this as much as I should. Historically I have been using (don't cry) classic ASP as a hobby so I have many things to get up to speed on!

    I had retained the loops as I actually need to process the image url further, I am sending the url to a method to check if we have a cached copy and if not to download and save the image locally before adding the "new" image path to the list. Due to inexperience with Linq, when I have tried to "manipulate" data as part of the selection process I have failed.

    I will have to do some more reading :) 

    Monday, April 25, 2016 10:11 AM