Why is Dataportal_Create an override while other Dataportal_XYZ are not ?Why is Dataportal_Create an override while other Dataportal_XYZ are not ?
Old forum URL: forums.lhotka.net/forums/t/8230.aspx
lukky posted on Tuesday, December 29, 2009
Hi Rocky,
I was watching CSLA Core video #3 at about 44:00 minutes, and it fell on me that Dataportal_Create is implemented in the BO as an override, while all other Dataport_XYZ are not.
I had never noticed it since I use a code generator and never had to code a BO manually. The templates I use do follow this pattern, I just never noticed.
The question is why the different approach for Create ? I've looked in the book, but it doesn't seem to be explained.
Thanks
RockfordLhotka replied on Tuesday, December 29, 2009
In the early days of CSLA (pre 2005) the data portal only had one method signature for each DP_XYZ method - and they all took parameters of type object.
Starting with CSLA .NET 2.0, the data portal got a lot smarter, and implements method overloading, so your DP_XYZ methods are strongly typed and you can overload them. I think that was a huge improvement.
But in the 1.x days I'd implemented protected methods for the only set of valid signatures to make it easier, and I didn't remove them because I didn't want to break existing code.
I think that was a mistake, because it leads to confusion. On the other hand, DP_Create is a special case, because 90% of the time people don't implement it, and so having a base implementation is actually useful.
So in 4.0 I may very well remove the protected virtual DP_Fetch/Update/Insert/etc - but I'll probably still leave DP_Create because it is actually useful.
Or in 4.0 I may just leave it all alone, because the confusion isn't that great, and backward compatibility is terribly nice.
rxelizondo replied on Tuesday, December 29, 2009
RockfordLhotka:
But in the 1.x days I'd implemented protected methods for the only set of valid signatures to make it easier, and I didn't remove them because I didn't want to break existing code.
I think that was a mistake, because it leads to confusion.
No doubt, I totally agree with you, it was a mistake to keep them :)
RockfordLhotka:
So in 4.0 I may very well remove the protected virtual DP_Fetch/Update/Insert/etc - but I'll probably still leave DP_Create because it is actually useful.
Yes, *PLEASE* remove them and avoid the confusion. And when I when I say remove them I mean *ALL* of them. This way we will be consistent in a manner that if a user doesn’t implement a needed DataPortal_XYZ, the CSLA will throw an error for all of them and not just some.
I would personally prefer to have a class level attribute that was something like [UseDefaultDataPortalCreate] for the class to use a predefine pluming... And yes, I realize that nobody will like it and even if they did the effort to add that feature is not worth it and blah, blah it but I wanted to express my opinion anyway :)
RockfordLhotka:
Or in 4.0 I may just leave it all alone, because the confusion isn't that great, and backward compatibility is terribly nice.
Now what makes you think that the confusion isn’t
that great Rocky? I had the same question for a while only I never care to post the question because I figured that the reason why that was left like that was due to the worst thing that could happen to software development and humanity known as “backward compatibility” or should we say “awkward compatibility”?
Changing things may break “backward compatibility” but it won’t break “feature compatibility”, people will still be able to accomplish the same thing if you where to remove the DataPortal_XYZ methods, all they will have to do is to implement it manually. Big deal, copy and paste, get over it.
And yes Andy, I know I am not the only one using the CSLA ;)
RockfordLhotka replied on Tuesday, December 29, 2009
Since the base DataPortal_Create() actually does something real, removing it seems wrong. Any base method that actually does something should remain in the base, and should (maybe) be virtual.
The other DP_XYZ methods in the base don't do anything - they just throw exceptions. That has no value any longer (and hasn't since 2.0), so removing them is clean-up work.
ajj3085 replied on Wednesday, December 30, 2009
Ya, I'd like to see the Create stay. In the cases where you don't need to initialize everything (which I imagine is fairly common), the default implementation makes sense, and removing it would just create a lot of busy non-productive work for everyone.lukky replied on Wednesday, December 30, 2009
RockfordLhotka: Since the base DataPortal_Create() actually does something real, removing it seems wrong. Any base method that actually does something should remain in the base, and should (maybe) be virtual.
The other DP_XYZ methods in the base don't do anything - they just throw exceptions. That has no value any longer (and hasn't since 2.0), so removing them is clean-up work.
Rocky,
Correct me if I'm wrong, but the only thing that the base implementation of Dataportal_Create does is call ValidationRules.CheckRules().
Couldn't this be done by the Dataportal itself, just like calling MarkOld() in Dataportal.Fetch() ?
RockfordLhotka replied on Wednesday, December 30, 2009
lukky:
[...] the only thing that the base implementation of Dataportal_Create does is call ValidationRules.CheckRules(). Couldn't this be done by the Dataportal itself, just like calling MarkOld() in Dataportal.Fetch() ?
That's true, and that is a valid point.
ajj3085 replied on Wednesday, December 30, 2009
Is there a case where someone wouldn't want the rules to run though? As it is, you can override and do nothing. If moved, you'd lose that ability. Of course maybe that's not an issue.
Although I do have some code where in the Create or Fetch, I set a flag prior to calling ValidationRules.CheckRules, so that some rules don't process in those methods, because they should only run in response to the user changing properties. That could screw me up..
lukky replied on Wednesday, December 30, 2009
ajj3085: Is there a case where someone wouldn't want the rules to run though? As it is, you can override and do nothing. If moved, you'd lose that ability. Of course maybe that's not an issue.
Although I do have some code where in the Create or Fetch, I set a flag prior to calling ValidationRules.CheckRules, so that some rules don't process in those methods, because they should only run in response to the user changing properties. That could screw me up..
That is a valid point. In any case, I'm not really advocating a change right now. My only concern was that I didn't understand the reason behind having DP_C be different than the other DP_XYZ methods.
Now that I understand the reason behind it, it doesn't puzzle me. The thing is though, new users will probably have to go through the same questionning over and over. In this case, it might be more elegant to have a pattern that exposes more clearly the intention.
For example, Dataportal.Create() could have an overload with a boolean flag to indicate whether to execute validation rules or not. The default parameterless method simply calling it with "true". I don't know. What do you think ?
On the other hand, I wouldn't mind having to manually call ValidationRules.CheckRules in DP_C, just as I have to in DP_F. This would standardise the way we have to write our DP_XYZ methods.
ajj3085 replied on Wednesday, December 30, 2009
lukky:That is a valid point. In any case, I'm not really advocating a change right now. My only concern was that I didn't understand the reason behind having DP_C be different than the other DP_XYZ methods. Now that I understand the reason behind it, it doesn't puzzle me. The thing is though, new users will probably have to go through the same questionning over and over. In this case, it might be more elegant to have a pattern that exposes more clearly the intention. For example, Dataportal.Create() could have an overload with a boolean flag to indicate whether to execute validation rules or not. The default parameterless method simply calling it with "true". I don't know. What do you think ?
That wouldn't help me; its not that I don't want rules to run, its that I don't want SOME rules to run. So the rules which are validation always run, but the other rules check that flag (which is only set during the Create or Fetch methods).
rxelizondo replied on Wednesday, December 30, 2009
lukky:
Correct me if I'm wrong, but the only thing that the base implementation of Dataportal_Create does is call ValidationRules.CheckRules().
Thats not the only thing that the default Dataportal_Create method does, it also instructs CSLA to run the method locally [RunLocal]. I believe that if you don't have that attribute there the CSLA will make a call to the server for nothing (of course this may only really matter if you have the server on different physical tier).
ajj3085:
the default implementation makes sense, and removing it would just create a lot of busy non-productive work for everyone.
Andy,
It is NOT non-productive work for everyone. The key here is that we will have consistency on how all the DataPortal_XYZ behave. Inconsistence is the root of all evil.
You and I and a handful or other people may know exactly whats going on behind the scenes but PLEASE CONSIDER the people that are new to the CSLA.
Yes, I agree that is kind of awkward to have to include a method that does nothing or some method that its just pluming but it is also awkward to have automagical code doing things behind your back that you are not aware of until you run into an issue and spend 2 hour trying to figure the issue out.
As I have stated before, my main motivation for some of this changes is to try to make it easier for new people adopting the CSLA to get up to speed and have it easier, I am not just trying to piss people off. I already know the stuff, if it was just about me I could not care less if thing changed or not.
tmg4340 replied on Wednesday, December 30, 2009
Andy can certainly step in and tell me if I'm wrong, but I believe his "non-productive work" comment relates to your apparent distaste for the concept of "backwards compatability" - namely, that you seem to be standing up and shouting that backwards compatability is a terrible concept that should be summarily abandoned.
(I also don't happen to agree with your "inconsistency is the root of all evil" comment, but that's a different discussion.)
This is the rock upon which you will split with a lot of developers. Forcing me to go back and change code that is already working in a production environment just because you don't happen to like a method signature is quite frankly asking a lot. You're telling me that I have to go in and potentially introduce an error in code to "fix" something that is already working - and that gains me nothing in doing so. Sure, I might save a developer down the road some maintenance time because they might not have to scratch their head and wonder what's going on. But all that code changing, testing, and pushing to production that I would have to do to my entire CSLA-based codebase in order to conform to "the new rules" is time I have to spend that does not move my projects forward one inch, as far as my customers are concerned.
And yes, I realize that there's nothing forcing me to move to a new version of the framework. I could happily continue in my world with whatever version I'm on, dealing with all the inconsistencies/issues/etc. that exist there. But code that stays stagnant is eventually replaced, becomes unsupported, or any of a hundred other possibilities that makes my life harder. I cannot bury my head forever.
We all realize that upgrading to a new version of something can take time, and usually our customers understand that as well. And I have nothing against cleaning up an API. But the easier the transition to the new API is, the better it's going to go. Backwards compatability is not the most important concept in developing a framework - but it's still an important one.
- Scott
lukky replied on Wednesday, December 30, 2009
I'm a bit humbled to see all the buzz around my question :-)
While I do understand the need to preserve existing code base, I think it raises the question if we should maintain backward compatibility forever and a day.
With the coming release of CSLA.NET 4.0, I see a great opportunity to clean up the API. Let's face, whether we want it or not, 4.0 is going to introduce breaking changes. Let's just consider the splitting of the collections between WinForms and WPF/Silverlight, and I'm sure that'll introduce breaking changes.
Would it be that bad to have a fresh start ? I see 4.0 as a major release, one that will probably tie Rocky's hands for many years. Shouldn't we welcome the opportunity ?
Just my $0.02 :-)ajj3085 replied on Wednesday, December 30, 2009
I think Scott summed up the argument pretty well.
And for those of us that have been with Csla for a while, I don't think any of us are against breaking changes, so long as those changes get us something good in return. There have been a number of times a breaking change is proposed, everyone knew it would be painful, but the gain was deemed well worth it.
A slightly cleaner API, which only benefits you at the start of learning Csla, well, there are other things I'd rather Rocky focus on. Especially when this particular issue is easily solved by using the material that's out there. Its one of the things you learn when you learn Csla; you learn how to create DataPortal_xyz methods, via the book or videos.
MadGerbil replied on Wednesday, December 30, 2009
Interesting discussion.
It has confused me a bit though because in the 3.8.1 Templates section the EditableRoot.cs has this code:
protected override void DataPortal_Create()
{
// TODO: load default values
// omit this override if you have no defaults to set
base.DataPortal_Create();
}
private void DataPortal_Fetch(SingleCriteria criteria)
{
// TODO: load values
}
[Transactional(TransactionalTypes.TransactionScope)]
protected override void DataPortal_Insert()
{
// TODO: insert values
}
[Transactional(TransactionalTypes.TransactionScope)]
protected override void DataPortal_Update()
{
// TODO: update values
}
[Transactional(TransactionalTypes.TransactionScope)]
protected override void DataPortal_DeleteSelf()
{
DataPortal_Delete(new SingleCriteria(this.Id));
}
[Transactional(TransactionalTypes.TransactionScope)]
private void DataPortal_Delete(SingleCriteria criteria)
{
// TODO: delete values
}
As you can see, the DataPortal_Update, Insert, and DeleteSelf all use the override. Is the template incorrect? What am I missing here?rxelizondo replied on Wednesday, December 30, 2009
tmg4340:
Forcing me to go back and change code that is already working in a production environment just because you don't happen to like a method signature is quite frankly asking a lot. You're telling me that I have to go in and potentially introduce an error in code to "fix" something that is already working - and that gains me nothing in doing so.
I never said to make a change just because “I don’t like a method signature”, my argument for the change was to gain consistency and therefore potentially making the CSLA code easier to understand. For you that may mean very little, for me that is gold.
Also note that I would not want these type of changes done from within minor CSLA version, this types of changes could fit very well when going from CSLA 1.0 to 2.0 to 3.0 to 4.0 etc.
tmg4340:
Backwards compatability is not the most important concept in developing a framework - but it's still an important one
And I agree with you, thats why I made a distinction before between my *personal* definitions of "backwards compatibility" and "feature compatibility". If Rocky was to decide to no longer support DataPortal_XYZs and totally removed the methods from the framework AS WELL as no longer support that concept then I would consider that to break "backward compatibility". Bu that is not really the case here is it?, I mean, things would remain the same and the concept would still be supported with the exception that now you would have to make a *minor* code change and to me that is preserving "feature compatibility" which is what I believe its whats most important....... Ok, yes I get it, I get it, it still breaking "backwards compatibility" but I hope I did a good enough job to get my point across.
Also note that this is not the only place where the CSLA exhibits inconsistent/awkward behavior due to backward compatibility, too many of those and things can start to get complicated.
Listen guys, I do understand where you guys are coming from, I do respect your opinion, but like lukky said it, this is a great opportunity to move forward an make things better (assuming changes like this are really improvements), why not embrace it?
And by the way, for all of those that said the Rockys time would be best off spent implementing new feature, I would ask Rocky a simple question, how much time do you spend dealing with backward compatibility issues while supporting the code? I bet it must be significant, such time could be spent doing those new and worthy improvement. Also, how ofter have you had to settle for a mediocre implementation of a feature for the sake of preserving backwards compatibility?
Thank you.
RockfordLhotka replied on Wednesday, December 30, 2009
Yeah, I still tend to think it is best to leverage the power of OOP rather than relying overmuch on data portal magic.Copyright (c) Marimer LLC
