C# program in need of critique - will continuously update for critique

[wauterboi]

Hey - I'm a C# noob. And while I'm enjoying the language and getting the hang of some things, I still think I'm far from writing clean, sensible code. I'm still working on code style and following the C# way of solving problems.

Files · master · waut3r / GameEngine · GitLab

Here's some notes:
It's going to be based on SFML.
It's all done in Visual Studio Code with .NET Core because I'm a heathen. Do make fun of me for that and feel free to shame me into going back to Visual Studio.
It has a Configuration class that reads and writes to .json
It has a custom logging class for timestamps, colors, readkey/pausing, etc.

I've got 102 coins, so I'd be willing to share them (at a variable rate) with people interested in giving any advice. I've previously made attempts to make a game engine in C#, but I'm starting over and trying to bring the critique in early on while it's small and receiving gradual changes. I'm eager to write clean-looking and efficient code, so it means a lot.

[garry]

There's not really a lot there to crit.

Use newlines before {
It's okay to have the parameters on one line
You can use "var" instead of the whole type name when defining locals
A preference thing, but generally private vars start with lowercase, public with upper. No need for the _ in front.

[wauterboi]

Thanks! I'm generally afraid that I'm doing everything completely wrong. Maybe I just beat myself up from hanging around hardass developers who criticize anyone who's not using <choice of editor> and <choice of programming language>.

Do C# developers really have any conventions for line width? Or, rather, do you personally? With other languages I use, it's always suggested to keep stuff under 80 characters in width, but I wonder if that's a legacy left behind by classic programmers and their old CRT monitors.

[proboardslol]

DON'T use newlines before {
When you have a call like:
Log.WriteLine("Configuration could not be initialized",
   Log.Type.Error,
   exception,
   true
);
You can make them simpler to call by creating a method like Log.Error("message") or Log.Exception("Message")

[helifreak]

There's no hard limit on line length but don't go crazy and start doing 2-300 character lines if you can avoid it. Despite it now fitting on modern displays it's not as easy to read when you have to scan that far out, and there's weird motherfuckers like me that code in portrait (1440x2560) but even then I still get ~230 chars across. We'll ignore my coworker with the 34" ultrawide.

Across our ~1 million line code base very little ends up going over 200. This does include some auto generated stuff but I can't be bothered filtering it out, we're still a few thousand off the first million.

https://files.facepunch.com/forum/upload/494/11130ffb-903c-4cb7-81fe-67e5892c11df/image.png

Underscores are a preference thing really, we use them at work to separate locals (lowerCamelCase) and class privates (_lowerCamelCase), then publics are UpperCamelCase as standard.

        private Dictionary<string, string> _userData =
            new Dictionary<string, string>();
        public Dictionary<string, string> UserData {
            get => _userData;
            private set => _userData = value;
        }
This is pretty wordy and can be replaced with this:
private Dictionary<string, string> UserData { get; private set; }
Good work using expression bodies though.

[Tamschi]

One criticism is maybe that your configuration is a Dictionary<string, string> and built similarly.
C# has really good reflection, so you can write out your config (including defaults) as C# class(es) or structure(s)¹. The reader can then type-check your input automatically.

There is a bit of a performance penalty for doing reflection, but it's far lower than what you'd get for repeatedly parsing all those values later.
It also makes sure the compiler will catch any misspellings in places where you access the configuration.

¹ There's downsides to either approach, classes are the more general and easy solution.

[proboardslol]

To put it simply, var is staticly typed: once you instantiate a variable with var, it will never be any other type. The keyword `dynamic` lets you change the type of the variable, like:

dynamic x = 10;
x = "hello world!";

You can't do this with var:

var x = 10;
x = "hello world!"; //type error: "hello world" is a string but "x" is an int

[Tamschi]

That can be
Dictionary<string, string> UserData { get; } = new Dictionary<string, string>();
now in this case, if I'm not mistaken.

[proboardslol]

That doesn't mean anything to me. I think styling is a personal choice, just be consistent about it. Especially since C#/.Net Core are open source/run on linux, macOS, etc., Intellisense is not a valid excuse for formatting

[Tamschi]

Hit the format document key combo once in a while (or get an extension that does it before saving).
You've still got inconsistent line breaks around { and } in at least one file.

In (a German) VS installation, it's Ctrl + E, D by default. (Not Ctrl + E + D.)

[proboardslol]

Personally I don't like to pick peoples' code apart for styling and syntactical choices when they're learning because I think it's more important to get shit to work first before you start worrying about a code review process

[Tamschi]

Intellisense is something entirely different
But I agree with you. It might raise a few eyebrows, but consistency is key.

It might (probably will, really) throw off contributors, though.

[proboardslol]

Also, as for line length: At my company we encourage a max of 120 characters because we use github and github's column width for code views is 120 characters

[Protocol7]

I don't really prefer it one way or another, but why not just use the default C# coding conventions?

Personally I enjoy the spacing, pair it with a VS extension like Viasfora and it's super easy to read code.

[proboardslol]

Because I don't particularly care about Microsoft's opinion on what my code should look like. I prefer it my way and that's the cool thing about writing personal projects: nobody can tell me what I can and cannot do.

Additionally I don't use Visual Studio, I use either vim or vscode depending on what laptop I'm using, so I don't have any formatting extensions

[Protocol7]

You don't have the C# extension? Which provides a lot more than just formatting?

C# - Visual Studio Marketplace

Part of the popularity of C# is the excellent devtools, which this extension very closely brings VSCode into functional parity with Visual Studio itself.

[proboardslol]

I have to C# extension but there's no formatting with it, or maybe I disabled it

[Protocol7]

It's not auto, you have to right click and format.

[brianosaur]

I have to agree with proboardslol. I hate having a newline for just one character. It just makes your files longer than necessary for no reason.

I think c# is the only language where this is even a thing. But overall its project dependent. If there's a newline after every function, I'm going to go with the project conventions because of consistency. If I was starting a new c# project myself, you bet your ass I'm putting the curly brace on the same line.

[garry]

It's definitely not just c#. I'd go as far as to say that 99% of c#, c or c++ code you see will have curly brackets on newlines.

[Goz3rr]

Also consider reading the Framework Design Guidelines

[NoRegard]

Might as well just learn the format for whatever language you're writing. Newline for curlies in C#, no new line in Javascript.

[proboardslol]

If 99% of C++ coders jumped off a bridge would you do it too

[brianosaur]

gotcha, honestly I don't develop much in C family languages. I work for a web development consultancy, so I develop in A LOT of languages, so if a company is using a c family language, its likely C# and they are a big company. I try to avoid the enterprise-y (Java, C#) clients if possible

[wauterboi]

Definitely my intention to have a new line before brackets! Just occasionally miss a few.
Redid Log to have a main method that does all the work, and then convenience methods WriteLog, WriteSuccess, WriteWarning, and WriteRror
120 characters is going to be the maximum width for my code
Simplified DefaultData and UserData into properties only. What's a good reason to have backing fields with properties, by the way?
Found out that VSCode already supports auto-formatting.

[NoRegard]

We're comparing a style of coding with potentially committing suicide now? It's hardly the same thing.

[Vivelin]

Auto-implemented properties weren't introduced until C# 3.0. A typical use case is implementing events:

private string message;

public event EventHandler MessageChanged;

public string Message
{
get { return message; }
set
{
if (message != value)
{
message = value;
MessageChanged?.Invoke(new EventArgs());
}
}
}

[helifreak]

C# 3 came out in 2007 for the record.

[BepisMax]

I use the single-line dedented style myself

#include <stdio.h>

int main(int argc, char **argv) {
if (argc < 1000) {
printf("Not enough arguments!");
return -1;
}
return 0;
}

[Kidd]

I use something called Code Maid for VS. Every time I save it formats the code the way I want. Best part is it's free.