3 ms·

Very good effort! RE: modern C# 1) you can use pattern matching instead of switching 2) you can use native named tuples instead of Item1/Item2 ones

andix 5y ago
I thought the same, the c# Code is not very modern. It is possible to make it even nicer. But I guess it’s a great start, maybe I’ll submit a pull request :)
- eatonphil 5y ago
  Please somebody do! I'd like to see what more I've missed. Some cool things in there already were records and top level statements.
  - zigzag312 5y ago
    For example, here's IMO a bit cleaner Sexp record: record Sexp(SexpKind Kind, Token Atom, (Sexp First, Sexp Second)? Pair) { public String Pretty() { if (Kind == SexpKind.Atom) return Atom.Value; const string nil = "NIL"; return $"({Pair?.First.Pretty() ?? nil} . {Pair?.Second?.Pretty() ?? nil})"; } public static Sexp Append(Sexp first, Sexp second) { if (first == null) return new (SexpKind.Tuple, null, (second, null)); if (first.Kind == SexpKind.Atom) return new (SexpKind.Tuple, null, (first, second)); return new (SexpKind.Tuple, null, (first.Pair?.First, Append(first.Pair?.Second, second))); } } One issue with original code is that it assumes 'pair' tuple is not null in most places while it is initialized to null at the beginning. I haven't had the time to dive into the code to see how to properly refactor this. To get more modern C# I would: - turn on nullable reference types for the project (it takes some time at first to get used to it) - replace all Tuple.Create calls with new ValueTuple shorthand: (val1, val2) (Tuple is old class based tuple, ValueTuple is modern struct based tuple type that is integrated into the language) Code feels very imperative to me. While this is good for performance, using more functional style would probably simplify code a bit more. I'm not sure it is possible though, as I haven't tried to refactor it myself.

andix 5y ago

I thought the same, the c# Code is not very modern. It is possible to make it even nicer. But I guess it’s a great start, maybe I’ll submit a pull request :)

eatonphil 5y ago
Please somebody do! I'd like to see what more I've missed. Some cool things in there already were records and top level statements.
- zigzag312 5y ago
  For example, here's IMO a bit cleaner Sexp record: record Sexp(SexpKind Kind, Token Atom, (Sexp First, Sexp Second)? Pair) { public String Pretty() { if (Kind == SexpKind.Atom) return Atom.Value; const string nil = "NIL"; return $"({Pair?.First.Pretty() ?? nil} . {Pair?.Second?.Pretty() ?? nil})"; } public static Sexp Append(Sexp first, Sexp second) { if (first == null) return new (SexpKind.Tuple, null, (second, null)); if (first.Kind == SexpKind.Atom) return new (SexpKind.Tuple, null, (first, second)); return new (SexpKind.Tuple, null, (first.Pair?.First, Append(first.Pair?.Second, second))); } } One issue with original code is that it assumes 'pair' tuple is not null in most places while it is initialized to null at the beginning. I haven't had the time to dive into the code to see how to properly refactor this. To get more modern C# I would: - turn on nullable reference types for the project (it takes some time at first to get used to it) - replace all Tuple.Create calls with new ValueTuple shorthand: (val1, val2) (Tuple is old class based tuple, ValueTuple is modern struct based tuple type that is integrated into the language) Code feels very imperative to me. While this is good for performance, using more functional style would probably simplify code a bit more. I'm not sure it is possible though, as I haven't tried to refactor it myself.