search

dotnet / samples

Sample code referenced by the .NET documentation
https://docs.microsoft.com/samples/browse
Creative Commons Attribution 4.0 International
3.32k stars 5.04k forks source link

Pattern docs can be improved #6893

Open DickBaker opened 2 months ago

DickBaker commented 2 months ago

Pattern docs at samples\csharp\tutorials\patterns can be improved ...

  1. the samples\csharp\tutorials\patterns\start\toll-calculator.csproj does not compile without TollCalculator implementation
  2. attempting to add samples\csharp\tutorials\patterns\finished\toll_calculator.csproj as shared project fails with duplicate project names
  3. changing project names [e.g. to Toll-start.csproj, Toll-finished.csproj] complains due to conflicting ExternalSystems
  4. classes in ExternalSystems.cs should have default properties or marked required [e.g. otherwise new DeliveryTruck() inits GrossWeightClass=0 !] although better to have explicit ctors with initializer for all props [maybe derive from external source to enforce]
  5. should be simplified to "public static class TollCalculator{..}" so don't need concrete tollCalc instance
  6. CalculateToll parameter should be nullable [i.e. public decimal CalculateToll(object? vehicle) => ...], and change calls from null! to null.
  7. perhaps fullVan and fullVanPool should be fullCar and fullTaxi respectively ("full van" a peculiarly American phrase) also "GrossWeightClass" -> "GrossWeight" since unit is lbs as int
  8. code is littered with magic numbers (e.g. variously as 2.00m or 2.0m) when should instead be defined in ONE place (e.g. as const decimal CarFare=2.00m)
  9. "The null pattern can be last because the other patterns match only a non-null object of the correct type." is non-optimal in switch expression the null check should happen once [first] otherwise each "is" typecast would have to perform its own null guard compare
  10. inefficient switch arms in CalculateToll for Bus, DeliveryTruck [repeated typecast executions]
  11. final (catch-all) arm [discard pattern] can be simplified to => [e.g. not (, , )]
  12. IsWeekDay can be simplified
  13. PeakTimePremiumIfElse compares hour < 20 whereas GetTimeBand uses > 19 [needs more brain cycles to verify equivalence]
  14. need more vehicle types and/or catch-all [can't just declare unknown!]. e.g. see NYC CRZ at https://congestionreliefzone.mta.info/tolling practical to accurately assess GrossWeightClass without an actual weighbridge stop ?
  15. unnecessary subfolder hierarchy [e.g. simplify start/toll-calculator/ to start-calculator/]
  16. should update TFM [i.e. "net6.0" to "net8.0"] on all projects

I am in process of crafting a PR to suggest how I think docs and sample code should look.

BillWagner commented 2 months ago

Hi @DickBaker

There are several good suggestions here:

  1. the samples\csharp\tutorials\patterns\start\toll-calculator.csproj does not compile without TollCalculator implementation
  2. classes in ExternalSystems.cs should have default properties or marked required [e.g. otherwise new DeliveryTruck() inits GrossWeightClass=0 !] although better to have explicit ctors with initializer for all props [maybe derive from external source to enforce]

I like the required properties suggestion.

  1. should be simplified to "public static class TollCalculator{..}" so don't need concrete tollCalc instance
  2. perhaps fullVan and fullVanPool should be fullCar and fullTaxi respectively ("full van" a peculiarly American phrase) also "GrossWeightClass" -> "GrossWeight" since unit is lbs as int
  3. code is littered with magic numbers (e.g. variously as 2.00m or 2.0m) when should instead be defined in ONE place (e.g. as const decimal CarFare=2.00m)
  4. final (catch-all) arm [discard pattern] can be simplified to => [e.g. not (, , )]
  5. IsWeekDay can be simplified
  6. PeakTimePremiumIfElse compares hour < 20 whereas GetTimeBand uses > 19 [needs more brain cycles to verify equivalence]
  7. should update TFM [i.e. "net6.0" to "net8.0"] on all projects

And a few that I'd prefer not be changed:

  1. attempting to add samples\csharp\tutorials\patterns\finished\toll_calculator.csproj as shared project fails with duplicate project names
  2. changing project names [e.g. to Toll-start.csproj, Toll-finished.csproj] complains due to conflicting ExternalSystems
  3. unnecessary subfolder hierarchy [e.g. simplify start/toll-calculator/ to start-calculator/]

This is by design. The goal is for readers to load the starter project, follow along with the companion article, and then compare their results with the finished code. These changes would make that harder.

  1. CalculateToll parameter should be nullable [i.e. public decimal CalculateToll(object? vehicle) => ...], and change calls from null! to null.

Nope. This is an important technique to demonstrate. The API declares that the argument should not be null. However, defensive coding practices mandate checking for and testing null values.

  1. "The null pattern can be last because the other patterns match only a non-null object of the correct type." is non-optimal in switch expression the null check should happen once [first] otherwise each "is" typecast would have to perform its own null guard compare
  2. inefficient switch arms in CalculateToll for Bus, DeliveryTruck [repeated typecast executions]

Nope. The lowered code combined with the runtime JIT will optimize these. It's not going to be a measurable performance improvement.

And finally, one "it depends"

  1. need more vehicle types and/or catch-all [can't just declare unknown!]. e.g. see NYC CRZ at https://congestionreliefzone.mta.info/tolling practical to accurately assess GrossWeightClass without an actual weighbridge stop ?

I'm concerned with growing the sample in ways that obscures the core learning concepts. I can see room for more cases, but I don't want to lose the plot.

DickBaker commented 2 months ago

2-4. OK I will have to back-pedal my changes to keep your folder structure before I PR [hope to alot time this w/e]

  1. Looks awry to me having CalculateToll(object vehicle) but then testing is null. Yes I get defensive coding to avoid bad calls. seems to me a case for CalculateToll([AllowNull]object vehicle) to show why we are doing that guard test 9-10. I argue doing null arm first still best style as happy-path to eliminate bandits. shouldn't need JIT nanny to wipe your ar$e and you're right NEITHER of us have conducted objective measurement. ask MadsT/JaredP/EricL/etc ?
  2. yup I go with KISS but thought you'd enjoy that NYC URL !