Open VBAndCs opened 5 years ago
For your specific example, I think support for ranges #418, #25, would have more general usefulness (e.g. apart from just XML literals).
Something like:
Dim x = <div>
<%= From i In (1 to 10) Select <p>@i</p>
%>
</div>
I've done quite a bit of work with XML literals in my time; I have a couple of thoughts here about things I've learnt through years of having to maintain them:
1) Anything more complicated than the simple example you've got needs to be in its own method
2) In general, any time you're using a For i = 1 to 10
, you should be looking for the For Each i
version of it, and as soon as you find the For Each
version, then you can use the From .. Select
version.
@pricerc: Using XML literals is not about calcualting values. The output is an XML document, and one can modify it at anytime, so, it will be easier to see all the document parts in one place. Doing this is possible by inline invoked lambdas (as in the example) but the resultant code is verbose and lee readable. I am using xml literal to generate razor views, and I not pleases with it as is. Look at this:
<div class="container">
<%= (Function()
If Me.CatalogModel.CatalogItems.Any() Then
Return <zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
<div class="esh-catalog-items row">
<%= (Iterator Function()
For i = 0 To CatalogModel.CatalogItems.Count - 1
Yield _
<div class="esh-catalog-item col-md-4">
<partial name="_product" for=<%= $"CatalogItems[{i}]" %>/>
</div>
Next
End Function)() %>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
</zml>
Else
Return _
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
End If
End Function)() %>
</div>
It will be better if it becomes"
<div class="container">
<(If Me.CatalogModel.CatalogItems.Any() Then
<zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
<div class="esh-catalog-items row">
<(For i = 0 To CatalogModel.CatalogItems.Count - 1
<div class="esh-catalog-item col-md-4">
<partial name="_product" for=<%= $"CatalogItems[{i}]" %>/>
</div>
Next)>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
</zml>
Else
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
End If)>
</div>
@VBAndCs;
If I replace your Me.CatalogModel.CatalogItems
with CatalogItems As New List(Of String) From {"Foo", "Bar"}
, then this is the output from your code:
<div Class="container">
<zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo" />
<div class="esh-catalog-items row">
<div class="esh-catalog-item col-md-4">
<partial name="_product" for="CatalogItems[0]" />
</div>
<div class="esh-catalog-item col-md-4">
<partial name="_product" for="CatalogItems[1]" />
</div>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo" />
</zml>
</div>
--or, for an empty list:
<div Class="container">
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
</div>
I've loaded your code, and two alternatives into LinqPad. Here they are:
Sub Main
Original.Dump()
Alternate.Dump()
GetZml.Dump()
End Sub
Dim CatalogItems As New List(Of String) From {} ' "Foo", "Bar"}
Dim Original As XElement =
<div class="container">
<%= (Function()
If CatalogItems.Any() Then
Return <zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
<div class="esh-catalog-items row">
<%= (Iterator Function()
For i = 0 To CatalogItems.Count - 1
Yield _
<div class="esh-catalog-item col-md-4">
<partial name="_product" for=<%= $"CatalogItems[{i}]" %>/>
</div>
Next
End Function)() %>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
</zml>
Else
Return _
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
End If
End Function)() %>
</div>
Dim Alternate As XElement =
If(CatalogItems.Any(),
<div Class="container">
<zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
<div class="esh-catalog-items row">
<%=
From Item In CatalogItems Select
<div class="esh-catalog-item col-md-4">
<partial name="_product" for=<%= $"CatalogItems[{CatalogItems.IndexOf(Item)}]" %>/>
</div>
%>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
</zml>
</div> , 'else
<div Class="container">
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
</div>
)
Function GetZML() As XElement
If CatalogItems.Any() Then Return _
<div Class="container">
<zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
<div class="esh-catalog-items row">
<%=
From Item In CatalogItems Select
<div class="esh-catalog-item col-md-4">
<partial name="_product" for=<%= $"CatalogItems[{CatalogItems.IndexOf(Item)}]" %>/>
</div>
%>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
</zml>
</div>
Return _
<div Class="container">
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
</div>
End Function
For me, I think the method version is the easiest to read, because it has space between the two conditions, and makes it easier to process them separately.
But both alternatives a) move the conditional to the outside, and b) use query expressions instead of anonymous methods to produce a more legible (for me) bit of code than your example. They both also require less code.
Getting a bit off-topic: I don't like the {CatalogItems.IndexOf(Item)}
bit, but a) this example is intended to demonstrate some alternatives, b) a real object would probably be more interesting - you'd probably use something like a part number or database row id, and c) as I mentioned earlier, #25 and #418 would offer an actual numbered range option.
@pricerc
Nice, but not a general solutions. For example: If you have nested ifs and loops, you would have many permutations for the output, so you will return to use embded lambdas (or functions).
About the {CatalogItems.IndexOf(Item)}
:
The original cshtml code was using fopreach item
and sending the item
var to the tag helper. But using vbxml evaluates the loop, so there is no item
left to send to the helper method! To solve this, I added the CatalogItems property to the model, to return an array, so that I can index it in the VB loop (to avoid searching for the index in the enumerator in each loop). So, you brought back this issue again :).
In fact, I didn't like this solution, so I developped ZML to write an XML loop and translate it to C# directly:
<z:foreach var="catItem" in="Model.CatalogModel.CatalogItems">
<div class="esh-catalog-item col-md-4">
<partial name="_product" for="@catItem" />
</div>
</z:foreach>
I would use ZML loops in such cases, but in this version of the example, I am showing how to do it with vb. The big issue for both vbxml and ZML now is the lake of editor support. I hope I find a solution so these two methods can be used in practice.
Nice, but not a general solutions. For example: If you have nested ifs and loops, you would have many permutations for the output, so you will return to use embded lambdas (or functions).
I beg to differ. The number of layers of loop makes no difference, except that the more layers deep you go, the more important it is to break down the problem into smaller pieces to make them manageable.
The techniques I've used here, I've used many times before to achieve the result you're after - XML with a specific structure, almost always more complicated than your quite trivial example. Usually, in my case it's something like sales orders being sent to a web service, but the concepts are the same - constructing well-formed XML with multiple layers, and loops at multiple layers.
Multiple nested loops in the same method are usually (in my experience) a sign of code that needs refactoring - a single method should do as little as possible, ideally just one thing.
If you have, for example, a sales order, with say, some header information and then some order lines, and the order lines might have components, and the components might have serial numbers, I would have:
It means I can modify and (probably more importantly) test each one individually without interfering with the other parts.
In general, I make it my goal to never have a method that exceeds one screen in height. I frequently fail (e.g. because I have to build XML for a sales order header that has 50 required elements), but that doesn't change the goal, because it makes for easier maintenance in the long term.
@pricerc You seem focusing on the output. We can get the compose the output by string literals or any other way, but the most important is to easily formatting the input to be readable and maintainable. Writing a bunch of methods just to represent an xml document is overkill and defiys the purpose of having xml literals in first place. Related to this, XML literals xsd schemas stopped working and there is no way to have intelligence support for whatever schema used, which gave away half of xml literals powerfulness. I reported this here, but for some reason it is not marked as a bug yet! https://github.com/dotnet/roslyn/issues/34816 Anyway, thanks @pricerc for good conversation. You always give me new ideas.
@pricerc You seem focusing on the output.
Yes, because that's what you're creating - output in the form of XML, I think I possibly don't understand your point with that statement.
but the most important is to easily formatting the input to be readable and maintainable.
Input is what the users of your application type into their computers. What we're talking about is the program that the user interfaces with. The most important thing is that the program's source code needs to be readable and maintainable. The XML literals in your source file are not 'input', they are syntax sugar for LINQ to XML objects.
Writing a bunch of methods just to represent an xml document is overkill and defiys the purpose of having xml literals in first place.
I disagree. But that's ok. What I described is exactly what I have done for a specific project. It was not overkill, it was the right amount of effort to create an easily-maintained, structured program that produced the right XML, reliably. If you try to produce a >100 line XML document all in one big VB method, you're in for a world of hurt with a maintenance nightmare.
To think that the point of XML literals is to allow you to code a whole XML document into your VB source file is a very limiting view. The true power comes from being able to build the various pieces of a large document in small manageable chunks, using XML literals instead of having to use XML classes (System.Xml or System.Xml.Linq) explicitly.
@pricerc OK. Lets say we use xml literals in deferent ways. My suggestion will not heart your way, as you can totally not use it. But it will make xml literals more powerful and resilient tool to compose dynamic xml documents. There are many defects need to be fixed in XML literals, so I think making all improvement all together with a proper redesign can minimize the total cost.
@VBAndCs , of course we use XML literals in different ways, and that's awesome.
But do you really think that this:
If(CatalogItems.Any(),
<div Class="container">
<zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
<div class="esh-catalog-items row">
<%= From i In (1 To CatalogItems.Count - 1) Select
<div class="esh-catalog-item col-md-4">
<partial name="_product" for=<%= $"CatalogItems[{i}]" %>/>
</div>
%>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
</zml>
</div> , 'else
<div Class="container">
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
</div>
)
is more complicated and harder to read than this?:
<div class="container">
<(If CatalogItems.Any() Then
<zml>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
<div class="esh-catalog-items row">
<(For i = 0 To CatalogItems.Count - 1
<div class="esh-catalog-item col-md-4">
<partial name="_product" for=<%= $"CatalogItems[{i}]" %>/>
</div>
Next)>
</div>
<partial name="_pagination" for="CatalogModel.PaginationInfo"/>
</zml>
Else
<div Class="esh-catalog-items row">
THERE ARE NO RESULTS THAT MATCH YOUR SEARCH
</div>
End If)>
</div>
Both would produce the same output, within a single statement.
BTW, I also think that the improvement you're proposing could be addressed with block expressions(#109), which even has some examples using XML literals.
Ok. Let's try another example. How would you represent this:
Function GetVbXml() As XElement
Return _
_
<zml xmlns:z="zml">
<z:model type="ExternalLoginsViewModel"/>
<z:title>Manage your external logins</z:title>
<partial name="_StatusMessage" for="StatusMessage"/>
<%= (Function()
If CurrentLogins?.Count > 0 Then
Return <zml xmlns:z="zml">
<h4>Registered Logins</h4>
<table class="table">
<tbody>
<%= (Iterator Function()
For Each login In CurrentLogins
Yield <tr>
<td>@login.LoginProvider</td>
<td>
<%= (Function()
If ShowRemoveButton Then
Return <form asp-action="RemoveLogin" method="post">
<div>
<input asp-for="@login.LoginProvider" name="LoginProvider" type="hidden"/>
<input asp-for="@login.ProviderKey" name="ProviderKey" type="hidden"/>
<button type="submit" class="btn btn-default" title="Remove this @login.LoginProvider login from your account">Remove</button>
</div>
</form>
Else
Return " "
End If
End Function)() %>
</td>
</tr>
Next
End Function)() %>
</tbody>
</table>
</zml>
End If
If OtherLogins?.Count > 0 Then
Return <zml>
<h4>Add another service to log in.</h4>
<hr/>
<form asp-action="LinkLogin" method="post" class="form-horizontal">
<div id="socialLoginList">
<p>
<%= (Iterator Function()
For Each provider In OtherLogins
Yield <button type="submit" class="btn btn-default" name="provider" value="@provider.Name" title="Log in using your @provider.DisplayName account">@provider.DisplayName</button>
Next
End Function)() %>
</p>
</div>
</form>
</zml>
End If
Return Nothing
End Function)() %>
</zml>
End Function
Of course I want it to be:
Function GetVbXml() As XElement
Return _
_
<zml xmlns:z="zml">
<z:model type="ExternalLoginsViewModel"/>
<z:title>Manage your external logins</z:title>
<partial name="_StatusMessage" for="StatusMessage"/>
<(If CurrentLogins?.Count > 0 Then
<zml xmlns:z="zml">
<h4>Registered Logins</h4>
<table class="table">
<tbody>
<(For Each login In CurrentLogins
<tr>
<td>@login.LoginProvider</td>
<td>
<(If ShowRemoveButton Then
<form asp-action="RemoveLogin" method="post">
<div>
<input asp-for="@login.LoginProvider" name="LoginProvider" type="hidden"/>
<input asp-for="@login.ProviderKey" name="ProviderKey" type="hidden"/>
<button type="submit" class="btn btn-default" title="Remove this @login.LoginProvider login from your account">Remove</button>
</div>
</form>
Else
" "
End If)>
</td>
</tr>
Next)>
</tbody>
</table>
</zml>
End If)>
If OtherLogins?.Count > 0 Then
<zml>
<h4>Add another service to log in.</h4>
<hr/>
<form asp-action="LinkLogin" method="post" class="form-horizontal">
<div id="socialLoginList">
<p>
<(For Each provider In OtherLogins
<button type="submit" class="btn btn-default" name="provider" value="@provider.Name" title="Log in using your @provider.DisplayName account">@provider.DisplayName</button>
Next)>
</div>
</form>
</zml>
End If)>
</zml>
End Function
@VBAndCs, you didn't actually answer my question.
Nonetheless, I'm prepared to take up your challenge: if you can provide a complete working example of that code, that I can run in LinqPad (I need to be able to paste the code into LinqPad, press 'Execute' and have it call your GetVbXml()
and dump the result), then I will have a go at refactoring it for you.
@VBAndCs;
I've just spotted something. Is this bit that I wrote the reason why you thought I was focussed on the output?:
If I replace your
Me.CatalogModel.CatalogItems
withCatalogItems As New List(Of String) From {"Foo", "Bar"}
, then this is the output from your code:<div Class="container"> <zml> <partial name="_pagination" for="CatalogModel.PaginationInfo" /> <div class="esh-catalog-items row"> <div class="esh-catalog-item col-md-4"> <partial name="_product" for="CatalogItems[0]" /> </div> <div class="esh-catalog-item col-md-4"> <partial name="_product" for="CatalogItems[1]" /> </div> </div> <partial name="_pagination" for="CatalogModel.PaginationInfo" /> </zml> </div> --or, for an empty list: <div Class="container"> <div Class="esh-catalog-items row"> THERE ARE NO RESULTS THAT MATCH YOUR SEARCH </div> </div>
I suggest to embed VB code blocks in xml literals like this
I think the
<( )>
is a suitable quote. The <%= %> will continue used to embed values. The<( )>
will grantee not to break any existing code. It is supposed to return a value, so it is lowered to a function call:Notes:
<( )>
, implicit return is assumed.Return Nothing
is assumed. This will allow writing Subs also to define some variables in the XML literal scope.@
rules to switch between text, VB Code, and XML Literals. Again, since the<( )>
is new quote, there is no fear to break any existing code.Note that This proposal can be implemented using @AnthonyDGreen 's Top-Level Code Prototype can achieve this proposal:
and many more, like embedding code in interpolated strings