Circular Reference

  • Thread starter Thread starter shapper
  • Start date Start date
S

shapper

Hello,

I have a class named BrandRepository:

public class BrandRepository : IBrandRepository {
private ProductRepository productRepository;
// Methods
}

On brand repository methods I use productRepository to get for example
all products on a brand.
But then on ProductRepository I have the following:

public class ProductRepository : IProductRepository {
private BrandRepository brandRepository;
// Methods
}

For example, when I get a Product I have the BrandId and with it I
need to fill the other Brand properties.

So I end up with circular reference.
The alternative is to place the Linq code for XML in all my
repositories.
But then I will have a lot of repeating code.
And if I change something on the XML files structure I will need to
change the same code in many places.

Is there a way to fix this circular reference.
Maybe some kind of implementation to avoid this and yet allow to keep
the objectives.

Thanks,
Miguel
 
Hello,

I have a class named BrandRepository:

  public class BrandRepository : IBrandRepository {
    private ProductRepository productRepository;
    // Methods
  }

On brand repository methods I use productRepository to get for example
all products on a brand.
But then on ProductRepository I have the following:

  public class ProductRepository : IProductRepository {
    private BrandRepository brandRepository;
    // Methods
  }

For example, when I get a Product I have the BrandId and with it I
need to fill the other Brand properties.

So I end up with circular reference.
The alternative is to place the Linq code for XML in all my
repositories.
But then I will have a lot of repeating code.
And if I change something on the XML files structure I will need to
change the same code in many places.

Is there a way to fix this circular reference.
Maybe some kind of implementation to avoid this and yet allow to keep
the objectives.

Thanks,
Miguel
Click to expand...

Circular references are not a problem in .NET so unless you have other
concerns I wouldn't bother "fixing" anything.
 
Circular references are not a problem in .NET so unless you have other
concerns I wouldn't bother "fixing" anything.
Click to expand...

But if ProductRepository I have BrandRepository brandRepository = new
BrandRepository()

And in BrandRepository I have ProductRepository productRepository =
new ProductRepository()

I think I will have a problem;
In fact I already had earlier and now I am trying to implement this in
some kind of way that makes everything possible.
 
I replicated the error using the following BrandRepository:

public class BrandRepository : IBrandRepository {
private ProductRepository productRepository;
public String Path {
get { return _Path; }
set {
_Path = value;
productRepository = new ProductRepository(String.Concat(_Path,
"Products.xml"));
}
} private string _Path;

And the ProductRepository:

public class ProductRepository : IProductRepository {
private BrandRepository brandRepository;
public String Path {
get { return _Path; }
set {
_Path = value;
brandRepository = new BrandRepository(String.Concat(_Path,
"Brands.xml"));
}
} private string _Path;
}

With this I get the following error:
An unhandled exception of type 'System.StackOverflowException'
occurred in mscorlib.dll
Make sure you don't have an infinite loop or infinite recursion.
 
I replicated the error using the following BrandRepository:

  public class BrandRepository : IBrandRepository {
    private ProductRepository productRepository;
    public String Path {
      get { return _Path; }
      set {
        _Path = value;
        productRepository = new ProductRepository(String.Concat(_Path,
"Products.xml"));
      }
    } private string _Path;

And the ProductRepository:

 public class ProductRepository : IProductRepository {
    private BrandRepository brandRepository;
    public String Path {
      get { return _Path; }
      set {
        _Path = value;
        brandRepository = new BrandRepository(String.Concat(_Path,
"Brands.xml"));
      }
    } private string _Path;

}

With this I get the following error:
An unhandled exception of type 'System.StackOverflowException'
occurred in mscorlib.dll
Make sure you don't have an infinite loop or infinite recursion.
Click to expand...

I'm guessing that in the BrandRepository and ProductRepository
constructions you're creating instances of the opposite class. If
that's the case then that's the source of your error. You have
infinite recursion.

You can fix the problem by accepting a BrandRepository as one of the
arguments to the ProductRepository constructor. That way you can
initialize a ProductRepository using an existing BrandRepository as a
parent.
 
I'm guessing that in the BrandRepository and ProductRepository
constructions you're creating instances of the opposite class.  If
that's the case then that's the source of your error.  You have
infinite recursion.

You can fix the problem by accepting a BrandRepository as one of the
arguments to the ProductRepository constructor.  That way you can
initialize a ProductRepository using an existing BrandRepository as a
parent.
Click to expand...

I see that is an option ...
.... But is there another way to do this? Maybe a different way to
organize my code?

It seems to pass the repositories that another repository needs as
arguments seem a little bit strange to me.
 
[...]
I'm guessing that in the BrandRepository and ProductRepository
constructions you're creating instances of the opposite class.  If
that's the case then that's the source of your error.  You have
infinite recursion.

You can fix the problem by accepting a BrandRepository as one of the
arguments to the ProductRepository constructor.  That way you can
initialize a ProductRepository using an existing BrandRepository as a
parent.
Click to expand...

I see that is an option ...
... But is there another way to do this? Maybe a different way to
organize my code?

It seems to pass the repositories that another repository needs as
arguments seem a little bit strange to me.
Click to expand...

What's "strange" is the way you've intentionally created an infinite
recursion in your code. Unfortunately, because even though you've been
informed many times in reply to past questions that without a
concise-but-complete code example, it's extremely difficult, if not
impossible, to know exactly what your problem is and thus provide useful
advice, you still have not provided a concise-but-complete code example
that reliably demonstrates the problem.

Brian's guess about the constructor is as good as any, and if he's correct
that would definitely explain the _problem_. But we still don't have any
way to know that for sure.

You're not really explaining why it is when you set the Path property of a
BrandRepository (presumably something that also happens in the
constructor), you see a need to immediately create a new
ProductRepository, and vice a versa. What is it about setting the Path
property in each class that makes you feel that creating a new
XXXRepository is always the correct thing to do? Why does the changing
the Path of one XXXRepository mandate that you have to create a whole new
YYYRepository as well?

I have a suspicion that what you really want, rather than creating a whole
new YYYRepository you simply want to make sure that the YYYRepository
class associated with the XXXRepository class for which the Path is
changing has its Path property changed as well to match. Alternatively,
perhaps you _do_ always want a new YYYRepository instance, but you need
some way to indicate to that instance that it's subordinate to a primary
operation happening (i.e. the modification of the Path property of the
XXXRepository class that's creating the new YYYRepository class).

You can address the first possibility simply by updating an existing
YYYRepository's Path property, rather than creating a whole new one:

public class BrandRepository : IBrandRepository
{
private ProductRepository productRepository;
public String Path
{
get { return _Path; }
set
{
_Path = value;
if (productRepository.Path != value)
{
productRepository.Path = value;
}
}
}
private string _Path;
}

And the same thing in your ProductRepository class.

Alternatively, if you really must create a new YYYRepository instance any
time the XXXRepository.Path property is changed, use the approach Brian
suggests. Note that in that approach, you'll want to avoid using the
property setter from the constructor; instead, just set the backing field
directly, and only create a new YYYRepository for the XXXRepository being
created if none was passed to the constructor.

Finally, on top of all of the above, it doesn't make sense to me that the
value you pass to the constructor is a fully-qualified filename, but you
seem to have a Path property that is just the directory path before the
filename. Does that mean in your constructor, you decompose the passed in
path back to the directory path (which is assigned to Path) and a filename
again? If so, wouldn't it make more sense to just leave those things
separate all the time? And if not -- if the passed in value is assigned
directly to Path -- how is that even supposed to work, since you'll just
tack on some ".xml" filename to whatever was passed in, which in the case
of the subordinate assignments will include the other classes filename?

Pete
 
What's "strange" is the way you've intentionally created an infinite  
recursion in your code.  Unfortunately, because even though you've been 
informed many times in reply to past questions that without a  
concise-but-complete code example, it's extremely difficult, if not  
impossible, to know exactly what your problem is and thus provide useful  
advice, you still have not provided a concise-but-complete code example  
that reliably demonstrates the problem.
Click to expand...

Hi Pete,

I am posting my entire code. I removed just a few methods to not be
sol long.
But if necessary I post them to.

BrandRepository:

public class BrandRepository : IBrandRepository {

private ProductRepository productRepository;
private XDocument brands;
private XDocument products;

public String Path {
get { return _Path; }
set {
_Path = value;
productRepository = new ProductRepository(String.Concat(_Path,
"Products.xml"));
brands = XDocument.Load(String.Concat(Path, "Brands.xml"),
LoadOptions.SetBaseUri);
products = XDocument.Load(String.Concat(Path, "Products.xml"),
LoadOptions.SetBaseUri);
}
} private string _Path;

public BrandRepository(String path) {
Path = path;
} // BrandRepository

// Delete
public void Delete(Guid id) {

// Delete brand
XElement brand = brands.Root.Elements("Brand").FirstOrDefault(b
=> b.Element("BrandId").Value == id.ToString());
if (brand != null) {
// Remove brand
brand.Remove();
brands.Save(new Uri(brands.BaseUri).LocalPath);

// Remove products directly on the XML file
IEnumerable<XElement> _products = products.Root.Elements
("Product").Where(p => p.Element("BrandId").Value == id.ToString());
if (_products != null) _products.Remove();
products.Save(new Uri(products.BaseUri).LocalPath);

// OR in alternative use productRepository
productRepository.Delete(brand.Element("BrandId");
}
} // Delete

// GetBrand
public Brand GetBrand(Guid id) {
Brand brand = (from b in brands.Root.Elements("Brand")
where b.Element("BrandId").Value == id.ToString()
select new Brand {
Id = new Guid(b.Element("BrandId").Value),
Created = DateTime.Parse(b.Element
("Created").Value),
Name = b.Element("Name").Value,
Updated = DateTime.Parse(b.Element
("Updated").Value)
}).SingleOrDefault();
return brand;
} // GetBrand
} // BrandRepository

ProductRepository:

public class ProductRepository : IProductRepository {

private BrandRepository brandRepository;
private XDocument Brands;
private XDocument Products;

public String Path {
get { return _Path; }
set {
_Path = value;
brandRepository = new BrandRepository(String.Concat(_Path,
"Brands.xml"));
Brands = XDocument.Load(String.Concat(Path, "Brands.xml"),
LoadOptions.SetBaseUri);
Products = XDocument.Load(String.Concat(Path, "Products.xml"),
LoadOptions.SetBaseUri);
}
} private string _Path;

public ProductRepository(String path) {
Path = path;
} // ProductRepository

// Delete
public void Delete(Guid id) {
XElement product = Products.Root.Elements
("Product").SingleOrDefault(p => p.Element("ProductId").Value ==
id.ToString());
if (product != null) product.Remove();
Products.Save(new Uri(Products.BaseUri).LocalPath);
} // Delete

// GetProduct
public Product GetProduct(Guid id) {

// Define product getting Brand directly from XML file
Product product = (from p in Products.Root.Elements("Product")
where p.Element("ProductId").Value ==
id.ToString()
select new Product {
Id = new Guid(p.Element
("ProductId").Value),
Name = p.Element("Name").Value,
Updated = DateTime.Parse(p.Element
("Updated").Value),
Brand = (from b in Brands.Root.Elements
("Brand")
where b.Element("BrandId").Value
== p.Element("BrandId").Value
select new Brand {
Id = new Guid(b.Element
("BrandId").Value),
Created = DateTime.Parse
(b.Element("Created").Value),
Name = b.Element("Name").Value,
Updated = DateTime.Parse
(b.Element("Updated").Value)
}).SingleOrDefault()
}).SingleOrDefault();

// OR define product getting the product Brand using Brand
repository
Product product = (from p in Products.Root.Elements("Product")
where p.Element("ProductId").Value ==
id.ToString()
select new Product {
Id = new Guid(p.Element
("ProductId").Value),
Name = p.Element("Name").Value,
Updated = DateTime.Parse(p.Element
("Updated").Value),
Brand = brandRepository.Get(p.Element
("BrandId").Value),
}).SingleOrDefault();

return product;

} // GetProduct
} // ProductRepository

In BrandRepository Delete method and in ProductRepository GetProduct I
display two options of my code:
One it uses the XML file; the other accesses the other repository.

When using the other repository I avoid having "duplicated" code on my
repositories.
For example if I change my Brands.xml file structure I don't need to
change code in various places.

The Path is common to all XDocuments or repositories. What changes is
the file that it refers to.
I define the XDocuments on the Path property because they use Path
value and they are used in almost all methods on the repository.

I understood Brian suggestion but if the dependence between
repositories increase with more repositories I think it will be a
little bit confused.

I feel that calling repositories from another repositories feels
strange to me ... But having duplicated code to.
So I am not sure what should I do.

I hope I explained it well.

Thanks,
Miguel
 
You need a caching factory method.

Something like:

public class BrandRepository
{
private static Dictionary<string,BrandRepository> knownBrands;
protected BrandRepository(string path) { knownBrands.Add(path, this); }

public static BrandRepository Get(string path)
{
BrandRepository retval;
if (!knownBrands.TryGetValue(path, out retval)) retval = new
BrandRepository(path);
return retval;
}
}

and then replace every

new BrandRepository(path)

with

BrandRepository.Get(path)
 
Hi Pete,

I am posting my entire code.
Click to expand...

But we don't need your _entire_ code. Just the minimal,
concise-but-complete example.

That said, at least now we can see your constructor and you are in fact
doing what Brian surmised. Which has not only the problem he points out,
but also the one I did as well (i.e. you're passing a string like
"C:\some\path\Brands.xmlProducts.xml" to the constructor of the
ProductRepository, and vice a versa for BrandRepository).

Ben has provided yet another possible approach; his suggestion would be
especially appropriate if creating one of these "XXXRepository" classes is
expensive, and/or you want to always make sure you have just one instance
for a given path. It also will help if, as you say, you start having to
include a large number of "XXXRepository" classes as dependencies in each
"XXXRepository" class.

But you still need to fix the path string creation issue as well.

Finally, I'll suggest you use the Path.Combine() method rather than
String.Concat() to create new paths from existing ones and filenames. It
will be more reliable. It won't fix the problem of adding "Products.xml"
to a path that already ends with "Brands.xml", but it will be a better
approach in other ways.

Pete
 
Ben has provided yet another possible approach; his suggestion would be
especially appropriate if creating one of these "XXXRepository" classes is
expensive, and/or you want to always make sure you have just one instance
for a given path. It also will help if, as you say, you start having to
include a large number of "XXXRepository" classes as dependencies in each
"XXXRepository" class.
Click to expand...

Or if they aren't read-only. If you allow more than one instance connected
to the same file, then if you change one you may change the file, but all
the other in-memory copies won't be updated.
 
Or if they aren't read-only. If you allow more than one instance
connected to the same file, then if you change one you may change the
file, but all the other in-memory copies won't be updated.
Click to expand...

Which could be a positive or negative thing, depending on the intended
behavior. :) But yes, a pseudo-singleton, factory-based pattern such as
you suggested has a lot of useful applications.
 
Back
Top