0
\$\begingroup\$

Based on feedback on my previous question, I made many small adjustments to the code, added more APIs and tried to follow through with delivering on the excellent advice I have received in regards to my code. I have updated my code with small summaries to provide more documentation.

Here is the updated interface.

namespace Constrainable
{
    public interface IConstrainableSet<T>
    {

        T AlterInput(T Value);
        T AlterInputConstrainedStore(T Value);
        ConstrainableSet<T> ChainLinkClone();
        ConstrainableSet<N> ChainLinkNew<N>(Predicate<N> CheckBeforeSet = null, Func<N, N> ModifyBeforeSet = null, Action<ConstrainableSet<N>> FailAction = null, Action<ConstrainableSet<N>> SuccessAction = null);
        T ConstrainedStore(T Value);
        T GetStoredData();
        bool IsValid(T Value);
    }
    public interface IConnectable
    {
        public object Root { get; set; }
        public object Next { get; set; }
        public object Parent { get; set; }
    }
}

And here is the new code.

using System;
namespace Constrainable
{
public class Connectable : IConnectable
{
    public object Root { get; set; }
    public object Next { get; set; }
    public object Parent { get; set; }
}
/// <summary>
/// ConstrainableSet is intended to Allow full control over what data gets set in the inner field `data,
/// You can optioning modify the input. This is also intended to be a test of idea's,
/// new and/or interesting ways of accomplishing tasks. 
/// </summary>
/// <typeparam name="T"></typeparam>

public class ConstrainableSet<T> : Connectable, IConstrainableSet<T>
    {
        public ConstrainableSet(Predicate<T> CheckBeforeSet = default, Func<T, T> ModifyBeforeSet = default, Action<ConstrainableSet<T>> FailAction = default, Action<ConstrainableSet<T>> SuccessAction = default)
        {
            _failAction = FailAction;
            _checkBeforeSet = CheckBeforeSet;
            _modifyBeforeSet = ModifyBeforeSet;
            _successAction = SuccessAction;
        }


        #region ControlFlow

        /// <summary>
        ///`_failAction` field recieves the `FailAction` parameter in the constructor. 
        ///This Parameter is a callback that is run when a failure occurs in `ConstrainedStore()`,
        ///for both Callbacks the whole `ConstrainableSet` Object is passed as a parameter.
        /// </summary>

        private readonly Action<ConstrainableSet<T>> _failAction;

        /// <summary>
        /// _successAction field recieves the `SuccessAction` parameter in the constructor.
        /// This Parameter is a `Callback` , this function is called on in `ConstrainedStore()` 
        /// and only after successful set of the inner private field `data`.
        /// </summary>

        private readonly Action<ConstrainableSet<T>> _successAction;

        /// <summary>
        /// _checkBeforeSet is a bool lambda expression which evaluates the Parameter
        /// to be set, and must return true for the data to be set
        /// </summary>

        private readonly Predicate<T> _checkBeforeSet;

        /// <summary>
        /// _modifyBeforeSet this routine Takes type T as input and returns type T
        /// this function is for Encryption or Concatenating or anything that involves modifying the parameter passed in to it
        /// </summary>

        private readonly Func<T, T> _modifyBeforeSet;
        #endregion

        private T data;

        /// <summary>
        /// The ConstrainableStore Function is the combined input of 
        /// 3 parameters in this fashiion (PreCheck ? SuccessAction : FailAction)
        /// if there is no _checkBeforeSet function defined then this simply returns the value passed in,
        /// if there is an _checkBeforeSet function defined, it calls that function and based on success or failure. 
        /// if that Action is defined for success it moves the value into storage then calls SuccessAction, 
        /// if there is no success action it sets the successful checked value and returns the value, 
        /// if there is no FailAction defined at this point then it returns the default value for this type
        /// </summary>
        /// <param name="Value"></param>
        /// <returns>T</returns>
        public virtual T ConstrainedStore(T Value)
        {
            if (_checkBeforeSet != default)
            {
                if (_checkBeforeSet(Value))
                {
                    if (_successAction != default)
                    {
                        data = Value;
                        _successAction(this);
                    }
                    data = Value;
                    return Value;
                }
                if (_failAction != default)
                {
                    _failAction(this);
                }
                return default;
            }
            return Value;
        }
        /// <summary>
        /// IsValid runs the define checkBeforeSet function and returns result.
        /// </summary>
        /// <param name="Value"></param>
        /// <returns>bool</returns>
        public virtual bool IsValid(T Value)
        {
            if (_checkBeforeSet != null)
            {
                return _checkBeforeSet(Value);
            }
            return false;
        }
        /// <summary>
        /// This is the interesting function, 
        /// I think with this simple function actionable values can be achieved
        /// many tasks could benefit from Actionable `Validated` values imho.
        /// Actionable values is values that can be used in some way(s) to get a new value or object, 
        /// or encrypting the value passed in, this could be used for something like that.
        /// </summary>
        /// <param name="Value"></param>
        /// <returns></returns>
        public virtual T AlterInput(T Value)
        {
            if (_modifyBeforeSet != default)
            {
                return _modifyBeforeSet(Value);
            }
            return Value;
        }
        //Combined usage
        public virtual T AlterInputConstrainedStore(T Value)
        {            
            return ConstrainedStore(AlterInput(Value));
        }
        /// <summary>
        /// I made a simple way to link objects together
        /// I simply needed to pinpoint Cloning and Creation and
        /// push the proper values to  these objects during cloning or creation.
        /// </summary>
        /// <returns></returns>
        public ConstrainableSet<T> ChainLinkClone()
        {
            if(Root == default)
            {
                Root = this;
            }
            ConstrainableSet<T> newpo = new ConstrainableSet<T>(_checkBeforeSet, _modifyBeforeSet, _failAction, _successAction);
            IConnectable Iterator;
            if (Next != default)
            {
                Iterator = Next as IConnectable;
                while (Iterator.Next != default)
                {
                    Iterator = Iterator.Next as IConnectable;
                }
                Iterator.Next = newpo;
                newpo.Root = Root;
                newpo.Parent = this;
                return newpo;
            }
            else
            {
                Next = newpo;
                newpo.Root = Root;
                newpo.Parent = this;
                return newpo;
            }
        }

        public ConstrainableSet<N> ChainLinkNew<N>(Predicate<N> CheckBeforeSet = default, Func<N, N> ModifyBeforeSet = default, Action<ConstrainableSet<N>> FailAction = default, Action<ConstrainableSet<N>> SuccessAction = default)
        {
            if (Root == default)
            {
                Root = this;
            }
            ConstrainableSet<N> newpo = new ConstrainableSet<N>(CheckBeforeSet, ModifyBeforeSet, FailAction, SuccessAction);
            IConnectable Iterator;
            if (Next != default)
            {
                Iterator = Next as IConnectable;
                while (Iterator.Next != default)
                {
                    Iterator = Iterator.Next as IConnectable;
                }
                Iterator.Next = newpo;
                newpo.Root = Root;
                newpo.Parent = this;
                return newpo;
            }
            else
            {
                Next = newpo;
                newpo.Parent = this;
                newpo.Root = Root;
                return newpo;
            }

        }
        //returns the data field.
        public T GetStoredData()
        {
            return data;
        }
    }
}

I would also like to provide a example of usage. This example demonstrates the new ChainLinkNew Function which can be used in the SuccessCallback function to chain validations and data stores together, or in the way I accomplish this below. Any input on my approach is highly appreciated.

    public Test()
    {
        ConstrainableSet<int> programmable = new ConstrainableSet<int>(
                                        CheckBeforeSet: x => x == 100
                                        ).ChainLinkNew<int>(
                                            CheckBeforeSet: x => x == 10 | x == 1,
                                            ModifyBeforeSet: x => x / 10,
                                            FailureCallBack,
                                            SuccessCallBack)
                                            .Root as ConstrainableSet<int>;

        void FailureCallBack(ConstrainableSet<int> set)
        {
        }
        void SuccessCallBack(ConstrainableSet<int> set)
        {
        }
        ((ConstrainableSet<int>)programmable.Next).AlterInputConstrainedStore(programmable.ConstrainedStore(100));
    }
\$\endgroup\$

closed as off-topic by Peter Taylor, yuri, pacmaninbw, esote, Edward May 18 at 19:58

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Peter Taylor, yuri, pacmaninbw, esote, Edward
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Bah realized I introduced a flaw.. although workable as it is I will still fix it anyways.. The flaw is that IConstrainableSet interface defined root next and parent, so that means Root, Next and Parent in my implementation class also define these 3 items each, to remedy this I will make a impl class for this interface.. please do not review until I make this change. \$\endgroup\$ – BanMe May 14 at 18:05
  • \$\begingroup\$ updated per last comment, please review the horrible hack I just implemented.. :[ \$\endgroup\$ – BanMe May 14 at 18:41
  • 1
    \$\begingroup\$ What exactly are your goals/requirements for this code? What actual problems are you trying to solve? \$\endgroup\$ – Pieter Witvoet May 16 at 7:30
  • \$\begingroup\$ Well my goals are: I want a tool that can be used to validate input and store or alter that input. I also want connect them together, and easily reuse th. I think maybe I covered all the ground I can Setting a field.. tbh this all started with just wanting to code better, and learn more, and tackle a topic that is simple and often not considered useful, property set. I dislike declaring data, so this tool is a experiment to abstract that away, as well.. I came up with the original generic design, which was closed, and wanted to improve upon it. \$\endgroup\$ – BanMe May 16 at 13:23
  • 2
    \$\begingroup\$ You should only ask for code review on working code. This code doesn't compile, so it is definitely not working. \$\endgroup\$ – Peter Taylor May 17 at 8:33
3
\$\begingroup\$

Like Pieter Witvoet I don't quite understand which problem this code is trying to solve. There is an ObservableCollection Class that partly does the same. Together with validation logic in properties and possibly the implementation of INotifyPropertyChanged, this comes close to your solution.

Apart from this, your code has problems and can be improved.

  • It does not compile, because interface members in IConnectable are declared as public. Interface members are always public (C# 8 will change this, but we are not quite there yet). You should always post compilable code on Code Review.

  • ChainLinkNew<N> allows to add elements of another type to the collection. The purpose of generics is to provide type safety while allowing you to create type variants at design time. With this construct you are losing type safety, as the type of added elements in the set will be determined at run time. Use the class' type parameter T instead. If you need to add items of different types, create a ConstrainableSet<object> or use a base type common to all elements. Like this, at least your constrainable set is consistent and type safe, even if the data is not. Otherwise none of them will be.

  • The interface IConstrainableSet<T> depends on its own implementation! As explained here,

    The point of the interfaces is to have something that is common to all implementations. By trying to do this you destroy the whole reason why interfaces exist.

    Change the interface to (and adapt the implementation)

    public interface IConstrainableSet<T>
    {
        ...
        IConstrainableSet<T> ChainLinkClone();
        IConstrainableSet<T> ChainLinkNew(Predicate<T> CheckBeforeSet = null,
           Func<T, T> ModifyBeforeSet = null, Action<IConstrainableSet<T>> FailAction = null,
           Action<IConstrainableSet<T>> SuccessAction = null);
        ...
    }
    
  • The IConnectable interface should be generic.

    public interface IConnectable<T>
    {
        T Root { get; set; }
        T Next { get; set; }
        T Parent { get; set; }
    }
    
  • The Connectable class can be abstract and must implement IConnectable<T>. Connectable by itself does not contain data and does not seem to be useful other than as base class. If it contained a Data field it would make sense to instantiate a Connectable. As it is now, you could only create a doubly linked list containing empty nodes.

    public abstract class Connectable<T> : IConnectable<T>
    
  • These improvements require some changes in the implementation but also allow some simplifications. E.g. in ChainLinkClone

    var newpo = new ConstrainableSet<T>(_checkBeforeSet, _modifyBeforeSet, _failAction,
        _successAction);
    if (Next != default) {
        ConstrainableSet<T> Iterator = Next;
        while (Iterator.Next != default) {
            Iterator = Iterator.Next;
        }
    ...
    } else {
        ...
    }
    
    • Use var in the new statement to avoid rewriting the lengthy type name.
    • Move Iterator inside if and use an initializer.
    • We can drop some casts.
\$\endgroup\$
  • \$\begingroup\$ thank you for reviewing, it's a WIP, and I really want to make it as DRY as possible, and solidly reviewed, before expanding upon it. My vision is to eventually make the data transient, and add configurable data flow to the created ConstrainableSet, to allow IO between the created items in a defined way. I am having trouble understanding why Connectable can/should be abstract, and why it should implement the IConnectable<T>. if I move the interface to ConstrainableSet then I can do IConnectable<IConstrainableSet<T>>, does this work well or is it bad taste? \$\endgroup\$ – BanMe May 17 at 2:02
  • \$\begingroup\$ Connectable by itself does not contain data and does not seem to be useful other than as base class. If it contained a Data field it would make sense to instantiate a Connectable. As it is now, you could only create a doubly linked list containing empty nodes. \$\endgroup\$ – Olivier Jacot-Descombes May 17 at 13:12

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