PR for a CreateOrFind method?

[benkoshy]

Hi Wolfgang,

Would you accept a PR for the following:

Before

Group test = db.Groups.Create("test");

// some time later:
Group test = db.Groups.Create("test"); // Group already exists - exception thrown.

// to avoid the exception:
Group test;

if (GroupExists(test) ){
   // it exists. Let's find it:
    test = findTestGroup
}
else
{
     test = new Group(...)
}
// kind of a pain to write such checks

After

Group test = db.Groups.CreateOrFind("test");
test.Clear();  // 
test.Add(entities);

Without it, we'd have to add a conditional to check if the Group exists, and if not, to return the appropriate result. The proposal is a very Rails paradigm - which is very handy to use. Now that I think about it, it could probably apply to any of the named objects dictionaries.

Please LMK your thoughts?

Ben

[benkoshy]

Consider the following:

                    IEnumerable<Entity> entities = polylines.Cast<Entity>();
                    IEnumerable<ObjectId> ids = entities.Select(e => e.ObjectId);
                    ObjectIdCollection idCollection = new ObjectIdCollection(ids.ToArray());

Before

                    Group gp = db.Groups.Element("1", true);
                    gp.Clear();
                    gp.InsertAt(0, idCollection); // inserting at a specific index, must use ObjectIdCollection 

After

                    Group gp = db.Groups.Element("1", true);
                    gp.Add(ids);        // add IEnumerable of ObjectId
                    gp.Add(entities);   // add IEnumerable of Entities
                                          // caller should not worry about inserting at a specific index, 
                                          // or passing ObjectIdCollections

Perhaps this could be an extension method on the Group class?
Thoughts? Would you reject a PR on this problem?

[wtertinek]

This could probably be a solution: https://stackoverflow.com/a/61991596

[benkoshy]

Chrs. I'm having trouble linking the file to the Extensions folder.

[benkoshy]

This is what I have done so far: #17 - would appreciate a review?

[wtertinek]

The file has to go into src\Sources\Linq2Acad\Extensions. Then, in Visual Studio, right click on the Extensions folder and choose to add an existing item. Select the file in the dialog and then select the other option in the "drop-down button" (should be something like "Add as link").

[benkoshy]

The file has to go into src\Sources\Linq2Acad\Extensions. Then, in Visual Studio, right click on the Extensions folder and choose to add an existing item. Select the file in the dialog and then select the other option in the "drop-down button" (should be something like "Add as link").

For some curious reason it is not referencing. I might have to come back to this

[wtertinek]

Hi Ben,

thanks, interesting idea. I agree that writing checks is a pain sometimes. In the above case the check could be simplified to something like this:

var group = db.Groups.Contains("Test")
              ? db.Groups.Element("Test")
              : db.Groups.Create("Test");

By implementing a method like CreateOrFind, in my opinion, it saves too little code on the client side, so I think the new method on the API (CreateOrFind) side would not be justfied.

But, on the other side, CreateOrFind is a perfect candidate for an extension method (in the client code):

public static class GroupContainerExtensions
{
  public static Group CreateOrFind(this GroupContainer container, string groupName)
    => container.Contains(groupName)
         ? container.Elemenet(groupName)
         : container.Create(groupName);
}

So in my opinion an extension method is definitively the way to go.

[benkoshy]

Hi Wolfgang,

I wish to throw in my final two cents based on your response:

By implementing a method like CreateOrFind, in my opinion, it saves too little code on the client side, so I think the new method on the API (CreateOrFind) side would not be justfied.

The idea being: only a little bit of client code is saved, rendering a public API not justifiable. I do agree - it saves little code. In response, I would adduce my own experience: in rails - which are full of such handy methods and also in a code sample within Linq2Acad - small code saved here and there is very pleasing - to me and aruably to others.

I merely propose this as an idea, and leave it to your discretion to reject or accept it, as you see fit.

rgds

Ben

[wtertinek]

Hello Ben,

sorry for the late reply. Thanks for contributing so many ideas to this project. I understand what you mean and would like to respond with a few thoughts on API design:

I totally agree with you that it is very nice to save a bit of code every now and then. For that, a method like CreateOrFind is definitely helpful. However, in order to become a part of the API a method has to meet some criteria.

One of them is that if a method saves code in the client code, the amount of code saved should be worth the effort (we already had that).

Another one is that the method should be generally applicable. CreateOrFind, in my opinion, covers a rather special use case, where, as the name suggests, you either want to create an element, or find one. This is very special in my opinion. The two separate use cases to create an element (Create) and to find an element (ElementAt) are broadly applicable. I think it's better to keep them separate.

Another criterion is that the preconditions and the result of a method call should be explicit, intuitive and unambiguous. No questions should remain. For example, Create throws an exception if an element with the same name already exists (the precondition is violated). If no exception would be thrown and the method would return an element, there would be an ambiguity and a question would remain: was the element created or was an existing one retrieved? And I think the answer to the question is important. Because exactly the same question remains after a call to CreateOrFind. After calling CreateOrFind on a Group the remaining question would be: is the group now empty, or does it contain entities? And there is a significant difference if the group is empty or not. If the group was newly created, it is certainly empty. If it already existed, it could contain entities. But since we don't know that, we have to (if I interpret your example correctly) remove all possibly contained elements of the group, because the group should be empty, because it should always be refilled. And this refilling has to be done after each call of CreateOrFind, so actually a CreateOrFindAndClear would be the right thing to do (and would be even more special than CreateOrFind). But the real problem here is: because of the ambiguity created, we are calling code that we would otherwise never call, namely the Clear method on a newly created Group. That's the point I'm making.

// Doing it the "normal" way

Group group = null;

if (db.Groups.Contains("Group1"))
{
  group = db.Groups.ElementAt("Group1");
  group.Clear();
}
else
{
  group = db.Groups.Create("Group1");
}

// Using CreateOrFind

var group = db.Groups.CreateOrFind("Group1");
group.Clear();  // Here we call Clear() on the newly created Group (most probably without thinking about it)

Of course, one might say that the Clear call on the newly created group doesn't matter, because it obviously has no effect. That's true. Why bother? Because the class Group inherits countless methods where it is not impossible that a call on a newly created or a retrieved object has different effects.

All this may sound very theoretical and detailed. Well, it is to some extent. The point I'm making is that ambiguities in APIs are often the origin of a lot of errors in client code that would not have occurred if the API had been designed more carefully.

As for CreateOrFind in Rails, unfortunately I can't say much about that, I've never developed in Ruby/Rails. It may be that ambiguities don't play such a big role in dynamic languages ike Ruby or in frameworks like Rails. Or maybe they do and are accepted in favor of faster general development speed. I don't know.

Which now brings me to my original answer: CreateOrFind is a wonderful extension method - in client code. Because the one who uses the method also implements it. The ambiguity is accepted because it only affects one's use case and one knows it well.

tl;dr

• CreateOrFind is too special for a general API
• The result of CreateOrFind is ambiguous, ambiguity is bad

Ben, thanks for your work and the interesting discussions!

-Wolfgang

[benkoshy]

I appreciate the response. The discussion of ideas in a marketplace enables maintainers to make the best decision possible, fully apprised of all the facts - that has always been my goal. Use cases / benefits / costs are hidden, and can only be drawn out with discussion. We have done that and I consider the enterprise successful. I have nothing further to add: we have a perfect meeting of the minds, though we might differ on the conclusion.

BR,

Ben