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

Spatial_Engine: Create NewElement1D etc. should be Query methods #1572

Closed
al-fisher opened this issue Mar 12, 2020 · 5 comments · Fixed by #3245
Closed

Spatial_Engine: Create NewElement1D etc. should be Query methods #1572

al-fisher opened this issue Mar 12, 2020 · 5 comments · Fixed by #3245
Assignees
Labels
type:compliance Non-conforming to code guidelines

Comments

@al-fisher
Copy link
Member

As discussed here: #1543 (comment)

[...] all of these "New" methods actually should be Queries, not Creates. If we see Create methods as constructors, then these should most definitely not be Creates at least, and seeing it as "I get a new compatible type of a lower level object" rather than "I create a new lower level object, based on this higher level object" I think actually even semantically makes more sense.

The NewElement0D NewElement1D, NewInternalElement2D need to be migrated to Query namespace as above.

This will need to be coordinated with Physical_Engine, Structure_Engine, Environment_Engine and any other IElementXD implementations of those interfaces.

As part of this I would suggest we flatten the folder structures completely. Compliance for for Query should not be concerned with this in any case I do not believe.

FYI @adecler @IsakNaslundBh @FraserGreenroyd @pawelbaran

@al-fisher
Copy link
Member Author

Thinking of these as Query methods should also eschew the prefix "New".
For example being simply:

public static IElement2D InternalElement2D(this Panel Panel);
public static IElement1D Element1D(this Opening opening, ICurve curve);
public static IElement0D Element0D(this Bar bar, Point point);

@IsakNaslundBh
Copy link
Contributor

Thinking of these as Query methods should also eschew the prefix "New".
For example being simply:

public static IElement2D InternalElement2D(this Panel Panel);
public static IElement1D Element1D(this Opening opening, ICurve curve);
public static IElement0D Element0D(this Bar bar, Point point);

I would keep the "New" for this case actually, or some other way to distinguish this. (think this is the reason it ended up in the create in the first palce).

Important to highlight that this is not grabbing anything out from the elements themself, but it is a completely new element being returned, matching the type appropriate for the bar/panels etc. Just calling it Element0D would at least make me think that it would be the endnodes of the bar.

@al-fisher
Copy link
Member Author

I would keep the "New" for this case actually, or some other way to distinguish this. (think this is the reason it ended up in the create in the first place).

Ah I see! I wonder if a word like "Extract" might be better than "New"?
Saying that how is this any different from say Structure.Query.CentreLine(this Bar bar) ? This returns a "New" generated object.

@IsakNaslundBh @adecler

@IsakNaslundBh
Copy link
Contributor

I would keep the "New" for this case actually, or some other way to distinguish this. (think this is the reason it ended up in the create in the first place).

Ah I see! I wonder if a word like "Extract" might be better than "New"?
Saying that how is this any different from say Structure.Query.CentreLine(this Bar bar) ? This returns a "New" generated object.

@IsakNaslundBh @adecler

There is a clear difference here I would say. Structure.Query.CentreLine(this Bar bar) is 100% dependant on the properties of the bar. It is returning a line between the start node and end node positions of the particular bar provided. If you change to another bar, you get another centreline.

This is not the case for the methods in question. The only purpose of the bar is to be able to identify what type of IElement0D to provide, for this case a Node. If you change to another bar, you will get exactly the same result, basically. The content of the bar is completely irrelevant. Job of those methods are basically just to return a new type of the particular sub-object that is compatible with the one provided (and setting the geometry to this sub-object for some of the cases).

A method just called Element0D I would expect to return something that is relating to the Nodes of that particular bar, i.e., different bar -> different result.

Saying all of this, I would be more than happy to change the name to something that makes this a bit clearer, but I would personally not just skip the leading "New".

@Tom-Kingstone
Copy link
Contributor

Currently, most Create.NewElementXD have been marked ToBeRemoved since 3.2, but where there is not a ToBeRemoved attribute, there are some cases where the method has not been moved to the Query class.
I will therefore remove any methods marked ToBeRemoved, and move any methods still remaining in the Create class to the Query class.

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
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

4 participants