Feature: Tweaks to allow new SoilTest/NBalanceSummary to be used on the webapp #47
Conversation
SVSModel/Configuration/Constants.cs
Outdated
| @@ -14,7 +12,7 @@ public static class Constants | |||
| public const double InitialN = 50; | |||
|
|
|||
| /// <summary>Dictionary containing values for the proportion of maximum DM that occurs at each predefined crop stage</summary> | |||
| public static readonly Dictionary<string, double> PropnMaxDM = new() | |||
| public static readonly Dictionary<string, double> PropNMaxDM = new() | |||
There was a problem hiding this comment.
Not major but this change is not really correct. the lower case n was part of a shortening of proportion. Changing it to upper case infers nitrogen which is not correct for this variable. Would be better to remove the n completely
SVSModel/Configuration/Constants.cs
Outdated
| /// <summary>Dictionary containing values for the proportion of thermal time to maturity that has accumulate at each predefined crop stage</summary> | ||
| public static readonly Dictionary<string, double> PropnTt = new() | ||
| /// <summary>Dictionary containing values for the proportion of thermal time to maturity that has accumulated at each predefined crop stage</summary> | ||
| public static readonly Dictionary<string, double> PropNTt = new() |
There was a problem hiding this comment.
Same with this one, delete the N
| double soilDepthFactor = SampleDepthFactor[DepthOfSample]; | ||
| double result = TestValue / soilMF * BulkDensity * 3 * soilDepthFactor; | ||
| return new Dictionary<DateTime, double>() { { TestDate, result } }; | ||
| if (TypeOfTest == TestType.QuickTest.ToString()) |
There was a problem hiding this comment.
could just replace TestType.QuickTest.ToString() with "QuickTest". The longer winded way is a hangover from when I was using enums
There was a problem hiding this comment.
I think I am fine with leaving it like this for now.
Using enums is certainly cleaner in the long run, but it will require some refactoring on the webapp backend to accommodate.
| public double CropIn { get; } | ||
| public double ResidueIn { get; } | ||
| public double SOMIn { get; } | ||
| public double FertiliserIn { get; } |
There was a problem hiding this comment.
Don't these properties need to have setters on them as well so they can be set by the constructor.
There was a problem hiding this comment.
The setters are only required if you intend to set values outside of the constructor.
Initially this class had no getters or setters, so the webapp was not able to actually get the values.
There was a problem hiding this comment.
I didn't know that but it makes sense when you think about it.
There was a problem hiding this comment.
A few things to check @Nibble-Byte, mostly minor.
I will pull your branch and check the everything still works OK in the excel testing system
@HamishBrownPFR Mostly cosmetic changes on this one. Please let me know if you would like to have a chat.
I would also advise we don't merge this one until you are happy with the webapp.