-2
\$\begingroup\$

This is the 'middle-ware' for a document management application. A document has properties which describe it, and allow users to search for it. The server will interact with the database, and the User-Interface will consume the client code.

I don't have any particular concerns, I'm mostly looking for general comments and critique.

public class SDocsClient
{
    //document management application  
    //documents and system fields like ID and date added  
    //administrators can add custom fields subject, from, to from a web page
    //have a WPF client for speed and will eventually add disconnected operation

    #region enums
    public enum RORW { [Description("Read Only")] RO, [Description("Read Write")] RW };

    public enum SVMV { [Description("Single Value")] SV, [Description("Multi Value")] MV };

    public enum SearchOpUIE { Union, Intersect, Except };  //this confuses users but the UI is free to use other terms this mimics the TSQL commands 

    public enum SearchOpSV { GT, GE, EQ, LE, LT, Any  };

    public enum SearchOpMV { GT, GE, EQ, LE, LT, Any, All };

    public enum SearchOpDate { GT, GE, EQ, LE, LT };

    public enum SystemCustom { System, Custon };

    public enum DataType { String, Date };
    #endregion enums

    #region serverCalls
    private static SDocsServer sDocsServer;

    public SDocsClient(SDocsServer _sDocsServer, List<DocPropDef> docPropDefs)
    {
        sDocsServer = _sDocsServer;  //only the server can access the database
        DocPropDefs = docPropDefs;
    }
    #endregion serverCalls

    #region Document
    public static bool AddDocument(Document document)
    {
        return sDocsServer.AddDoc(document);
    }

    public static List<DocPropDef> DocPropDefs { get; private set; } = new List<DocPropDef>();  //load in ctor

    public class DocPropDef     
    {
        //raw definition of the properties - no value
        public override int GetHashCode() { return ID; }
        public override bool Equals(Object obj)
        {
            if (obj == null || GetType() != obj.GetType())
                return false;
            DocPropDef docPropDef = (DocPropDef)obj;
            return (docPropDef.ID == this.ID);
        }
        public byte ID { get; private set; }
        public byte DefaultSort { get; private set; }
        public string Name { get; private set; }
        public RORW RORW { get; private set; }
        public SVMV SVMV { get; private set; }
        public DataType DataType { get; private set; }
        public SystemCustom SystemCustom { get; private set; }
        public DocPropDef(byte id, byte defaultSort, string name, RORW rorw, SVMV svmv, DataType dataType, SystemCustom systemCustom)
        {   
            ID = id;
            DefaultSort = defaultSort;
            Name = name;
            RORW = rorw;
            SVMV = svmv;
            DataType = dataType;
            SystemCustom = systemCustom;
        }
        public DocPropDef(DocPropDef docPropDef)
        {
            //this is used by search which has access DocPropDefs 
            ID = docPropDef.ID;
            DefaultSort = docPropDef.DefaultSort;
            Name = docPropDef.Name;
            RORW = docPropDef.RORW;
            SVMV = docPropDef.SVMV;
            DataType = docPropDef.DataType;
            SystemCustom = docPropDef.SystemCustom;
        }
    }
    #region value

    public abstract class DocPropValue
    {
        //generic document property with string DisplayValue for all DataTypes

        //on the UI it is convienent to access other DocProp so access to Document is how to get there 
        //this passes a lot of data to the server for update so probably going to build a lighweigt object like DocPropLoad
        public Document Document { get; private set; }

        //considered inhertance of DocPropDef here 
        //but if search returns 100000 that is bit of overhead
        public DocPropDef DocPropDef { get; private set; }
        public abstract string DisplayValue { get; }
        public bool Update()
        {
            return sDocsServer.UpdateDocPropValue(this);
        }
        public DocPropValue(Document document, DocPropDef docPropDef)
        {
            Document = document;
            DocPropDef = docPropDef;
        }
    }

    public class DocPropStringSV : DocPropValue
    {
        // for DataType string single value
        public override string DisplayValue { get { return Value; } }
        private string _value;
        public string Value
        {   get { return _value; }
            set
            {
                if (_value != value)  //don't waste a server call
                {
                    _value = value;
                    Update();
                }
            }
        }
        public DocPropStringSV(string value, Document document, DocPropDef docPropDef)
            : base(document, docPropDef)
        {
            _value = value;
        }
    }

    public class DocPropStringMV : DocPropValue
    {
        // for DataType string multi value
        public override string DisplayValue { get { return String.Join(",", Values.OrderBy(x => x)); } }
        HashSet<String> values = new HashSet<string>(StringComparer.OrdinalIgnoreCase);  //match the DB 
        //List would be more convenient but do not want to allow duplicate values

        public IEnumerable<String> Values
        {
            get { return values.OrderBy(x => x); }
        }
        public bool Add(string value)
        {
            if(string.IsNullOrEmpty(value))
            {
                return false;
            }
            value = value.Trim();
            if(ValuesRaw.Add(value))
            {
                return Update();  
            }
            return false;
        }
        public bool Remove(string value)
        {
            if (string.IsNullOrEmpty(value))
            {
                return false;
            }
            value = value.Trim();
            if (ValuesRaw.Remove(value))
            {
                return Update();
            }
            return false;
        }
        public bool Revise(string value, string valueOld)
        {
            if (string.IsNullOrEmpty(value))
            {
                return false;
            }
            if (string.IsNullOrEmpty(valueOld))
            {
                return false;
            }
            if (ValuesRaw.Remove(valueOld.Trim()))
            {
                ValuesRaw.Add(value.Trim());
                return Update();
            }
            return false;
        }
        internal HashSet<String> ValuesRaw
        {
            get { return values; }  //weakness here is if they modify this directly the DB does not update 
                                    //so made this internal
            set
            {
                if (!values.SetEquals(value))
                {
                    values.Clear();
                    values.UnionWith(values.Select(x => x.Trim()));
                    Update();
                }
                //I know I do it different in the ctor - trying to decide which I like best 
                //there could be an egde case where !values.SetEquals(value) fails due to a space but this is internal
            }
        }
        //right now make the UI client add and remove but that may be better here
        public DocPropStringMV(HashSet<String> _values, Document document, DocPropDef docPropDef)
             : base(document, docPropDef)
        {
            values.Clear();
            foreach (string s in _values)
            {
                values.Add(s.Trim());
            }
        }
    }

    public class DocPropDateSV : DocPropValue
    {
        // for DataType date single value
        public override string DisplayValue { get { return Value.ToString(); } }
        private DateTime _value;
        public DateTime Value
        {
            get { return _value; }
            set
            {
                if (_value != value)
                {
                    _value = value;
                    Update();
                }
            }
        }
        public DocPropDateSV(DateTime value, Document document, DocPropDef docPropDef)
             : base(document, docPropDef)
        {
            _value = value;
        }
    }

    public class DocPropDateMV : DocPropValue
    {
        // for DataType date multi value
        public override string DisplayValue { get { return String.Join(",", Values.ToString()); } }
        private HashSet<DateTime> values = new HashSet<DateTime>();
        public IEnumerable<DateTime> Values
        {
            get { return values.OrderByDescending(x => x); }  //latest date is typically what they are most interested in
        }
        internal HashSet<DateTime> ValuesRaw
        {
            get { return values; }  //weakness here is if they modify this directly the DB does not update 
                                    //the UI client must call Update()
            set
            {
                if (!values.SetEquals(value))
                {
                    values = value;
                    Update();
                }
            }
        }
        public DocPropDateMV(HashSet<DateTime> _values, Document document, DocPropDef docPropDef)
             : base(document, docPropDef)
        {
            values = _values;
        }
    }
    #endregion value

    #region load
    public abstract class DocPropLoad
    {
        //this is used for getting data from the server 
        //it is designed to be small
        internal int FieldDefID { get; set; }
        internal DocPropLoad(int fieldDefID)
        {
            FieldDefID = fieldDefID;
        }
    }

    public abstract class DocPropLoadStringSV : DocPropLoad
    {
        internal string Value { get; set; }
        public DocPropLoadStringSV(int fieldDefID, string value)
            : base (fieldDefID)
        {
            Value = value;
        }
    }

    public abstract class DocPropLoadStringMV : DocPropLoad
    {
        public HashSet<string> Values { get; set; }
        public DocPropLoadStringMV(int fieldDefID, HashSet<string> values)
            : base(fieldDefID)
        {
            Values = values;
        }
    }

    public abstract class DocPropLoadDateSV : DocPropLoad
    {
        internal DateTime Value { get; set; }
        public DocPropLoadDateSV(int fieldDefID, DateTime value)
            : base(fieldDefID)
        {
            Value = value;
        }
    }

    public abstract class DocPropLoadDateMV : DocPropLoad
    {
        internal HashSet<DateTime> Values { get; set; }
        public DocPropLoadDateMV(int fieldDefID, HashSet<DateTime> values)
            : base(fieldDefID)
        {
            Values = values;
        }
    }
    #endregion load

    #region DocumentDocument
    public class Document 
    {
        //the properties and other data for the document 
        //can have a document with no text and just used as a record 
        public override int GetHashCode() { return ID; }
        public override bool Equals(Object obj)
        {
            if (obj == null || GetType() != obj.GetType())
                return false;

            Document document = (Document)obj;
            return (document.ID == this.ID);
        }
        public int ID { get; private set; }
        public List<DocPropValue> DocPropValues { get; private set; }
        public Document(int id, List<DocPropLoad> docPropLoads)
        {  
            ID = id;
            foreach (DocPropLoad docPropLoad in docPropLoads)
            {
                int docPropDefID = docPropLoad.FieldDefID;
                DocPropDef docPropDef = DocPropDefs.FirstOrDefault(x => x.ID == docPropDefID);
                if (docPropDef == null)
                {
                    throw new IndexOutOfRangeException($"docPropDefID not valid {docPropDefID}");
                }
                if (docPropLoad is DocPropLoadStringSV)
                {
                    DocPropLoadStringSV docPropLoadStringSV = (DocPropLoadStringSV)docPropLoad;
                    DocPropStringSV docPropStringSV = new DocPropStringSV(docPropLoadStringSV.Value, this, docPropDef);
                }
                else if (docPropLoad is DocPropLoadStringMV)
                {
                    DocPropLoadStringMV docPropLoadStringMV = (DocPropLoadStringMV)docPropLoad;
                    DocPropStringMV docPropStringMV = new DocPropStringMV(docPropLoadStringMV.Values, this, docPropDef);
                }
                else if (docPropLoad is DocPropLoadDateSV)
                {
                    DocPropLoadDateSV docPropLoadDateSV = (DocPropLoadDateSV)docPropLoad;
                    DocPropDateSV DocPropDateSV = new DocPropDateSV(docPropLoadDateSV.Value, this, docPropDef);
                }
                else if (docPropLoad is DocPropLoadDateMV)
                {
                    DocPropLoadDateMV docPropLoadDateMV = (DocPropLoadDateMV)docPropLoad;
                    DocPropDateMV docPropDateMV = new DocPropDateMV(docPropLoadDateMV.Values, this, docPropDef);
                }
                else
                {
                    throw new IndexOutOfRangeException($"docPropLoad not found");
                }
            }
        }
    }

    public class DocumentWithText : Document
    {
        public string Text { get; private set; }
        public DocumentWithText(int id, List<DocPropLoad> docPropLoads, string text)
            : base(id, docPropLoads)
        {
            Text = text;
        }
    }

    public class DocumentWithFlowDoc : DocumentWithText
    {   //this application is currently targeted for product documentation and WPF has great controls for FlowDocument  
        //this is the free version - the paid version will support additional formats and features  
        public FlowDocument FlowDocument { get; private set; }
        public DocumentWithFlowDoc(int id, List<DocPropLoad> docPropLoads, string text, FlowDocument flowDocument)
            : base(id, docPropLoads, text)
        {
            FlowDocument = flowDocument;
        }
    }
    #endregion DocumentDocument

    #endregion Document

    #region Search
    public static List<Document> ExecuteSearch(List<SearchItem> searchItems, out bool parenMisMatach)
    {
        parenMisMatach = false;
        int lp = 0, rp = 0;
        foreach (SearchItem si in searchItems)
        {
            lp += si.ParenLft;
            rp += si.ParenRht;
            if (rp > lp)
            {
                parenMisMatach = true;
                return new List<Document>();
            }
        }
        if (rp != lp)
        {
            parenMisMatach = true;
            return new List<Document>();
        }
        SearchHistory.Add(searchItems);
        return sDocsServer.Search(searchItems);  //with the stucture SearchItem Search should never fail
                                                 //if it does fail UI client needs to deal with the exception 
    }

    public static List<List<SearchItem>> SearchHistory { get;  } = new List<List<SearchItem>>();  //not saved session to session
                                                                                                  //it is up the UI client to just submit List<SearchItem> if they want to re-execute   

    public List<SearchItem> SearchItems { get; private set; } = new List<SearchItem>();  //optional for building the current search 

    public abstract class SearchItem : DocPropDef
    {
        //UI client builds up SearchItems to define the search 
        public abstract string DispValue { get; }
        public byte ParenLft { get; set; }
        public byte ParenRht { get; set; }
        public SearchOpUIE SearchOpUIE { get; set; }
        public SearchItem(DocPropDef docPropDef, SearchOpUIE searchOpUIE, byte parenLft, byte parenRht)
             : base(docPropDef)
        {
            ParenLft = parenLft;
            ParenRht = parenRht;
            SearchOpUIE = searchOpUIE;
        }
        public SearchItem(SearchItem searchItem)
             : base(searchItem)
        {
            ParenLft = searchItem.ParenLft;
            ParenRht = searchItem.ParenRht;
        }
    }

    public class SearchItemTextSV : SearchItem
    {
        public SearchOpSV SearchOP { get; set; } = SearchOpSV.EQ;
        public override string DispValue { get { return $"{SearchOP.ToString()} {string.Join(",", Values)}"; } }
        public HashSet<String> Values { get; } = new HashSet<string>(StringComparer.OrdinalIgnoreCase);  //here the UI client can modify
        public SearchItemTextSV(DocPropDef docPropDef, SearchOpUIE searchOpUIE)
             : base(docPropDef, searchOpUIE, 0, 0)
        { }
        public SearchItemTextSV(DocPropDef docPropDef, SearchOpUIE searchOpUIE, HashSet<String> values, byte parenLft, byte parenRht, SearchOpSV searchOp)
            : base(docPropDef, searchOpUIE, parenLft, parenRht)
        {
            Values = values;
            SearchOP = searchOp;
        }
        public SearchItemTextSV(SearchItemTextSV searchItemTextSV)
             : base(searchItemTextSV)
        {
            Values = searchItemTextSV.Values;
            SearchOP = searchItemTextSV.SearchOP;
        }
    }

    public class SearchItemTextMV : SearchItem
    {
        public SearchOpMV SearchOP { get; set; } = SearchOpMV.EQ;
        public override string DispValue { get { return $"{SearchOP.ToString()} {string.Join(",", Values)}"; } }
        public HashSet<String> Values { get; } = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

        public SearchItemTextMV(DocPropDef docPropDef, SearchOpUIE searchOpUIE)
             : base(docPropDef, searchOpUIE, 0, 0)
        { }
        public SearchItemTextMV(DocPropDef docPropDef, SearchOpUIE searchOpUIE, HashSet<String> values, byte parenLft, byte parenRht, SearchOpMV searchOP)
            : base(docPropDef, searchOpUIE, parenLft, parenRht)
        {
            Values = values;
            SearchOP = searchOP;
        }
        public SearchItemTextMV(SearchItemTextMV searchItemTextMV)
             : base(searchItemTextMV)
        {
            Values = searchItemTextMV.Values;
            SearchOP = searchItemTextMV.SearchOP;
        }
    }

    public class SearchItemDateSV : SearchItem
    {
        public SearchOpDate SearchOP { get; set; } = SearchOpDate.EQ;
        public override string DispValue { get { return $"{SearchOP.ToString()} {string.Join(",", Values)}"; } }
        public HashSet<DateTime> Values { get; } = new HashSet<DateTime>();

        public SearchItemDateSV(DocPropDef docPropDef, SearchOpUIE searchOpUIE)
             : base(docPropDef, searchOpUIE, 0, 0)
        { }
        public SearchItemDateSV(DocPropDef docPropDef, SearchOpUIE searchOpUIE, HashSet<DateTime> values, byte parenLft, byte parenRht, SearchOpDate searchOP)
            : base(docPropDef, searchOpUIE, parenLft, parenRht)
        {
            Values = values;
            SearchOP = searchOP;
        }
        public SearchItemDateSV(SearchItemDateSV searchItemDateSV)
             : base(searchItemDateSV)
        {
            Values = searchItemDateSV.Values;
            SearchOP = searchItemDateSV.SearchOP;
        }
    }

    public class SearchItemDateMV : SearchItem
    {
        public SearchOpDate SearchOP { get; set; }
        public override string DispValue { get { return $"{SearchOP.ToString()} {string.Join(", ", Values)}"; } }
        public HashSet<DateTime> Values { get; } = new HashSet<DateTime>();

        public SearchItemDateMV(DocPropDef docPropDef, SearchOpUIE searchOpUIE)
             : base(docPropDef, searchOpUIE, 0 , 0)
        { }
        public SearchItemDateMV(DocPropDef docPropDef, SearchOpUIE searchOpUIE, HashSet<DateTime> values, byte parenLft, byte parenRht, SearchOpDate searchOP)
            : base(docPropDef, searchOpUIE, parenLft, parenRht)
        {
            Values = values;
            SearchOP = searchOP;
        }
        public SearchItemDateMV(SearchItemDateMV searchItemDateMV)
             : base(searchItemDateMV)
        {
            Values = searchItemDateMV.Values;
            SearchOP = searchItemDateMV.SearchOP;
        }
    }
    #endregion Search
}

public class SDocsServer
{
    //this is a whole different server side program 
    //this is thousands of line of code 
    //NOT asking for review of this code 
    //this ONLY is here so you do not get syntax error if you load the code 
    public bool UpdateDocPropValue(SDocsClient.DocPropValue DocPropValue)
    {
        return true;
    }
    public bool AddDoc(SDocsClient.Document document)
    {
        return true;
    }
    public List<SDocsClient.Document> Search(List<SDocsClient.SearchItem> SearchItems)
    {
        return new List<SDocsClient.Document>();
    }
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Another code from you and wall of text again... Can you say why don't you like empty lines between class members and why you always place a lot of classes in one? Is it programming culture in your company? :) \$\endgroup\$ – Maxim Aug 4 '17 at 3:28
  • 7
    \$\begingroup\$ @Paparazzi It will improve readability and maintainability of the code. When you are writing a code you should always think about how beautiful it is :) Empty lines between class members even more important because without them all parts of code just stick together and it is very difficult to read this code. And I believe it is not only my opinion. I sure that most developers will say the same things. \$\endgroup\$ – Maxim Aug 4 '17 at 6:13
  • 4
    \$\begingroup\$ What would you like to have reviewed? There is only some short description of what it does, and lots of code? In many ways I would btw agree with @Maxim, simply putting everything in one file will give anybody else but you a hard time when reviewing your code. I am personally no fan of classes within classes either. Is there any reason why you seem to prefer to avoid namespaces for your code? \$\endgroup\$ – Icepickle Aug 4 '17 at 6:36
  • 8
    \$\begingroup\$ In defense of classes within classes, this is easy to copy-paste and play with. However, the title and the description are very poor, and surprisingly so, coming from a 3k+ user. Please do re-read How to Ask and improve these elements. \$\endgroup\$ – janos Aug 4 '17 at 6:56
  • 3
    \$\begingroup\$ @Paparazzi I've edited your question a bit to improve the English, add a reasonable title, and add some relevant tags. If you can, try to add more description to your body so that it's not just a wall of text. (Perhaps separate some of the classes out and add descriptions between them.) \$\endgroup\$ – Der Kommissar Aug 4 '17 at 13:36
3
\$\begingroup\$

On first glance this isn't bad, though I hate some of the naming.

Let's look at some of it in particular to see what's going on:

public enum RORW { [Description("Read Only")] RO, [Description("Read Write")] RW };

This really isn't great, and unless you're supporting a legacy system that already expects these enum members, I would rename the while thing:

public enum AccessLevel { ReadOnly, ReadWrite }

The same thing here:

public enum SVMV { [Description("Single Value")] SV, [Description("Multi Value")] MV };
public enum ValueType { SingleValue, MultiValue }

You shouldn't have to inspect something and read a 'Description' attribute when programming to find out what it does or represents, the name itself should tell me.


public static List<DocPropDef> DocPropDefs { get; private set; } = new List<DocPropDef>();  //load in ctor

The biggest issue I see here is that you have a private setter on a collection type, which you don't need in C#6.0 (looking through your code, you operate with the list and only in the constructor do you actually set it):

public static List<DocPropDef> DocPropDefs { get; } = new List<DocPropDef>();  //load in ctor

That is entirely acceptable, and prevents us from accidentally setting the list in a non-constructor. (The constructor can still set DocPropDefs directly, but no other code can.)


The same goes for this class, in C#6.0 the private set is obsolete, get rid of them.

public class DocPropDef     
{
    //raw definition of the properties - no value
    public override int GetHashCode() { return ID; }
    public override bool Equals(Object obj)
    {
        if (obj == null || GetType() != obj.GetType())
            return false;
        DocPropDef docPropDef = (DocPropDef)obj;
        return (docPropDef.ID == this.ID);
    }
    public byte ID { get; private set; }
    public byte DefaultSort { get; private set; }
    public string Name { get; private set; }
    public RORW RORW { get; private set; }
    public SVMV SVMV { get; private set; }
    public DataType DataType { get; private set; }
    public SystemCustom SystemCustom { get; private set; }
    public DocPropDef(byte id, byte defaultSort, string name, RORW rorw, SVMV svmv, DataType dataType, SystemCustom systemCustom)
    {   
        ID = id;
        DefaultSort = defaultSort;
        Name = name;
        RORW = rorw;
        SVMV = svmv;
        DataType = dataType;
        SystemCustom = systemCustom;
    }
    public DocPropDef(DocPropDef docPropDef)
    {
        //this is used by search which has access DocPropDefs 
        ID = docPropDef.ID;
        DefaultSort = docPropDef.DefaultSort;
        Name = docPropDef.Name;
        RORW = docPropDef.RORW;
        SVMV = docPropDef.SVMV;
        DataType = docPropDef.DataType;
        SystemCustom = docPropDef.SystemCustom;
    }
}

Even more problematic with the above class is the name: what does DocPropDef mean? Is it a DocumentPropertyDefinition? If so, call it that. Don't make the next programmer guess what it means.


This next bit really, really bothers me:

foreach (DocPropLoad docPropLoad in docPropLoads)
{
    int docPropDefID = docPropLoad.FieldDefID;
    DocPropDef docPropDef = DocPropDefs.FirstOrDefault(x => x.ID == docPropDefID);
    if (docPropDef == null)
    {
        throw new IndexOutOfRangeException($"docPropDefID not valid {docPropDefID}");
    }
    if (docPropLoad is DocPropLoadStringSV)
    {
        DocPropLoadStringSV docPropLoadStringSV = (DocPropLoadStringSV)docPropLoad;
        DocPropStringSV docPropStringSV = new DocPropStringSV(docPropLoadStringSV.Value, this, docPropDef);
    }
    else if (docPropLoad is DocPropLoadStringMV)
    {
        DocPropLoadStringMV docPropLoadStringMV = (DocPropLoadStringMV)docPropLoad;
        DocPropStringMV docPropStringMV = new DocPropStringMV(docPropLoadStringMV.Values, this, docPropDef);
    }
    else if (docPropLoad is DocPropLoadDateSV)
    {
        DocPropLoadDateSV docPropLoadDateSV = (DocPropLoadDateSV)docPropLoad;
        DocPropDateSV DocPropDateSV = new DocPropDateSV(docPropLoadDateSV.Value, this, docPropDef);
    }
    else if (docPropLoad is DocPropLoadDateMV)
    {
        DocPropLoadDateMV docPropLoadDateMV = (DocPropLoadDateMV)docPropLoad;
        DocPropDateMV docPropDateMV = new DocPropDateMV(docPropLoadDateMV.Values, this, docPropDef);
    }
    else
    {
        throw new IndexOutOfRangeException($"docPropLoad not found");
    }
}

I am actually quite terrified at what is going on here. In this method you're not returning any of those objects, you're literally using the constructors for their side-effects (when you construct the object, the base constructor does the server interaction) which is a terrible practice. You should absolutely refactor this so that you are forced to call an Update or something method.

Why, you might ask? You may say 'I will always want to update the server on what property was just created.' Yes, today. Wait until tomorrow, or the next day: there will come a point where you won't want to do that, and you'll be spending a lot of time undoing this setup. Do it now and save yourself the future trouble.

The other problem with this is that your API is no longer testable: how do we know if the server connection succeeded or failed? How do we know what the result of creating it is? We don't, really. Because it will always try to send an update, and that's a bad thing. A constructor should put this object in a valid state, then a method should operate on it.

\$\endgroup\$
  • \$\begingroup\$ Thanks I agree on access level. Use [Description("Read Only")] RO as our users like the space in Read Only and the description is typically used in the UI. When we create the text of search it is space limited and use the RO. I will make the adjustments you recommend. \$\endgroup\$ – paparazzo Aug 4 '17 at 14:13
  • \$\begingroup\$ @Paparazzi You can keep the Description in there for UI purposes, but you should still name the actual enum and values appropriately for developer purposes. \$\endgroup\$ – Der Kommissar Aug 4 '17 at 14:15
  • \$\begingroup\$ I usually like to wait a couple days to accept but this might get closed. \$\endgroup\$ – paparazzo Aug 4 '17 at 15:01
  • \$\begingroup\$ In the classes could get rid of the private but not here public static List<DocPropDef> DocPropDefs { get; private set; } \$\endgroup\$ – paparazzo Aug 4 '17 at 15:31
  • 1
    \$\begingroup\$ @Paparazzi Why? Why do you need a private set on a collection type? Usually you would just call methods on them which still work appropriately even when they're get-only. \$\endgroup\$ – Der Kommissar Aug 4 '17 at 15:33

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.