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

BHoM_Adapter: refactoring-remove NextId and AssignId #115

Closed
Tracked by #84
alelom opened this issue Jul 5, 2019 · 4 comments
Closed
Tracked by #84

BHoM_Adapter: refactoring-remove NextId and AssignId #115

alelom opened this issue Jul 5, 2019 · 4 comments
Assignees
Labels
type:compliance Non-conforming to code guidelines type:feature New capability or enhancement

Comments

@alelom
Copy link
Member

alelom commented Jul 5, 2019

Proposed actions

  • Remove the AssignId method completely.
  • Id assignment to be done always in the Create method.
  • Remove the NextId method completely.

Explanation

The AssignId and NextId methods enforce on the user an additional structure that may not be needed.

We propose a shift of the Id assignment to the Create method in all cases.
This leads to simplification of the NextId method that can be removed as well.

This also helps clarifying the responsibility of the Create action.

@alelom alelom added the type:compliance Non-conforming to code guidelines label Jul 5, 2019
@alelom alelom self-assigned this Jul 5, 2019
@alelom alelom changed the title BHoM_Adapter: refactoring-remove the NextId method BHoM_Adapter: refactoring-remove the NextId method - id assignment to be done always in the Create() Jul 5, 2019
@alelom alelom changed the title BHoM_Adapter: refactoring-remove the NextId method - id assignment to be done always in the Create() BHoM_Adapter: refactoring-remove NextId and AssignId Jul 5, 2019
@alelom alelom added this to the BHoM 2.5 β MVP milestone Aug 20, 2019
@alelom
Copy link
Member Author

alelom commented Sep 26, 2019

After chat with @adecler and @kThorsager :

  1. initally we thought to move the method in the Create() just to facilitate its implementation.
  2. However, after a discussion on Load assignment, we decided to evaluate how to store the IDs in the Adapter itself to avoid issue risen after the DeepClone
  3. This will be done thorugh a Dictionary<Type, Dictionary<string,object>> adaptersIds where:
    • Type is the type of the Adapter.
    • Dictionary<string,object>: the string key is the id of the object; the value object is the IBHoMObject reference.
      I believe this adaptersIds dictionary should be static, so it can be shared among all adapters.
      Potentially we could use a KeyedCollection like for the Fragments.

@alelom
Copy link
Member Author

alelom commented Nov 13, 2019

Action needed for Toolkits:

  • If you had Config.UseAdapterId set to true, AND you are implementing the Create() method (e.g. it's not empty):
    • The NextId() method is now deleted in the base Adapter. The logic of the Id assignment should now go directly in the Create() method. It is not called automatically anymore by the base Adapter.
      You can either:
      • a) Put the logic for the id assignment in your Create() method(s) as appropriate, before creating the objects.
      • b) Alternatively, keep the existing logic. To do this, you need to:
        • b1) Keep the NextId() as is, but removing the override from its signature
        • b2) Appropriately call the NextId() at the start of your Create() method(s).
          This is needed because the AssignId() method, that was in the base Adapter and was responsible for calling NextId(), has been deleted. You need to place its logic in your toolkit just before the object creation.
          In short, you need to put the following as the first bit of code in your Create() method override:
        // (Feel free to wrap this bit into a method like `AssignId<T>(IEnumerable<T> objects) where T : IBHoMObject`)
        bool refresh = true;
        foreach (var item in objects)
        {
            if (!item.CustomData.ContainsKey(AdapterId)) 
            {
                    item.CustomData[AdapterId] = NextId(item.GetType(), refresh);
                    refresh = false;
            }
        }    

@alelom
Copy link
Member Author

alelom commented Nov 13, 2019

An issue is that it becomes difficult to live with this:

if (Config.UseAdapterId)
{
// Map Ids to the original set of objects (before we extracted the distincts elements from it)
IEqualityComparer<T> comparer = Comparer<T>();
foreach (T item in objectsToPush)
item.CustomData[AdapterId] = newObjects.First(x => comparer.Equals(x, item)).CustomData[AdapterId];
}

that should be either done in all cases (slow because you need to check if Ids have been assigned for each object) or altogether removed.

I need to experiment with the Adapter ID dictionary.

@alelom
Copy link
Member Author

alelom commented Dec 4, 2019

Strategy changed in #164

@alelom alelom closed this as completed Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:compliance Non-conforming to code guidelines type:feature New capability or enhancement
Projects
None yet
Development

No branches or pull requests

1 participant