Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for rendering IEnumerable entities #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AbdoMahfoz
Copy link
Contributor

Consider the following case

public class Category : BaseModel
{
    [Required] public string Name { get; set; }

    public int? ParentId { get; set; }
    public virtual Category Parent { get; set; }
    
    public virtual ICollection<Category> Children { get; set; }
    public virtual ICollection<Product> Products { get; set; }

    public override string ToString() => $"{Name} - {(ParentId == null ? "root" : "child")}";
}

Currently, navigation properties Children and Products cause the code to crash.
After inspecting the code I found that there is no support for rendering such properties.
So, I made the controller eager load the entities before sending them to the View to avoid the crash, then I added code in the view that should render these properties as <ul>

{
foreach (var raw in (IEnumerable)entityProperty.GetValue(value)!)
{
finalString += $"<li><a href=\"{Url.Action("Index", new { id = raw.GetType().Name })}\">{raw}</a></li>";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strings behave like value variables and each time you add to them create another variable and its not optimum, better to use stringbuilder

finalString += $"<li><a href=\"{Url.Action("Index", new { id = raw.GetType().Name })}\">{raw}</a></li>";
}
}
catch(NullReferenceException) {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with that? Consuming exceptions like that is an anti-pattern. Shouldn't there be a way to avoid an NRE in the first place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants