Problematic Curvy API

[XtroTheArctic]

Hello.

I just noticed a serious problem in Curvy's code.

I contains extension methods which are considered as "non-best-practice"  and these method leaks into our project code via VisualStudio's IntelliSense because they are declared as public.

In .NET world (or at least, in C# world) we don't/should not use resizable Arrays. When we need resizing, we use List<> generic collection.

I see that Curvy contains extension methods (Add and AddRange) which resize an array. You may decide what to use and not to use in your own code. That's fine but declaring this  non-best-practice as public and leaking it into customer code is a serious problem.

I almost committed the usage of these public methods into our project repo. I just noticed the problem at the last second before committing.

As can be seen in the screenshot, SettlementBuildingOptions is a string array and VS IntelliSense is showing your non-best-practice Add and AddRange methods as suggestions. This can lead to programmer errors on our end.

Can you please reduce the accessibility of those methods to Internal or Private according to your usage please?

-- Last Second Edit --

I just checked. You don't even use these methods privately. Why are you pushing these extension methods into your customers' code?

Please don't tell me to manually modify the accessibility of these methods in your code. Doing so would make my 3rd party maintenance job very hard in the future.

[_Aka_]

Hi

I would like to discuss with you some points before taking any action. I will subdivide my answer in numbered points to make it easier for us to discuss:

1. To clarify, the Array.Add and Array.AddRange extension methods were not wrote by me, and are not part of Curvy Splines. They are part of DevTools, one of Curvy Splines' dependencies. All the dependencies of the asset are in ToolBuddy/Dependencies folder. For now, it contains DevTools, LibTessDotNet and Vector Graphics. The code owned by ToolBuddy is in another folder, ToolBuddy/Assets.
   
   
2. These methods are used in multiple places. As an example:
   AddRange is used in DTSelection.AddGameObjects, which is used in CurvyMenu.Create<T>(MenuCommand cmd)
   So removing these extension methods is not an ideal solution. Setting them private or internal is not neither, since DevTools can be in its own assembly when users use the provided assembly definitions.
   
   
3. You stated that resizable arrays should not be used in C#. I am not aware of such prohibition. Event the Array class has a resizing method, Array.Resize<T>(T[], Int32). Sure it is better in a lot of cases to use lists instead of arrays (knowing that lists use internally an array that is resized), but that does not mean that resizing arrays is prohibited. There are cases where resizing arrays makes sense. It really depends on the context. Can you please guide me to official guidelines prohibiting this usage?
   
   
4. You stated that public extension methods should not be used. I am not aware of this, especially that .Net itself uses public extension methods (LINQ). Can you please guide me to official guidelines prohibiting this usage? As long as the extension methods are useful, in their own namespace, and do what their name suggests, I don't see an issue in them being suggest by intellisense.
   

Please let me know what you think.

Have a nice day

[XtroTheArctic]

Thank you for your detailed answer.

First of all, I should clarify that my intention was not to sound harsh or rude. I'm sorry if it sounded like that. I super respect you because you are the developer of a very complex tool which means more than enough to prove you are a very experienced C# programmer.

I like discussing things in numbered points. Thank you for that too.

1. Hmm. I understand, those methods are from a library which is "3rd party" to you. That's a complicated situation (for my case). Thou, I don't see any folder named "ToolBuddy" when I import Curvy (excluding the Example folder) so I'm little bit confused. DevTools and LibTessDotNet are in Plugins folder next to Curvy but I don't see Vector Graphics anywhere. I don't know why you mentioned Vector Graphics and ToolBuddy folders.

1.b. Those 3rd party extension methods are in FluffyUnderware.* namespace which is your own namespace. How come 3rd party things are declared in your own namespace? I am very confused again.

2. I am sorry for saying "You don't even use these methods". My Resharper tool didn't scan my 3rdParty folder so it couldn't find the usages. Now, I see the usage of AddRange in DTSelection.AddGameObjects. Thank you.

About the usage of a custom extension method: As a fellow C# programmer, I'd like to suggest you to use a similar (and better) extension method from official .NET libraries "Concat". Of-course it's your decision but you can change that line like this without depending on custom extension methods.

                Selection.objects = gl.Concat(cur).ToArray();

3. I didn't mean to sound so harsh. I didn't use the word prohibition or anything like that. I am sorry If I sounded harsh. I just mentioned resizing arrays as "not being a best practice". Of-course we can resize arrays when needed (Which is very rare) and of-course List is using arrays internally (In an performance optimized way, as you know). If you consider the code I suggested in #2, it does the same thing without resizing an existing array. Your version creates 2 arrays but the suggestion creates 1 array. Of-course this is a super tiny improvement which may not justify the discussion time/effort but the suggested one saves you from using a custom dependency, If you would like to consider.

4. Again, I'm sorry if I sounded harsh but I never said "public extension methods should not be used". I said, leaking an extension method into customer code is a problem (Now I know this problem is caused by the 3rd party; not you). Also, when writing this post, I just noticed that I suffered from another Tooling malfunction. Visual Studio added "FluffyUnderware.DevTools.Extensions" namespace to my usings section without telling me because earlier I used another extension method from that namespace by mistake without noticing this leak problem. The extension I used earlier was "IndexOf" method in the same namespace. VS did me a sneaky trick by adding a 3rd party namespace into my code.

5. The developer of this 3rd party "DevTools" made another questionable thing by implementing "IndexOf" as a custom extension method. .NET already provides this method in Array class. Re-inventing an existing .NET method as a custom extension method and providing it as a public solution is not a best practice either. Thou, again, I don't know why this 3rd party stuff is in your own namespace.

Anyways, as a result of our discussion, I saw that I can get rid of the leak problem by removing your namespace containing 3rd party extensions from my usings section and switching to the use of .NET's original Array.IndexOf method.

If you would like to disscuss these numbered points further, I'd be happy to discuss because I enjoy every bit of C# discussion.

One last time, I am very sorry if I sounded harsh or rude. That wasn't my intention.

[_Aka_]

You are welcome. And don't worry about sounding harsh or rude, no offense taken.

1- You seem to be using a version prior to 8.0.0. This version brought a lot of useful features that you might be interested in.
https://www.youtube.com/watch?v=teq2_slOuKo
The folder structure I based my answer on started being used at 8.0.0.

1-b Fluffy Underware is the name under which the creator of Curvy Splines (know at the time simply as Curvy) was publishing his assets. Here is hit github: https://github.com/FluffyUnderware
The creator's name is Jake. In 2017 he couldn't keep supporting Curvy for various reasons. I, a customer of his at the time, bought Curvy from him and kept developing it/publishing it. I publish under the name of ToolBuddy.
https://toolbuddy.net/
DevTools was not part of the deal. Jake kept its intellectual property, planned to make it open source, and gave me permission to use it for Curvy.
To not break any existing project using Curvy, I chose to keep using the FluffyUnderware namespace, with Jake's approval. Even classes that I added to Curvy used the FluffyUnderware namespace, to keep things coherent. I do use the ToolBuddy namespace in my other assets, and in some sections of the code in Curvy Splines, sections that are easily distinguishable from the old code. This gave birth to the ToolBuddy.Pooling.* namespaces in Curvy Splines 8.0.0.

2- In newer versions, there are more usages for those methods.

Thanks for telling me about Concat().ToArray(). I was aware of its existence, but I am unsure about its performance compared to a Resize() then Copy() approach used in the AddRange() extension method. LINQ methods, as you probably already know, tend to do small allocations at each call. I have to investigate this further to make an opinion.

3- I believe that AddRange also leads to the creation of only one array (in Resize), which is the same as for Concat.ToArray. I will have to profile to verify my belief. But in the specific case of the
Selection.objects = cur.AddRange(gl.ToArray());
, yes you have a point, since it avoids the call to gl.ToArray().

I will try to get rid of the AddRange (and possibly Add) extension methods in the future. In general, I tend to remove code from DevTools when I see the opportunity. But, I try to limit the effort and time I put into improving DevTools (and other third party software), since most of the time improving Curvy Spline's code yields better results.

4- This is a VS behaviour that bothers me too. Sure, automatically adding using statements to your code can save a second here and there, but I prefer that VS suggests to me the addition of the using statement, and I have to validate it to be done. Avoids a lot of issues. I kid you not, yad the same issue only yesterday with NavMeshBuilder, which exists in two namespaces:
https://docs.unity3d.com/ScriptReference/AI.NavMeshBuilder.html

5- I agree. It is now removed thanks to your remark. This method was not used in Curvy Splines, so removing it was a no brainier. I did the same with a couple other methods. This should be part of the next update.

Once again, absolutely no offense taken, don't worry at all.
Thanks for your remarks and improvement suggestions, today and in the past.

Have a great day

[XtroTheArctic]

1. Oh my! That's an interesting developer story. That's for sharing it with me. As users of this great tool (I'm a beginner to Curvy) we are lucky you continued its development. Thank you!

2. Yes the enumerator is an allocation but in your version, there is the extra array creation so it counts as an allocation too. I suspect that Concat version should be less expensive (just a little bit) but we can't know without proper testing. It's not a big deal anyways. I just wanted to share how I would avoid resizing arrays. Thanks for the consideration.

3. AddRange creates 1 array (while resizing) if I'm not mistaken.

4. When VS introduced this feature for the first time, I immediately disabled it. Really don't like the idea. After some time, I wanted to give it a chance and I started to like it. Then this AddRange incident happened. Actually IndexOf happened first but I didn't even notice it until AddRange incident. Now I'm considering disabling it again

Thank you for everything, especially for the friendly chat!

[_Aka_]

You are welcome, my pleasure

[SAMYTHEBIGJUICY]

super cool to read about the origins of curvy and toolbuddy thanks for sharing!

[SomeGuyEight]

super cool to read about the origins of curvy and toolbuddy thanks for sharing!

Thank you for posting you response and bumping this  As a very new programmer, this thread is a great look at how more experienced programmers reason through their code. 

Thanks Aka & Xtro for the insightful discussion

[_Aka_]

You are welcome.