Code cleanup

+ Private fields should have an underscore as prefix
+ Simplified the implementation of the value converter
+ etc.

Author: abjerner

src/Skybrud.Umbraco.MultiNodeTreePicker/PropertyEditors/MntpEditor.cs

@@ -10,14 +10,14 @@ public class MntpEditor : MultiNodeTreePickerPropertyEditor {
 
         internal const string EditorView = "contentpicker";
 
-        private readonly IIOHelper iOHelper;
+        private readonly IIOHelper _iOHelper;
 
         public MntpEditor(IDataValueEditorFactory dataValueEditorFactory, IIOHelper iOHelper) : base(dataValueEditorFactory, iOHelper) {
-            this.iOHelper = iOHelper;
+            this._iOHelper = iOHelper;
         }
 
         protected override IConfigurationEditor CreateConfigurationEditor() {
-            return new MntpConfigurationEditor(iOHelper);
+            return new MntpConfigurationEditor(_iOHelper);
         }
 
     }

src/Skybrud.Umbraco.MultiNodeTreePicker/PropertyEditors/ValueConverters/MntpValueConverter.cs

@@ -20,13 +20,13 @@ public class MntpValueConverter : MultiNodeTreePickerValueConverter {
 
         #region Constructors
 
+        private readonly IMemberService _memberService;
         private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor;
-        private readonly IUmbracoContextAccessor umbracoContextAccessor;
         private readonly MntpConverterCollection _converterCollection;
 
         public MntpValueConverter(IPublishedSnapshotAccessor publishedSnapshotAccessor, IUmbracoContextAccessor umbracoContextAccessor, IMemberService memberService, MntpConverterCollection converterCollection) : base(publishedSnapshotAccessor, umbracoContextAccessor, memberService) {
             _publishedSnapshotAccessor = publishedSnapshotAccessor ?? throw new ArgumentNullException(nameof(publishedSnapshotAccessor));
-            this.umbracoContextAccessor = umbracoContextAccessor;
+            _memberService = memberService;
             _converterCollection = converterCollection;
         }
 
@@ -43,43 +43,48 @@ public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType
         }
 
         public override object ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object source, bool preview) {
-            return source?.ToString()
+            return source?.ToString()?
                 .Split(new[] { "," }, StringSplitOptions.RemoveEmptyEntries)
                 .Select(UdiParser.Parse)
                 .ToArray();
         }
 
         public override object ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object source, bool preview) {
 
-            object value = GetPickerValue(owner, propertyType, cacheLevel, source, preview);
-
-            bool single = IsSingleNodePicker(propertyType);
-
-            if (propertyType.DataType.Configuration is MntpConfiguration config && config.ItemConverter != null) {
-
-                string key = config.ItemConverter.GetString("key");
-
-                if (_converterCollection.TryGet(key, out IMntpItemConverter converter)) {
-
-                    if (value is IPublishedElement element) return converter.Convert(propertyType, element);
-
-                    if (single == false) {
-
-                        Type type = converter.GetType(propertyType);
-
-                        return (value as IEnumerable<IPublishedElement> ?? new IPublishedElement[0])
-                            .Select(x => converter.Convert(propertyType, x))
-                            .Cast(type)
-                            .ToList(type);
-
-                    }
+            // gets the picked items as IPublishedContent
+            object value = GetPickerValue(propertyType, source, preview);
+
+            // Return "value" if the data type isn't configured with an item converter
+            if (propertyType.DataType.Configuration is not MntpConfiguration config) return value;
+            if (config.ItemConverter == null) return value;
+            
+            // Get the key of the converter
+            string key = config.ItemConverter.GetString("key");
+            
+            // Return "value" if item converter wasn't found
+            if (!_converterCollection.TryGet(key, out IMntpItemConverter converter)) return value;
+            
+            switch (value) {
+                
+                // If "value" is a single element, we can call the converter directly
+                case IPublishedElement element:
+                    return converter.Convert(propertyType, element);
+
+                // If "value" is a list of elements, we call the converter for each item, and make sure to return a
+                // list of the type specified by the converter
+                case List<IPublishedElement> elements:
+                    Type type = converter.GetType(propertyType);
+                    return elements
+                        .Select(x => converter.Convert(propertyType, x))
+                        .Cast(type)
+                        .ToList(type);
 
-                }
+                // If neither of the above cases are matched, we return "value" as a fallback
+                default:
+                    return value;
 
             }
 
-            return value;
-
         }
 
         public override Type GetPropertyValueType(IPublishedPropertyType propertyType) {
@@ -104,75 +109,70 @@ public override Type GetPropertyValueType(IPublishedPropertyType propertyType) {
 
         }
 
-        private IPublishedContent GetPublishedContent<T>(T nodeId, ref UmbracoObjectTypes actualType, UmbracoObjectTypes expectedType, Func<T, IPublishedContent> contentFetcher) {
-
-            // is the actual type supported by the content fetcher?
-            if (actualType != UmbracoObjectTypes.Unknown && actualType != expectedType) {
-                // no, return null
-                return null;
-            }
-
-            // attempt to get the content
-            var content = contentFetcher(nodeId);
-            if (content != null) {
-                // if we found the content, assign the expected type to the actual type so we don't have to keep looking for other types of content
-                actualType = expectedType;
-            }
-            return content;
-        }
-
-        public object GetPickerValue(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object source, bool preview) {
-            umbracoContextAccessor.TryGetUmbracoContext(out var umbracoContext);
+        private object GetPickerValue(IPublishedPropertyType propertyType, object source, bool preview) {
+            
             if (source == null) return null;
 
-            if (umbracoContext == null) return source;
-
-            Udi[] udis = source as Udi[] ?? new Udi[0];
-
+            Udi[] udis = source as Udi[] ?? Array.Empty<Udi>();
             if (propertyType.Alias.Equals(Constants.Conventions.Content.InternalRedirectId)) return udis.FirstOrDefault();
             if (propertyType.Alias.Equals(Constants.Conventions.Content.Redirect)) return udis.FirstOrDefault();
+            
+            // Get a reference to the current published snapshot
+            _publishedSnapshotAccessor.TryGetPublishedSnapshot(out IPublishedSnapshot publishedSnapshot);
+            if (publishedSnapshot == null) return source;
 
+            // Is the data type configured as a single picker?
             bool single = IsSingleNodePicker(propertyType);
 
-            List<IPublishedContent> multiNodeTreePicker = new List<IPublishedContent>();
-
-            UmbracoObjectTypes objectType = UmbracoObjectTypes.Unknown;
+            // Initialize a new list for the items
+            List<IPublishedContent> items = new List<IPublishedContent>();
 
-            foreach (var udi in udis) {
+            foreach (Udi udi in udis) {
 
+                // Make sure we have a GUID UDI
                 GuidUdi guidUdi = udi as GuidUdi;
                 if (guidUdi == null) continue;
 
-                IPublishedContent multiNodeTreePickerItem = null;
-                var couldGetPublishedSnapshotAccessor = _publishedSnapshotAccessor.TryGetPublishedSnapshot(out var publishedSnapshot);
+                IPublishedContent item = null;
+                
                 switch (udi.EntityType) {
                     case Constants.UdiEntityType.Document:
-                        multiNodeTreePickerItem = GetPublishedContent(udi, ref objectType, UmbracoObjectTypes.Document, id => publishedSnapshot.Content.GetById(guidUdi.Guid));
+                        item = publishedSnapshot.Content.GetById(preview, guidUdi.Guid);
                         break;
                     case Constants.UdiEntityType.Media:
-                        multiNodeTreePickerItem = GetPublishedContent(udi, ref objectType, UmbracoObjectTypes.Media, id => publishedSnapshot.Media.GetById(guidUdi.Guid));
+                        item = publishedSnapshot.Media.GetById(preview, guidUdi.Guid);
                         break;
                     case Constants.UdiEntityType.Member:
-                        //multiNodeTreePickerItem = GetPublishedContent(udi, ref objectType, UmbracoObjectTypes.Member, id => publishedSnapshot.Members.GetByProviderKey(guidUdi.Guid));
+                        item = GetMemberByGuidUdi(guidUdi, publishedSnapshot);
                         break;
                 }
 
-                if (multiNodeTreePickerItem != null && multiNodeTreePickerItem.ContentType.ItemType != PublishedItemType.Element) {
-                    multiNodeTreePicker.Add(multiNodeTreePickerItem);
-                    if (single) break;
-                }
+                // Continue to the next UDI if "item" is either null or an element type
+                if (item == null) continue;
+                if (item.ItemType == PublishedItemType.Element) continue;
+
+                // Append the item to the list
+                items.Add(item);
+
+                // If the data type is configured as a single picker, we break the loop as we don't really need to
+                // look up any additional items that may be picked
+                if (single) break;
 
             }
 
-            if (single) return multiNodeTreePicker.FirstOrDefault();
-            return multiNodeTreePicker;
+            return single ? items.FirstOrDefault() : items;
 
         }
 
         private static bool IsSingleNodePicker(IPublishedPropertyType propertyType) {
             return propertyType.DataType.ConfigurationAs<MultiNodePickerConfiguration>().MaxNumber == 1;
         }
 
+        private IPublishedContent GetMemberByGuidUdi(GuidUdi udi, IPublishedSnapshot snapshot) {
+            IMember member = _memberService.GetByKey(udi.Guid);
+            return member == null ? null : snapshot.Members.Get(member);
+        }
+
         #endregion
 
     }