Blazor ViewModel additional functionality (3 questions)

[j055]

Question

1. RefreshAsync seems a bit opinionated. For example, if I want to copy a BO by passing in an ID RefreshAsync will try to do a 'fetch' instead of a 'create'. Wouldn't it be better to have CreateAsync and FetchAsync methods instead of RefreshAsync?

2. GetPropertyInfo can only handle properties of the root object. Should there be an overload method, e.g. GetPropertyInfo(target, propertyName) or another way to get property info in child objects?

3. Related to above - what's the difference between:

@if (vm.Model.CanReadProperty(nameof(vm.Model.Name)))

and

@if (vm.GetPropertyInfo(nameof(vm.Model.Name)).CanRead)

and why is one better than the other?

Version and Platform
CSLA version: 5.1
OS: Windows
Platform: Razor components (Blazor)

[bboney]

j055,

I ran into the same problem with the Child PropertyInfo's.

Here is an example I used in Blazor to get a Child's PropertyInfo

<TextInput ShowLabel="false" Property="@(new PropertyInfo(ChildToEdit,"ChildProperty"))"></TextInput>

ChildToEdit is the specific child instance in the Child Collection. I used this in an editable grid.

I guess the ViewModel could provide that same option, but ViewModel is also caching the PropertyInfo objects in a dictionary by PropertyName. Doing that for Child Collections might be hard to do in the Root Model.

Ben

[rockfordlhotka]

1. Yes, RefreshAsync is very opinionated. The escape hatch is to set the Model property directly from a DataPortal method call.
2. My thought, and I'm willing to be convinced otherwise, is that child objects/lists should be managed by child views. Remember that Blazor has an amazing component model, and wrapping all the details about dealing with a child object or list in a component of its own is a great way to keep your parent page clean, and also means that you can have a viewmodel for that "child component"
3. I think an alternative would be to do something where vm.GetPropertyInfo accepts an expression pointing to a property, like the For expression in the validation components. So something like vm.GetProperty(() => vm.Model.Child.Name)) - that might work, and would solve the problem as well, in a pretty consistent manner

[j055]

1. Opinionated is fine as long as it leads the developer to implement things in a good way.
2. This is a fair point. So you would inject the viewModel in the child components and set the vw.Model in the OnParametersSetAsync. Presumably we need to handle the PropertyChanged too if it's an editable BO.
3. OK
4. Do we have to have a dependency on the WebAssembly package? What if I'm only using server-side Blazor?

[rockfordlhotka]

1. This bears further thought, I'm sensing unhappiness
2. Yes
3. The For property is something to explore - I'll add a backlog item
4. This is not ideal, but I think the only way around it is to have Csla.Blazor and Csla.Blazor.WebAssembly NuGet packages, so I can pull the one method that needs a type from the WebAssembly package into a separate DLL and package; which do you think is more onerous - having to reference the WebAssembly package on the server, or having a whole new CSLA package to reference in the client?

[rockfordlhotka]

Regarding number 4, I am creating a new Csla.Blazor.WebAssembly package - I think that's clearly the right answer.

[rockfordlhotka]

Regarding number 1 - RefreshAsync I am pretty much sold on changing the method so it requires a data portal call:

  await vm.RefreshAsync(() => DataPortal.FetchAsync<PersonEdit>(123));

This eliminates all ambiguity, making the code explicit and more readable.

[rockfordlhotka]

I've addressed items 2 and 3 with a new GetPropertyInfo overload.

  var info = vm.GetPropertyInfo(() => vm.Model.Child.Name);

Basically, you can provide a nameof of property of the model itself, or an expression to pretty much any other property you'd like to work against.

[brinawebb]

If this were required then the factory methods that we've created would be bypassed. What if your object depended on getting data from the database and called a DataPortal.Create to load some other data, before the DataPortal.Fetch?

FWIW - we rarely (if at all) call a DataPortal method from outside the class as it bypasses our factory methods...

[rockfordlhotka]

I probably won't commit the PR until the end of the weekend, so if you'd like to review the code please check out the PR>

MarimerLLC/csla#1587

[rockfordlhotka]

@brinawebb The new RefreshAsync allows you to provide any method that returns a Task<T> - which should include your async factory methods or a direct DataPortal call.

[brinawebb]

Thanks @rockfordlhotka . I misunderstood, thought you were implying that 'required' meant that's the only way.

[rockfordlhotka]

Yes, confirmed - either works. Page code is like this:

  protected override async Task OnParametersSetAsync()
  {
    //await vm.RefreshAsync(
    //  () => Csla.DataPortal.FetchAsync<PersonEdit>(123, "Rocky"));
    await vm.RefreshAsync(
      () => PersonEdit.FetchPersonEditAsync(123, "Rocky"));
  }

[rockfordlhotka]

@brinawebb well, I am removing the RefreshAsync that accepts parameters - so you must call either a DataPortal method or a factory method.

You wouldn't have used the old RefreshAsync anyway, because it would not have called your factory.

[rockfordlhotka]

fwiw, I'm generally not using or recommending static factory methods anymore, because they are a blocker for unit testing. I don't plan to stop supporting them, I just don't plan to incorporate them in the new editions of the books.

[brinawebb]

Gotcha, thanks for noting that. We were just creating a set of coding guidelines on a new product that preferred the factory methods so it's good timing for us to not do that at all.

Odd, we have a lot of unit tests and don't have any troubles. Probably a separate topic, but what blocks them? Are they unit tests on top of the Csla project?

[rockfordlhotka]

Yeah, if you try to unit test the domain classes it is hard to create them using static factory methods.

Well "hard" is relative, because you can create a mock client-side data portal.

But if you want to mock the domain types then you might not want to be creating them through a factory, because that gets overly complex compared to using Rocks or Moq or whatever.

@JasonBock is kind of my go-to expert in this area.

[brinawebb]

Gotcha. Yes, from the business objects on up we don't have any problems with unit tests. We think of the library as having any UI on top of it, including commandline utilities as well as encouraging our customers to use it as an API. Sometimes we'll put Json web services on top. To us, the unit tests are just another UI.

However...

I'm a bit perplexed that you're not recommending factory method usage anymore as they play a HUGE role in the library. I realize that it's a factory pattern, and isn't really a Csla thing, but it is a very important logical layer (or teir? not sure if I have the correct terminology).

The factory methods simplify development for the tier above as well as the DataPortal methods in the tier below. I think of them as a layer between the business object and the CRUD operations. It is a very simple and thin layer, and my discussion here might make it sound complex, but we do rely on a lot of different things here (oftentimes not all at the same time).

This tier is where our criteria live (if we need them) and we don't expose them to the tier above. Only the DataPortals and the factory methods use them. This allows us to not only centralize our input/parameter validation, but we can fetch or create other things that are needed, use other complex criteria, setup business rules, etc. Using this tier in this fashion also protects our DataPortal methods from having to do all sorts of invalid parameter checking, etc., which you are forced to do if you call them directly. Parameter validation in the DataPortal methods puts logic in the wrong place IMO as it sometimes forces you to use an exceptions if you want to communicate back (very naughty). The DataPortals in our library are clean and just do what they're supposed to do (talk to a database). For us, if a parameter is wrong, we go to the factory/criteria method to fix it.

Also, just from a developer productivity standpoint, centralizing the create/fetch of an object makes it simpler and there are less bugs.

I realize that in the end the factory methods are just a software pattern, not really a Csla thing.

This thread was good as it forced me to think about how we create our library and validated that we're on the right path. I have decided to keep on our current path and use the factory methods as the only way to create instances of our objects. Unfortunately there's no way to enforce it, but there are a lot of things like that in life :-)

Brian

[Chicagoan2016]

fwiw, I'm generally not using or recommending static factory methods anymore, because they are a blocker for unit testing. I don't plan to stop supporting them, I just don't plan to incorporate them in the new editions of the books.

Hi everyone, I am late on this thread but I would love to see the samples or project tracker without factory methods, I have always used the factory methods approach.
Thanks

[rockfordlhotka]

@brinawebb there's a third alternative to consider as well.

Use static factory methods

  [Serializable]
  public class PersonEdit : BusinessBase<PersonEdit>
  {
    public static async Task<PersonEdit> GetPersonEditAsync()
    {
      return await DataPortal.FetchAsync<PersonEdit>();
    }
  }

usage:

  var obj = await PersonEdit.GetPersonEditAsync();

Use data portal directly

No setup, just usage, which is nice - keeps the code clean at the cost of abstraction.

  var obj = await DataPortal.FetchAsync<PersonEdit>();

Use factory type

  public interface IPersonEditFactory
  {
    Task<PersonEdit> FetchAsync();da
  }

  public class PersonEditFactory : IPersonEditFactory
  {
    public static async Task<PersonEdit> FetchAsync()
    {
      return await DataPortal.FetchAsync<PersonEdit>();
    }
  }

usage:

  IPersonEditFactory factory = new PersonEditFactory();
  var obj = await factory.FetchAsync();

Now let's consider using dependency injection of these types.

Use static factory methods

Not possible, because there's nothing to inject. All you can do is use the factory method. Makes it harder to unit test things like a viewmodel.

  public class PageOrSomething
  {
    public void OnInitialize()
    {
      this.DataSource = await PersonEdit.GetPersonEditAsync();
    }
  }

Use data portal directly

  public class PageOrSomething
  {
    private readonly IDataPortal<PersonEdit> portal;

    public PageOrSomething(IDataPortal<PersonEdit> dp)
    {
      portal = dp;
    }

    public void OnInitialize()
    {
      this.DataSource = await portal.FetchAsync<PersonEdit>();
    }
  }

Config in startup:

  services.AddTransient<IDataPortal<>, DataPortal<>>();

Use factory type

  public class PageOrSomething
  {
    private readonly IPersonEditFactory factory;

    public PageOrSomething(IPersonEditFactory f)
    {
      factory = f;
    }

    public void OnInitialize()
    {
      this.DataSource = await f.FetchAsync();
    }
  }

Config in startup:

  services.AddTransient<IPersonEditFactory, PersonEditFactory>();

[j055]

Like all the proposed changes above.

Saw this and thought there was a better way to create a key from an expression (a bit of OCD due to lockdown).

var keyName = property.ToString().Substring(property.ToString().IndexOf(").") + 2);

use instead

var keyName = CreateKey(property);

public static string CreateKey<T>(Expression<T> expression)
{
	var list = new List<string>();
	var member = expression.Body as MemberExpression;
	while (member != null)
	{
		list.Insert(0, member.Member.Name);
		member = member.Expression as MemberExpression;
	}
	return string.Join('.', list);
}

This is about 20% faster.

[rockfordlhotka]

Interesting. I wonder if that's due to calling ToString twice? Maybe this would also be faster-but-simpler?

      var keyName = property.ToString();
      keyName = keyName.Substring(keyName.IndexOf(").") + 2);

[j055]

Mine is still 11% better.

You don't actually need to do the substring stuff anyway do you? In which case they would take about the same amount of time. I'll keep my method in my 'might be useful one day' toolbox.

[rockfordlhotka]

I don't remember what is before the ")." in the string, so you might be right that the SubString isn't needed at all.

I think it is interesting though, that their ToString is clearly quite a lot slower than your code.

If I may though, my criticism of your code is that it seems like it would be better implemented with a StringBuilder than a List<string>. Whether it would be faster I don't know, but building a string without using StringBuilder just seems ... wrong somehow 🙂

[j055]

You might be right so for completeness here's a StringBuilder one. It is not faster and may be slightly slower.

public static string CreateKey<T>(Expression<T> expression)
{
	var sb = new StringBuilder();
	var member = expression.Body as MemberExpression;
	while (member != null)
	{
		sb.Insert(0, '.');
		sb.Insert(0, member.Member.Name);
		member = member.Expression as MemberExpression;
	}
	return sb.ToString().TrimEnd('.');
}

Your ToString creates something like () => value(UserQuery+<>c__DisplayClass4_0).vm.Model.Child.Name which is probably OK for your key anyway.

[rockfordlhotka]

Oh, that's funny! I took your stringbuilder code and though to myself it would be easier (maybe faster?) to use Append instead of Insert. Of course then everything comes out backwards! 😆

So in the actual codebase I'm using your original List<string> implementation.