A number of hopefully simple questions regarding best practices and coding conventions about this code

Question

I have placed comments with my questions inline. The code supports making rest calls.

    // (1) Is appending Base to the name of base classes useful?
    public abstract class RestCallBase : IRestCall
    {
        // (2) Is there a good way to decide on the ordering/grouping of members?
        // I have seen code that uses #region for this, but I feel like it's not
        // pervasive.
        public string Url { get; set; }
        public string Method { get; set; }
        public string PostData { get; set; }
        public string ResponseText { get; set; }

        // (3) How do you feel about using the same name for type and identifier
        // in cases like this?  I go back and forth because on one hand it feels
        // cleaner than using underscore (_MyPrivateProperty) or
        // camel cased (myPrivateProperty).
        private HttpWebRequest HttpWebRequest { get; set; }

        // (4) Is it clear that the target of the lone verb comprising the method
        // name is the noun which is the name of the class?  To me, it is 
        // redundant to say 'PrepareForRestCall'.
        protected abstract void Prepare();

        public IRestCall Go()
        {
            this.Prepare();

            HttpWebRequest = (HttpWebRequest)WebRequest.Create(Url);

            // (5) Here a region is used in place of a comment, but I have not
            // seen any other code that uses regions this way.  My thinking is
            // that it brings the code one step closer to becoming a private
            // method but can stay like this until it actually needs to be called
            // from multiple points in the logic.
            #region Add post data to request if present.
            if (!string.IsNullOrEmpty(PostData))
            {
                // (6) I changed this from 'sw' to 'writer' after code review.
                // Would you have as well?
                using(StreamWriter writer = new StreamWriter(HttpWebRequest.GetRequestStream()))
                    writer.Write(PostData); // (7) Would you use curly braces for a single statement?  I opt to save two lines so I can see more code on the screen.
            }
            #endregion

            using (HttpWebResponse response = HttpWebRequest.GetResponse() as HttpWebResponse)
            {
                StreamReader reader = new StreamReader(response.GetResponseStream());
                ResponseText = reader.ReadToEnd();
            }

            return this;
        }
    }

Answer 1

Even though no one liked all the rules associated with StyleCop, our team decided to adopt is so at least the simple things would be consistent throughout our code base.

1. Yes.
2. StyleCop
3. I do it all the time (as does the .NET Framework)
4. Agreed.
5. I personally wouldn't use regions like that.
6. Definitely
7. Yes. StyleCop would enforce this.

Answer 2

1. Prefixing the class name with Abstract is also used. You could use this instead of adding Base, but you would normally use this for abstract base classes of course;

2. Use whatever grouping/ordering is useful for your object;

3. This is quite normal, but, for public properties/fields. For private properties, you would probably use _myPrivateProperty;

4. Agree;

5. I think this is very confusing. You should just use regions to group many lines of codes. You usually see this to group all properties, methods or implementations of an interface. Not on this detail level;

6. Yes;

7. I always use curly braces for using, while, foreach. For if statements, I tend to leave them out for single line blocks, but only when all else's also have a single line.

Answer 3

I'll try to answer these in order:

1. Personally, I avoid this unless necessary. When using an abstract class, you're really setting up a class hierarchy where using a concrete class should be possible via the abstract class's API (see Liskov Substitution Principle for details). I feel strange typing RestCallBase instance = ... instead of just RestCall.

2. Ordering of members is entirely up to you. Order them in a way that makes sense.

3. There is no problem using the same name as the type identifier for a property. This is common in the BCL.

4. Yes. This seems perfectly acceptable as a rule. However, in this case, what does "Prepare" actually do? I would be confused seeing this API as I have no idea what "Prepare" would do for a rest call.

5. This is somewhat odd to me. If you feel that this deserves to be a private method, make it a method. Otherwise, use a comment. Using a region directive as a comment seems strange in my opinion.

6. I would have used streamWriter, personally, as it's even more clear.

7. This is personal preference, but I personally prefer to wrap in braces, even when it's a one liner. StyleCop will enforce their usage, BTW - as it's found to be less problematic over time.

Answer 4

(1) Is appending Base to the name of base classes useful?

This is a matter of personal preference. I personally can't stand it. I hate seeing code like RestCallBase instance = new SomeConcreteRestCall(); instance is a RestCall. Period.

(2) Is there a good way to decide on the ordering/grouping of members? I have seen code that uses #region for this, but I feel like it's not pervasive.

I like to see related items grouped together. I hate #region (it's just a way to hide code and it increases maintenance costs trying to keep all the #regions organized) and I consider it a code smell.

(3) How do you feel about using the same name for type and identifier in cases like this?

Love it. Please see my previous answer on this subject.

I go back and forth because on one hand it feels cleaner than using underscore (_MyPrivateProperty) or camel cased (myPrivateProperty).

I do not like the leading underscore anymore (I have to admit I use to). Now I prefix members with this; it's just clearer to me.

(4) Is it clear that the target of the lone verb comprising the method name is the noun which is the name of the class? To me, it is redundant to say PrepareForRestCall.

I could go either way on this one but would probably lean towards the shorter Prepare but if someone argued that PrepareForRestCall is clearer or asked what is Prepare preparing for, I would probably concede the point.

(5) Here a region is used in place of a comment, but I have not seen any other code that uses regions this way.

I hate regions. The way it is used here is ridiculous.

(6) I changed this from sw to writer after code review. Would you have as well?

Yes.

(7) Would you use curly braces for a single statement? I opt to save two lines so I can see more code on the screen.

Oh you better believe I would. Clearer, reduces maintenance costs.

Answer 5

#5 - I would replace that region-as-comment with a method named AddPostDataToRequestIfPresent.

Answer 6

(5) Here a region is used in place of a comment, but I have not seen any other code that uses regions this way.

I would say this is interesting. Here are my thoughts on why.

Possible pluses:

• If the comment explains what you are doing then it is enough to just read the comment and keep the region closed. Thus helping to understand the program with minimum effort.

• You see clearly what region the comment is referring to. Sometimes it's not as clear if you use a regular comment.

To me its like a method except its code is at the most logical place, where its used.

Possible minuses:

• The input and output is less clear.