I wrote a code comment today

Second one this year. I think its important to only write comments about why you do something not how. The how should be documented by the code. The code in question

Code (CSharp):

1. private readonly SemaphoreSlim _cacheLock = new SemaphoreSlim(1);
2.  
3. //GetOrCreateAsync is thread safe. But if multiple threads call it at the same time, the factory func will be called multiple times.
4. await _cacheLock.WaitAsync();
5. var token = await _cache.GetOrCreateAsync(key, FetchToken);
6. _cacheLock.Release();

 

I like comments because the green is easy to see and helps out when scanning quickly through code. I also highly encourage everyone to use comments as much as needed, because although various people might think their own code is self explanatory and thus doesn't need lowly comments, well, no.

 

I don't brush my teeth, because only idiots get cavities.


lol. i thought comments were for other people?

 

and future you.

 

Future me always appreciates quality comments. Especially when I leave notes to myself in them like...

Code (CSharp):

1. // hi Jenna. :)

 

What Style do you guys prefer?

 

Code (CSharp):

1. // ###############################################
2. // GIANT SECTION STARTING HERE - Added by Jenna
3. // This Section controls the section where things
4. // happen in sections. Section A is only used if
5. // needed to make this comment in the Unity forum.
6. // ###############################################
7.  
8. /*
9.     blah blah.blah(blah);
10. */

 

Same as the comments shown on the following page which would be the first example at the link you gave.

https://docs.microsoft.com/en-us/dotnet/csharp/codedoc

 

Simplicity:

Code (CSharp):

1. //comment_1
2. //comment_2
3. if(true)
4.     DoSomething();

 

Header info:

Code (CSharp):

1. // #######################
2. // This is the comment body.
3. // #######################

Line comments:

Code (CSharp):

1. // This is the comment body.

Commented out code:

Code (CSharp):

1. /*
2.      This is the comment body.
3.      Variation One.
4. */

At a glance, I can immediately tell what comment is there for what reason and act accordingly.

 

Commented out code is a death sentence crime, you have version control you know

Edit: if you feel the need to comment method bodies use a format that doc tools can use

 

Yeah and sometimes you need to comment out code for testing purposes that happens outside of commits.

 

That's a code quality problem

 

Sure but if you commit it, you are dead

(it's fine to stash it)

 

I hope the comment explains why you have a if block thats always true

 

That's why comments exist in the first place!

 

And //comment_2 explains what DoSomething actually does? But, since then someone changed the behavior of DoSomething without updating the comment?

 

I comment almost everywhere, I just make sure to keep my comments as short and to the point as possible. You never know how long in the future you may read the code, or who else may read it, or what you may need to repurpose it for. Reading comments is always faster than reading code, no matter how good a programmer you are. If I open an old script from 5 years ago, I can read through it very quickly and know whats happening due to clear, concise and regular comments.

Conversly, I open a lot of uncommented code here at work from previous developers and it drives me up the wall how much longer I spend deciphering instead of fixing.

 

Thats a code quality problem, not a problem with absence of comments. Comments should explain why you do something, not how. How should be self explanatory by the code. If you need to tell why everywere maybe its time to try to write less complex code?

 

More what you are doing rather than how or why.

I guess this is personal choice.

 

Thats nonsense, your implying there is a problem when there isnt one. You dont write comments to get out of a problem. You write them to prevent them happening in the future, often not even for yourself.

The idea that you only write a comment if you think the code is complex is nonsense. It may make sense to you and thats great ,everyone has their own styles. But we work in a multi-disciplinary team with many specialised members and as senior dev, not commenting things simply because I understand them and I believe they are self explanatory pieces of code is just bad management of a project pipeline. At work we work on multi million pound pieces of software and any chances taken are bad actions taken. An unnecessary comment is easy to ignore, the reverse is not.

 

People should write more methods. I refactored one of my code monkoeys code the other day, from this

Code (CSharp):

1.         public async Task Invoke(HttpContext ctx, DbContext db, IScriptSession script)
2.         {
3.             var settings = await db.Set<Setting>()
4.                 .ToListAsync();
5.  
6.             //Check if a setting exists for a request
7.             var queryInPath = false;
8.             var setting = settings.FirstOrDefault(s =>
9.             {
10.                 queryInPath = s.Path.IndexOf("{", StringComparison.Ordinal) >= 0;
11.                 return ctx.Request.Method.ToLower() == s.Method.ToLower() && CheckSegments(s.Path.Split('/'), ctx.Request.Path.Value.Split('/'));
12.             });
13.             if (setting == null)
14.             {
15.                 await _next(ctx);
16.                 return;
17.             }
18.  
19.             //Create a request DTO
20.             object body = null;
21.             var contentTypeDelim = ctx.Request.ContentType?.IndexOf(";", StringComparison.Ordinal);
22.             var strippedContentType = contentTypeDelim.HasValue && contentTypeDelim >= 0 ? ctx.Request.ContentType.Substring(0, contentTypeDelim.Value) : ctx.Request.ContentType;
23.  
24.             if (ctx.Request.Method == "POST" && BodyFactory.ContainsKey(strippedContentType) && ctx.Request.ContentLength > 0)
25.             {
26.                 body = await BodyFactory[strippedContentType](ctx.Request);
27.             }
28.  
29.             var request = new Request
30.             {
31.                 Headers = ctx.Request.Headers.ToDictionary(h => h.Key, h => h.Value.ToString()),
32.                 QueryParams = ctx.Request.Query.ToDictionary(q => q.Key, q => q.Value.ToString()),
33.                 Body = body
34.             };
35.             if (queryInPath)
36.             {
37.                 var segments = setting.Path.Split('/');
38.                 var values = ctx.Request.Path.Value.Split('/');
39.  
40.                 for (int i = 0; i < segments.Length; i++)
41.                 {
42.                     var segment = segments[i];
43.                     if(segment.Length == 0 || segment[0] != '{') continue;
44.  
45.                     request.QueryParams[segment.Trim('{', '}')] = values[i];
46.  
47.                 }
48.             }
49.  
50.             //Execute script
51.             var result = script.Execute(setting.Script, new Dictionary<string, object> {{"request", request}});
52.             var fullResponse = result as PythonDictionary;
53.             string content = null;
54.  
55.             if (fullResponse != null)
56.             {
57.                 foreach (var kvp in fullResponse["ResponseHeaders"] as PythonDictionary)
58.                     ctx.Response.Headers.Add(kvp.Key.ToString(), kvp.Value.ToString());
59.  
60.                 content = fullResponse["Content"] as string;
61.             }
62.             else
63.             {
64.                 content = result as string;
65.                 ctx.Response.ContentType = "text/plain";
66.             }
67.  
68.             //Output script result
69.             await ctx.Response.WriteAsync(content);
70.         }

To this

Code (CSharp):

1.         public async Task Invoke(HttpContext ctx, DbContext db, IScriptSession script)
2.         {
3.             var settings = await db.Set<Setting>()
4.                 .ToListAsync();
5.  
6.             var queryInPath = GetRequestSettings(ctx, settings, out var setting);
7.             if (setting == null)
8.             {
9.                 await _next(ctx);
10.                 return;
11.             }
12.  
13.             var request = await CreateRequestDto(ctx, queryInPath, setting.Path);
14.  
15.             var result = script.Execute(setting.Script, new Dictionary<string, object> {{"request", request}});
16.             await HandleScriptResult(ctx, result);
17.         }

 

My job is to get games out the door as smoothly and quickly as possible. I work fast. If you handed me that second piece of code and expected me to use it quickly, I'd expect you to stand there and quickly explain to me what it is and why you're handing it to me. If you'd just used proper commenting I could scan through it real quick, immediately understand what we're dealing with, get on more swiftly with my job, and you yourself could go back to your own.

In my experience there are people who fuss over the code, and there are people who fuss over just getting the game "fun and done" and in player's hands. It takes different types to make this all happen. If your goal is to learn and master the "correct" way of coding, then more power to you. It's much appreciated to have stellar programmers on a team, of course.

My personal choice and goal is to release games. Writing comments in my code helps me during development, and it also helps me to easily cannibalize and reuse parts of that code code base for years on end without having to continually decipher what I did four years ago. Comments help immensely. Use them or don't. Whatever.

 

That's fine I guess, but good games need good mechanics and mechanics need someone that can take the game design and the idea and implement it, if that domain is complex you will run into problems if you do not take time to design it correctly. Plus code reuse is a thing, if you have taken your time to design the domain correct it can be reused several times.

Edit: and fast is not always the fastest way to get products out the door, if you are sloppy it can come back and bite you in the ass

 

I think there's a gap between saving time and dealing with annoyance. I think that most of what programmers invest time on is avoiding annoying stuff, not actually saving time.

My preferred comments are more or less just block headers.

Code (csharp):

1.  
2.       //---------------------------
3.       // READ NEW INPUTS
4.       //---------------------------
5. ...
6.       //---------------------------
7.       // IMPACT OFFSETTING
8.       //---------------------------
9. ...
10.       //---------------------------
11.       // BEGIN COLLISION CHECCKS AND OFFSETS
12.       //---------------------------
13.  
14.  

I use them mostly to just clearly catch my eye when I'm scrolling through code.

Problem with comments is that they have a tendency to not get updated when code changes, and wrong comments are worse than no comments. So when I throw comments into something, they tend to be high level, simple, clear and intended to add hints to the reader, not 'explain the code'.

Exception being very specific and unexpected situations like ander's original. Comment justifying the code, not explaining it.

 

Absolutely agreed, but if you assume that "fast" always equates with "sloppy" then you're mistaken. I've been full time indie for twenty years, and I work fast. But, there's no way I could keep doing what I do if everything I did was sloppy. There are times when my code is less than stellar, totally(!), but it gets the job done.

When people play our games, I'm pretty sure none of them are thinking "wow I hope Jenna didn't waste any time putting comments in the code" or "wow I hope Jenna didn't half-ass this one method that handles character loading." They're likely just thinking "hey I like this," and that works for me.

 

I'm not sure that the second version is better. If you included the refactored methods in the snippit here, is there actually an improvement?

I'm a big fan of nested methods, where refactoring a method is actually valuable, but the other methods are really single use, but all in all, I think people fear long methods too much.

I know that, as an industry, we avoid long methods - but I have begun to wonder if this is often taken way too far. There is value sometimes to having every statement be a contiguous, structured and sequential block.

 

I'd add to this that refactoring code is useful if the refactored part becomes more clear and the refactored code will be reused. Keep an eye on the technical dept, but also keep an eye on execution speed.

 

Offcourse much refactor done in each part too. Humans tend to be better at follow code if it's splitted up like this.

 

Oh, I didn't mean that quite the oposite, If you write good code it's faster because less errors, more so in a large team

 

I think the question really comes down to: is the refactored code "more clear"?

This is a subjective issue, but I think people err on the side of refactoring with results that are mixed at best.

 

I think your examples should be solved with good method names, otherwise I agree with you

 

Wish I knew where this thread came from... or what were going on here.

 

I understood exactly 0% of this!

 

Show off!

 

Just be careful when you try to break up large sections of code using comments. Don't have one massive function with comment headings throughout that large function. Break that large function up in many smaller functions. Ideally, you should not need to scroll to view an entire function.

 

I disagree. There is absolutely time and place for single large methods, where splitting into more submethods does not make something better, cleaner, or more readable. We could have used Ander's refactoring as an example, but he won't paste the full 'improved' version, with all the new methods.

 

Its my career long fight against noise in code

 

Can do that when I'm at a computer
Edit: only time you might not want it is in a super tight loop since it comes with a veeeeery small cost, but that's taking it to the extremes, we are talking very close to the metal here. And not something we usually care about in the enterprise world or even in game logic/mechanics code.

 

Here it is

Code (CSharp):

1.     public class MockerMiddleware
2.     {
3.         private readonly RequestDelegate _next;
4.  
5.         public MockerMiddleware(RequestDelegate next)
6.         {
7.             _next = next;
8.         }
9.  
10.         public async Task Invoke(HttpContext ctx, ISettingsRepository repository, IScriptSession script)
11.         {
12.             var queryInPath = GetRequestSettings(ctx, await repository.ListSettings(), out var setting);
13.             if (setting == null)
14.             {
15.                 await _next(ctx);
16.                 return;
17.             }
18.  
19.             var request = await CreateRequestDto(ctx, queryInPath, setting.Path);
20.  
21.             var result = script.Execute(setting.Script, new Dictionary<string, object> {{"request", request}});
22.             await HandleScriptResult(ctx, result);
23.         }
24.  
25.         private bool GetRequestSettings(HttpContext ctx, IEnumerable<Setting> settings, out Setting setting)
26.         {
27.             var queryInPath = false;
28.             setting = settings.FirstOrDefault(s =>
29.             {
30.                 queryInPath = s.Path.AsSpan().IndexOf("{", StringComparison.Ordinal) >= 0;
31.                 return String.Equals(ctx.Request.Method, s.Method, StringComparison.CurrentCultureIgnoreCase) &&
32.                        CheckSegments(s.Path.Split('/'), ctx.Request.Path.Value.Split('/'));
33.             });
34.             return queryInPath;
35.         }
36.  
37.         private Task HandleScriptResult(HttpContext ctx, object result)
38.         {
39.             string GetResponse()
40.             {
41.                 if (result is PythonDictionary fullResponse)
42.                 {
43.                     foreach (var kvp in (PythonDictionary) fullResponse["ResponseHeaders"])
44.                         ctx.Response.Headers.Add(kvp.Key.ToString(), kvp.Value.ToString());
45.  
46.                     return fullResponse["Content"] as string;
47.                 }
48.  
49.                 ctx.Response.ContentType = "text/plain";
50.                 var str = result as string;
51.                 if (string.IsNullOrWhiteSpace(str))
52.                     ctx.Response.StatusCode = 204;
53.  
54.                 return str;
55.             }
56.  
57.             return ctx.Response.WriteAsync(GetResponse());
58.         }
59.  
60.         private async Task<Request> CreateRequestDto(HttpContext ctx, bool queryInPath, string configuredPath)
61.         {
62.             var contentType = GetContentType(ctx);
63.             var body = ctx.Request.Method == "POST" && BodyFactory.ContainsKey(contentType) && ctx.Request.ContentLength > 0 ?
64.                 await BodyFactory[contentType](ctx.Request) : null;
65.  
66.             var request = new Request
67.             {
68.                 Headers = ctx.Request.Headers.ToDictionary(h => h.Key, h => h.Value.ToString()),
69.                 QueryParams = ctx.Request.Query.ToDictionary(q => q.Key, q => q.Value.ToString()),
70.                 Body = body
71.             };
72.             if(queryInPath)
73.                 ExtractQueryParamsFromPath(ctx, configuredPath, request);
74.  
75.             return request;
76.         }
77.  
78.         private static void ExtractQueryParamsFromPath(HttpContext ctx, string configuredPath, Request request)
79.         {
80.             var segments = configuredPath.Split('/');
81.             var values = ctx.Request.Path.Value.Split('/');
82.  
83.             segments
84.                 .Zip(values, (s, v) => (s.AsSpan().StartsWith("{") ? s.Trim('{', '}') : null, v))
85.                 .Where(zip => zip.Item1 != null)
86.                 .AddTo(request.QueryParams);
87.         }
88.  
89.         private string GetContentType(HttpContext ctx)
90.         {
91.             var contentTypeDelim = ctx.Request.ContentType?.AsSpan().IndexOf(";", StringComparison.Ordinal);
92.             var strippedContentType = contentTypeDelim.HasValue && contentTypeDelim >= 0 ? ctx.Request.ContentType.Substring(0, contentTypeDelim.Value) : ctx.Request.ContentType;
93.  
94.             return strippedContentType;
95.         }
96.  
97.         private bool CheckSegments(string[] expectedSegments, string[] actualSegments)
98.         {
99.             if (expectedSegments.Length != actualSegments.Length) return false;
100.  
101.             return expectedSegments.Select((e, i) => e.Length == 0 || e[0] == '{' || e == actualSegments[i]).All(b => b);
102.         }
103.  
104.         private static async Task<object> HandleBodyAsString(HttpRequest request, Func<string, object> factory)
105.         {
106.             using (var reader = new StreamReader(request.Body))
107.             {
108.                 var content = await reader.ReadToEndAsync();
109.                 return factory(content);
110.             }
111.         }
112.  
113.         private static readonly Func<HttpRequest, Task<object>> FormFactory = async request =>
114.         {
115.             var files = await Task.WhenAll(request.Form.Files.Select(async f =>
116.             {
117.                 using (var stream = f.OpenReadStream())
118.                 using (var reader = new StreamReader(stream, Encoding.GetEncoding(1252), true))
119.                 {
120.                     return await reader.ReadToEndAsync();
121.                 }
122.             }));
123.  
124.             var body = request.Form.ToDictionary(f => f.Key, f => f.Value.ToString() as object);
125.             body["files"] = files;
126.             return body;
127.         };
128.  
129.         private static readonly Dictionary<string, Func<HttpRequest, Task<object>>> BodyFactory = new Dictionary<string, Func<HttpRequest, Task<object>>>
130.             {
131.                 {
132.                     "application/json",
133.                     request => HandleBodyAsString(request, JObject.Parse)
134.                 },
135.                 {
136.                     "application/xml",
137.                     request => HandleBodyAsString(request, body =>
138.                     {
139.                         var doc = XDocument.Parse(body);
140.                         var jsonText = JsonConvert.SerializeXNode(doc);
141.                         return JObject.Parse(jsonText);
142.                     })
143.                 },
144.                 {
145.                     "multipart/form-data",
146.                     FormFactory
147.                 },
148.                 {
149.                     "application/x-www-form-urlencoded",
150.                     FormFactory
151.                 }
152.             };
153.     }

 

Wait. Do people actually use algorithms in their code with Unity? I'm half kidding here, I've rarely had to implement anything algorithmic because almost everything I've done has been OO or handled by the Unity Engine. I kind of long to have something worth implementing in that regard...

 

Its probably not very common, atleast in c# land, maybe more common if you are doing compute shaders etc

 

It's because he doesn't have a blog.

 

I do think this is a good example. You took a tight sequential 70 lines of code, and made it into something closer to 120 lines, broken up into 9 separate methods.
Side by side comparison:

Code (CSharp):

1.     public class MockerMiddleware
2.     {
3.         private readonly RequestDelegate _next;
4.         public MockerMiddleware(RequestDelegate next)
5.         {
6.             _next = next;
7.         }
8.         public async Task Invoke(HttpContext ctx, ISettingsRepository repository, IScriptSession script)
9.         {
10.             var queryInPath = GetRequestSettings(ctx, await repository.ListSettings(), out var setting);
11.             if (setting == null)
12.             {
13.                 await _next(ctx);
14.                 return;
15.             }
16.             var request = await CreateRequestDto(ctx, queryInPath, setting.Path);
17.             var result = script.Execute(setting.Script, new Dictionary<string, object> {{"request", request}});
18.             await HandleScriptResult(ctx, result);
19.         }
20.         private bool GetRequestSettings(HttpContext ctx, IEnumerable<Setting> settings, out Setting setting)
21.         {
22.             var queryInPath = false;
23.             setting = settings.FirstOrDefault(s =>
24.             {
25.                 queryInPath = s.Path.AsSpan().IndexOf("{", StringComparison.Ordinal) >= 0;
26.                 return String.Equals(ctx.Request.Method, s.Method, StringComparison.CurrentCultureIgnoreCase) &&
27.                        CheckSegments(s.Path.Split('/'), ctx.Request.Path.Value.Split('/'));
28.             });
29.             return queryInPath;
30.         }
31.         private Task HandleScriptResult(HttpContext ctx, object result)
32.         {
33.             string GetResponse()
34.             {
35.                 if (result is PythonDictionary fullResponse)
36.                 {
37.                     foreach (var kvp in (PythonDictionary) fullResponse["ResponseHeaders"])
38.                         ctx.Response.Headers.Add(kvp.Key.ToString(), kvp.Value.ToString());
39.                     return fullResponse["Content"] as string;
40.                 }
41.                 ctx.Response.ContentType = "text/plain";
42.                 var str = result as string;
43.                 if (string.IsNullOrWhiteSpace(str))
44.                     ctx.Response.StatusCode = 204;
45.                 return str;
46.             }
47.             return ctx.Response.WriteAsync(GetResponse());
48.         }
49.         private async Task<Request> CreateRequestDto(HttpContext ctx, bool queryInPath, string configuredPath)
50.         {
51.             var contentType = GetContentType(ctx);
52.             var body = ctx.Request.Method == "POST" && BodyFactory.ContainsKey(contentType) && ctx.Request.ContentLength > 0 ?
53.                 await BodyFactory[contentType](ctx.Request) : null;
54.             var request = new Request
55.             {
56.                 Headers = ctx.Request.Headers.ToDictionary(h => h.Key, h => h.Value.ToString()),
57.                 QueryParams = ctx.Request.Query.ToDictionary(q => q.Key, q => q.Value.ToString()),
58.                 Body = body
59.             };
60.             if(queryInPath)
61.                 ExtractQueryParamsFromPath(ctx, configuredPath, request);
62.             return request;
63.         }
64.         private static void ExtractQueryParamsFromPath(HttpContext ctx, string configuredPath, Request request)
65.         {
66.             var segments = configuredPath.Split('/');
67.             var values = ctx.Request.Path.Value.Split('/');
68.             segments
69.                 .Zip(values, (s, v) => (s.AsSpan().StartsWith("{") ? s.Trim('{', '}') : null, v))
70.                 .Where(zip => zip.Item1 != null)
71.                 .AddTo(request.QueryParams);
72.         }
73.         private string GetContentType(HttpContext ctx)
74.         {
75.             var contentTypeDelim = ctx.Request.ContentType?.AsSpan().IndexOf(";", StringComparison.Ordinal);
76.             var strippedContentType = contentTypeDelim.HasValue && contentTypeDelim >= 0 ? ctx.Request.ContentType.Substring(0, contentTypeDelim.Value) : ctx.Request.ContentType;
77.             return strippedContentType;
78.         }
79.         private bool CheckSegments(string[] expectedSegments, string[] actualSegments)
80.         {
81.             if (expectedSegments.Length != actualSegments.Length) return false;
82.             return expectedSegments.Select((e, i) => e.Length == 0 || e[0] == '{' || e == actualSegments[i]).All(b => b);
83.         }
84.         private static async Task<object> HandleBodyAsString(HttpRequest request, Func<string, object> factory)
85.         {
86.             using (var reader = new StreamReader(request.Body))
87.             {
88.                 var content = await reader.ReadToEndAsync();
89.                 return factory(content);
90.             }
91.         }
92.         private static readonly Func<HttpRequest, Task<object>> FormFactory = async request =>
93.         {
94.             var files = await Task.WhenAll(request.Form.Files.Select(async f =>
95.             {
96.                 using (var stream = f.OpenReadStream())
97.                 using (var reader = new StreamReader(stream, Encoding.GetEncoding(1252), true))
98.                 {
99.                     return await reader.ReadToEndAsync();
100.                 }
101.             }));
102.             var body = request.Form.ToDictionary(f => f.Key, f => f.Value.ToString() as object);
103.             body["files"] = files;
104.             return body;
105.         };
106.         private static readonly Dictionary<string, Func<HttpRequest, Task<object>>> BodyFactory = new Dictionary<string, Func<HttpRequest, Task<object>>>
107.             {
108.                 {
109.                     "application/json",
110.                     request => HandleBodyAsString(request, JObject.Parse)
111.                 },
112.                 {
113.                     "application/xml",
114.                     request => HandleBodyAsString(request, body =>
115.                     {
116.                         var doc = XDocument.Parse(body);
117.                         var jsonText = JsonConvert.SerializeXNode(doc);
118.                         return JObject.Parse(jsonText);
119.                     })
120.                 },
121.                 {
122.                     "multipart/form-data",
123.                     FormFactory
124.                 },
125.                 {
126.                     "application/x-www-form-urlencoded",
127.                     FormFactory
128.                 }
129.             };
130.     }

And here is the single method version:

Code (CSharp):

1.         public async Task Invoke(HttpContext ctx, DbContext db, IScriptSession script)
2.         {
3.             var settings = await db.Set<Setting>()
4.                 .ToListAsync();
5.             //Check if a setting exists for a request
6.             var queryInPath = false;
7.             var setting = settings.FirstOrDefault(s =>
8.             {
9.                 queryInPath = s.Path.IndexOf("{", StringComparison.Ordinal) >= 0;
10.                 return ctx.Request.Method.ToLower() == s.Method.ToLower() && CheckSegments(s.Path.Split('/'), ctx.Request.Path.Value.Split('/'));
11.             });
12.             if (setting == null)
13.             {
14.                 await _next(ctx);
15.                 return;
16.             }
17.             //Create a request DTO
18.             object body = null;
19.             var contentTypeDelim = ctx.Request.ContentType?.IndexOf(";", StringComparison.Ordinal);
20.             var strippedContentType = contentTypeDelim.HasValue && contentTypeDelim >= 0 ? ctx.Request.ContentType.Substring(0, contentTypeDelim.Value) : ctx.Request.ContentType;
21.             if (ctx.Request.Method == "POST" && BodyFactory.ContainsKey(strippedContentType) && ctx.Request.ContentLength > 0)
22.             {
23.                 body = await BodyFactory[strippedContentType](ctx.Request);
24.             }
25.             var request = new Request
26.             {
27.                 Headers = ctx.Request.Headers.ToDictionary(h => h.Key, h => h.Value.ToString()),
28.                 QueryParams = ctx.Request.Query.ToDictionary(q => q.Key, q => q.Value.ToString()),
29.                 Body = body
30.             };
31.             if (queryInPath)
32.             {
33.                 var segments = setting.Path.Split('/');
34.                 var values = ctx.Request.Path.Value.Split('/');
35.                 for (int i = 0; i < segments.Length; i++)
36.                 {
37.                     var segment = segments[i];
38.                     if(segment.Length == 0 || segment[0] != '{') continue;
39.                     request.QueryParams[segment.Trim('{', '}')] = values[i];
40.                 }
41.             }
42.             //Execute script
43.             var result = script.Execute(setting.Script, new Dictionary<string, object> {{"request", request}});
44.             var fullResponse = result as PythonDictionary;
45.             string content = null;
46.             if (fullResponse != null)
47.             {
48.                 foreach (var kvp in fullResponse["ResponseHeaders"] as PythonDictionary)
49.                     ctx.Response.Headers.Add(kvp.Key.ToString(), kvp.Value.ToString());
50.                 content = fullResponse["Content"] as string;
51.             }
52.             else
53.             {
54.                 content = result as string;
55.                 ctx.Response.ContentType = "text/plain";
56.             }
57.             //Output script result
58.             await ctx.Response.WriteAsync(content);
59.         }

Personally, I find the single method version easier to read, clearer and more straight forward.

One could argue that the refactored code is better if the methods are being reused, and that could certainly be true, but the implementation marks all as private, with no other use case. So the code itself is saying that there will not be reuse.

There are arguments for the refactored code. I give respect to @AndersMalmgren for picking 100% top quality method names (that's really important and a sign of professionalism). But I don't think the refactored example is a clear win, and I would personally prefer working with the sequential single method.

 

It's actually only 83 lines since that body factory stuff and the constructor is in the none refactored too

But sure it's more code and also potentially somewhat slower because extra awaits (I would say the method call overhead is neglectable in this case).

I don't have statistics for it but I would bet money on that the majority like short tight methods over longer ones. Let's say a dev that never have seen the code gets tasked with investigating why requests some sometimes get malformatted, he probally right away will look inside the CreateRequestDto method. Dumb example I know but it's more relevant in a larger class or system. Talking about class sizes I think the sweet point is not larger than 100 lines, if it's bigger try to make it smaller by composition etc.

 

I don't have statistics either, and honestly this may simply come down to personal preference, or how different people's brains are wired differently. But I personally would prefer to work with the single method version in this specific example.

If I was a completely new team member and came across the code blind, I think the single method example is actually clearer and more concise, there is less file navigation needed to see exactly what's happening, and it's all written out visually in the sequence its executed in.

End of the day, I can read the code from top to bottom in a single screen and see all the relevant variables. Refactored example, I need to use more brain power to fully understand the process. Since its not written top to bottom, you need to jump around, and that takes more brain energy.

I am not arguing that original code was 100% 'better' - but I think there are pros and cons for both, and I don't think the refactored code is a clear cut improvement - just a rearrangement.

 

Damn reading this makes me think how much I miss python. Shame it isn't performant.

 

With multiple methods I have to keep skipping around to get a grip on what's happening. After a few jumps and skips I sometimes loose track and then have to start over, jotting the sequence down in my notes so I know what the heck is going on. With one method, it's one or two scrolls. Done.

And even though I know that, I'm still guilty of splitting things up into multiples, trying to keep things tidy. It's a constant battle, I tell ya.

 

It's the 1323rd iteration of the discussion about

- everyone needs enterprise-grade code-architecture
vs
- not everyone needs enterprise-grade code-architecture

Obviously not everyone and not every project need enterprise-grade code architecture.