0

I have multiple methods calling each other to simplify changing anything in the code and to avoid fixing errors and copy pasting. It looks like this:

Methods linking

  1. Is this a bad practice?
  2. Does it cause too much overhead from experience? (I don't have any performance problems but I try to do best in every case I can)
  3. Is this a good solution to make a private inline methods along with public ones? And call them when required?

Example Code

#if UNITY_EDITOR
public static class ScriptableObjectUtility
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static T CreateAssetInline<T>(string path, string tagName, T asset)
        where T : ScriptableObject
    {
        if (path == string.Empty)
            path = PathUtility.ASSETS_PATH_NAME;
        else if (!string.IsNullOrEmpty(Path.GetExtension(path)))
        {
            path = path.Replace(Path.GetFileName(AssetDatabase.GetAssetPath(Selection.activeObject)), string.Empty);
        }

        string fullGeneratedPath = AssetDatabase.GenerateUniqueAssetPath(Path.Combine(path, $"[{tagName}] name.asset"));

        AssetDatabase.CreateAsset(asset, fullGeneratedPath);

        AssetDatabase.SaveAssets();
        AssetDatabase.Refresh();
        EditorUtility.FocusProjectWindow();

        Selection.activeObject = asset;

        return asset;
    }

    public static T CreateAsset<T>(string path, string tagName, T asset)
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline(path, tagName, asset);
    }
}
#endif

Final Version

This is a version where all public methods make a call to their corresponding inlined methods.

#if UNITY_EDITOR
public static class ScriptableObjectUtility
{
    /// <summary>
    /// 
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="path"></param>
    /// <param name="tagName"></param>
    /// <param name="asset"></param>
    /// <returns></returns>
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static T CreateAssetInline<T>(string path, string tagName, T asset)
        where T : ScriptableObject
    {
        if (path == string.Empty)
            path = PathUtility.ASSETS_PATH_NAME;
        else if (!string.IsNullOrEmpty(Path.GetExtension(path)))
        {
            path = path.Replace(Path.GetFileName(AssetDatabase.GetAssetPath(Selection.activeObject)), string.Empty);
        }

        Directory.CreateDirectory(path);

        string fullGeneratedPath = AssetDatabase.GenerateUniqueAssetPath(Path.Combine(path, $"{tagName}.asset"));

        AssetDatabase.CreateAsset(asset, fullGeneratedPath);

        AssetDatabase.SaveAssets();
        AssetDatabase.Refresh();

        EditorUtility.FocusProjectWindow();

        Selection.activeObject = asset;

        return asset;
    }

    public static T CreateAsset<T>(string path, string tagName, T asset)
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline(path, tagName, asset);
    }

    /// <summary>
    /// Makes a copy of instance T and creates its asset at path.
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="instance"></param>
    /// <param name="path"></param>
    /// <param name="tagName"></param>
    /// <returns></returns>
    public static T CreateAsset<T>(T instance, string path, string tagName)
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline<T>(path, tagName, Object.Instantiate(instance));
    }

    // (string path, string tagName)

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static T CreateAssetInline<T>(string path, string tagName)
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline<T>(path, tagName, ScriptableObject.CreateInstance<T>());
    }

    public static T CreateAsset<T>(string path, string tagName)
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline<T>(path, tagName);
    }

    // (string path)

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static T CreateAssetInline<T>(string path)
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline<T>(path, typeof(T).ToString());
    }

    public static T CreateAsset<T>(string path)
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline<T>(path);
    }

    // 

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static T CreateAssetInline<T>()
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline<T>(AssetDatabase.GetAssetPath(Selection.activeObject));
    }

    public static T CreateAsset<T>()
        where T : ScriptableObject
    {
        return ScriptableObjectUtility.CreateAssetInline<T>();
    }
}
#endif
  • I don't think there's anything wrong with this, but it appears to be (albeit localized) overkill as you're only using the inline method in one place for now. We can never really be sure if the runtime/jit will actually inline, but fyi, these functions are also logically eligible for tail call optimization as well, though we similarly can't be sure the runtime will do it on its own (e.g. without the compiler asking for it). – Erik Eidt Feb 18 at 23:38
  • @ErikEidt I was hoping that [MethodImpl(MethodImplOptions.AggressiveInlining)] would definitely inline stuff ignoring compiler own decision on inlining because of how the option is set. In the current version, I have created a private inline method for each method, and those inline methods call each other, thus I was hoping they would all be just plain code after compilation but you wouldn't be able to use them outside of the class to avoid inlining from outside that class. – Candid Moon _Max_ Feb 18 at 23:43
  • Note that even when a method isn't annotated with [MethodImpl(MethodImplOptions.AggressiveInlining)], it might still be inlined. And if a method is a good inlining candidate, you would usually want it to be inlined for all references, not just local ones. – Clockwork-Muse Feb 18 at 23:51
  • 2
    If you are saying you are concerned about stack depth because you have a couple of methods calling each other - don't be. For what you are doing, this is not a concern at all and it's not even worth thinking about. Trying to "optimize" this is a literally a waste of time; you don't gain anything of consequence, but you pay in maintainability. – Filip Milovanović Feb 19 at 0:32
  • 2
    In practice, your stack depth is going to be so large from things outside your control (the rest of Unity, here), adding an extra level or two at this point (especially with something that's trivially inlinable) isn't worth being concerned about. Write methods based on one primary criteria - modularity (not, as often taught, for reusability). If profiling points to this as being a pain point, then (and only then) worry about whether to inline parts of a method. – Clockwork-Muse Feb 19 at 0:36
2
  1. Despite the nest chains of calls that provide default parameters, you actually only call the inlined method once. Unless you're planning on implementing the Strategy pattern, or having multile other sibling methods call it, wrapping the private method is probably not needed.
  2. Overhead in which sense? App Size? Your executable code will be noticeably larger than otherwise. Stack consumption? Probably not, unless your objects are very large (as in the range of 100's of kB each). Execute time? Again, probably not, as your stack depth isn't very large, and there aren't any loops involved at this level.
  3. The call isn't generally when to wrap a private inline method with a public method, but whether to wrap a private method with a public method. Whether a method is inlined or not is generally an optimization question (profile first!), and is usually best reserved for very small bits of code that will be called very frequently. Given that this method is making a call to a database, it's not what I would consider a good candidate for inlining. 15 lines is probably short enough, given there's only one test and no loops.
  • 1. I was planning on making a private inline method for each public one, or use the main one with different pre-set arguments. 2. Stack consumption. Thanks for all mentioned points. 3. Makes sense. - The main question for me is - if it's a bad practice to write such code because it would look ugly and not really justified performance wise - to have privates inline that are exposed by standard public methods that may be or may not be inlined by compiler. – Candid Moon _Max_ Feb 19 at 0:27
  • 1
    Re your question on 3: What you have isn't ugly (in the aesthetic sense), it's just (as others have noted) not needed. In this case, what you have is an instance of what I'd call "Theoretical Pre-Optimization", which is generally a Bad Thing. However, your particular instance of it if, frankly, relatively benign. This probably wouldn't even break the top 10 issues in a code review. If you're looking for an explicit recommendation/solution: the private's fine (especially if you plan to have multiple different versions), but I wouldn't inline it. – John Meehan Feb 26 at 23:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.