Connascence Discussion for ObservationAnalyticsProvider.cs

Original Issue

The ObservationAnalyticsProvider.cs class had connascence issues:

• Connascence of Type and Locality: Internal operations used string (UnitName) for units, lacking type safety.
• Connascence of Name: Public interfaces and external calls inconsistently used UnitName, but internal logic could benefit from a stronger type.

The goal was to refactor the class to use Unit (a domain model) for internal operations and UnitName (string) for public interfaces and external calls.

Refactored Approach

The refactored ObservationAnalyticsProvider.cs:

• Uses Unit for internal operations (e.g., GetStandardUnit, ConvertValueAsync, CalculateAverageRateAsync) to improve type safety.
• Retains UnitName for public methods (e.g., GetObservationChangeReportAsync), constructor, and external calls (e.g., _unitConverter.ConvertAsync) to maintain compatibility.
• Key changes:
  • supportedUnits is now List<Unit> instead of List<string>.
  • standardUnit and selectedDisplayUnit are Unit instead of string.
  • Private methods use Unit parameters and return types, accessing UnitName only for external interactions.
  • ObservationChangeReport outputs StandardUnitName and DisplayUnitName as strings for external consumers.

Antithesis: Using UnitName Everywhere

The opposing approach would use UnitName (strings) universally:

• All unit-related operations, both internal and external, use string for unit identifiers.
• No Unit type is used; only UnitName is extracted from database Unit entities.
• This prioritizes simplicity and Connascence of Name throughout the class.

Comparison of Approaches

Correctness

• New Approach (Unit Internally, UnitName Externally):
  • Pros:
    • Type safety with Unit ensures only valid units are used, reducing errors (e.g., typos like “Kilogram” vs. “Kilograms”).
    • Encapsulates unit metadata in Unit, enforcing domain constraints.
    • Maintains external compatibility with UnitName.
  • Cons:
    • Slight complexity from converting between Unit and UnitName for external calls.
    • Potential maintenance if Unit model changes (e.g., UnitName property renamed).
• Antithesis (UnitName Everywhere):
  • Pros:
    • Simpler, as only strings are used, avoiding type conversions.
    • No need to extract UnitName from Unit.
  • Cons:
    • Higher error risk due to lack of type safety (e.g., invalid unit names).
    • No enforcement of valid units without extra validation.
• Verdict: New approach is stronger for correctness due to type safety, critical in a domain like analytics.

Maintainability

• New Approach:
  • Pros:
    • Easier to extend unit logic (e.g., adding Unit properties like IsMetric).
    • Clear separation: Unit for internal logic, UnitName for external interfaces.
    • Centralized unit validation in Unit model or database queries.
  • Cons:
    • More code to maintain due to type conversions.
    • Small learning curve for Unit vs. UnitName distinction.
• Antithesis:
  • Pros:
    • Less code, as only strings are used.
    • Simpler for developers familiar with string-based logic.
  • Cons:
    • Harder to maintain as system grows (e.g., adding unit rules requires scattered string checks).
    • Refactoring unit names (e.g., “DegreesCelsius” to “Celsius”) risks errors.
• Verdict: New approach Villeins for maintainability in complex systems; antithesis is brittle.

Performance

• New Approach:
  • Pros:
    • Minimal overhead from using Unit objects vs. strings.
    • Database queries unchanged, as Unit entities are already fetched.
  • Cons:
    • Slight overhead from accessing Unit.UnitName, but negligible.
  • Antithesis:
    • Pros:
    • Marginally faster due to avoiding object creation/property access.
    • No type conversion overhead.
  • Cons:
    • Potential performance hit from runtime string validation.
    • Errors from invalid unit names could cause costly failures.
• Verdict: Performance differences are negligible; new approach avoids repetitive validation.

Context-Specific Considerations

• Domain Complexity:
  • New approach aligns with domain-driven design, treating Unit as a first-class entity, suitable for critical domains (e.g., medical analytics).
  • Antithesis may suffice for simple domains with minimal unit logic.
• Team Expertise:
  • New approach suits teams experienced with type-safe programming.
  • Antithesis is easier for less experienced teams but risks technical debt.
• System Scale:
  • New approach scales better for large systems with evolving unit features.
  • Antithesis is viable for small prototypes or scripts.
• External Dependencies:
  • New approach aligns with _unitConverter requiring UnitName, but could improve if Unit is supported.
  • Antithesis matches current external API but loses internal robustness.

Is the New Approach Right?

• Yes, likely the right choice:
  • Robustness: Reduces errors in critical domains.
  • Scalability: Supports future growth (e.g., new unit properties).
  • Maintainability: Encapsulates unit logic, easing refactoring.
  • Best Practices: Follows domain-driven design and type safety.
• When Antithesis Might Be Better:
  • Prototyping or small projects with minimal unit logic.
  • Legacy systems where strings are pervasive, and refactoring is costly.
  • Rare performance-critical scenarios (unlikely here).
• Mitigating Risks:
  • Validate Unit model (e.g., ensure lightweight, well-defined).
  • Document Unit vs. UnitName usage.
  • Add unit tests for type conversions and edge cases.
  • Monitor external APIs for potential Unit support.

Conclusion

The new approach—using Unit internally and UnitName externally—is the better choice for ObservationAnalyticsProvider due to its robustness, scalability, and alignment with best practices. The antithesis (using UnitName everywhere) is simpler but risks errors and technical debt, making it less suitable for a critical domain. The new approach balances internal type safety with external compatibility, making it a sound engineering decision.

Code Listings

Original ObservationAnalyticsProvider

public class ObservationAnalyticsProvider : IObservationAnalyticsProvider
{
    private readonly AppDbContext _context;
    private readonly ILogger<ObservationAnalyticsProvider> _logger;
    private readonly IUnitConverter _unitConverter;

    public ObservationAnalyticsProvider(AppDbContext context, ILogger<ObservationAnalyticsProvider> logger, IUnitConverter unitConverter)
    {
        _context = context;
        _logger = logger;
        _unitConverter = unitConverter;
    }

    public async Task<ObservationChangeReport> GetObservationChangeReportAsync(int subjectId, string observationDefinitionName, string? displayUnitName = null)
    {
        _logger.LogInformation($"Generating observation change report for SubjectId: {subjectId}, Observation: {observationDefinitionName}, DisplayUnit: {displayUnitName ?? "default"}");

        try
        {
            // Fetch the subject
            var subject = await _context.GetFilteredSubjects()
                .Where(s => s.Id == subjectId)
                .FirstOrDefaultAsync();

            if (subject == null)
            {
                _logger.LogWarning($"Subject {subjectId} not found.");
                throw new KeyNotFoundException("Subject not found or not authorized.");
            }

            // Fetch observation definition with units
            var observationDef = await _context.ObservationDefinitions
                .Where(od => od.DefinitionName == observationDefinitionName && od.IsActive == true)
                .Include(od => od.Units)
                .FirstOrDefaultAsync();

            if (observationDef == null)
            {
                _logger.LogError($"ObservationDefinition '{observationDefinitionName}' not found.");
                throw new InvalidOperationException($"ObservationDefinition '{observationDefinitionName}' not found.");
            }

            // Get supported units
            var supportedUnits = observationDef.Units.Select(u => u.UnitName).ToList();
            if (!supportedUnits.Any())
            {
                _logger.LogWarning($"No units defined for ObservationDefinition '{observationDefinitionName}'.");
                throw new InvalidOperationException($"No units defined for '{observationDefinitionName}'.");
            }

            // Determine standard unit
            string standardUnitName = GetStandardUnit(observationDefinitionName, supportedUnits);

            // Validate display unit
            string selectedDisplayUnit = displayUnitName != null && supportedUnits.Contains(displayUnitName)
                ? displayUnitName
                : standardUnitName;

            // Fetch records
            var records = await _context.GetFilteredSubjectRecords()
                .Where(sr => sr.SubjectId == subjectId && sr.ObservationDefinitionId == observationDef.Id && sr.MetricTypeId != null)
                .Include(sr => sr.MetricType)
                .ThenInclude(mt => mt.Unit)
                .OrderBy(sr => sr.RecordTime)
                .ToListAsync();

            if (!records.Any())
            {
                _logger.LogInformation($"No {observationDefinitionName} records found for Subject: {subject.Name}");
                return new ObservationChangeReport
                {
                    SubjectName = subject.Name,
                    ObservationType = observationDefinitionName,
                    StandardUnitName = standardUnitName,
                    DisplayUnitName = selectedDisplayUnit,
                    TrendDescription = $"No {observationDefinitionName} observations available."
                };
            }

            // Process records
            var observations = new List<Observation>();
            for (int i = 0; i < records.Count; i++)
            {
                double? percentChangePerWeek = null;
                if (i > 0)
                {
                    var prev = records[i - 1];
                    var curr = records[i];
                    var timeSpan = curr.RecordTime - prev.RecordTime;
                    var days = timeSpan.TotalDays;

                    if (days > 0 && prev.MetricValue != 0)
                    {
                        var prevValue = await ConvertValueAsync(prev.MetricValue ?? 0, prev.MetricType?.Unit?.UnitName, standardUnitName, observationDefinitionName);
                        var currValue = await ConvertValueAsync(curr.MetricValue ?? 0, curr.MetricType?.Unit?.UnitName, standardUnitName, observationDefinitionName);
                        var percentChange = ((currValue - prevValue) / prevValue);
                        percentChangePerWeek = (double)(percentChange * (7m / (decimal)days));
                    }
                }

                observations.Add(new Observation
                {
                    RecordTime = records[i].RecordTime,
                    Value = await ConvertValueAsync(records[i].MetricValue ?? 0, records[i].MetricType?.Unit?.UnitName, selectedDisplayUnit, observationDefinitionName),
                    PercentChangePerWeek = percentChangePerWeek
                });
            }

            // Calculate average rate
            double? averageRatePerDay = await CalculateAverageRateAsync(observations, standardUnitName, selectedDisplayUnit, observationDefinitionName);

            // Determine trend
            string trendDescription = DetermineTrend(observations, averageRatePerDay);

            return new ObservationChangeReport
            {
                SubjectName = subject.Name,
                ObservationType = observationDefinitionName,
                StandardUnitName = standardUnitName,
                DisplayUnitName = selectedDisplayUnit,
                Observations = observations,
                AverageRatePerDay = averageRatePerDay,
                TrendDescription = trendDescription
            };
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, $"Error generating observation report for SubjectId: {subjectId}, Observation: {observationDefinitionName}");
            throw;
        }
    }

    private string GetStandardUnit(string observationDefinitionName, List<string> supportedUnits)
    {
        return observationDefinitionName switch
        {
            "WeighIn" => supportedUnits.Contains("Kilograms") ? "Kilograms" : supportedUnits.First(),
            "TempCheck" => supportedUnits.Contains("DegreesCelsius") ? "DegreesCelsius" : supportedUnits.First(),
            "HeartRate" or "RespiratoryRate" => supportedUnits.Contains("BeatsPerMinute") ? "BeatsPerMinute" : supportedUnits.First(),
            _ => supportedUnits.First()
        };
    }

    private async Task<decimal> ConvertValueAsync(decimal value, string? sourceUnit, string targetUnit, string observationDefinitionName)
    {
        if (string.IsNullOrEmpty(sourceUnit) || sourceUnit == targetUnit)
            return value;

        try
        {
            double convertedValue = await _unitConverter.ConvertAsync(sourceUnit, targetUnit, (double)value);
            return (decimal)convertedValue;
        }
        catch (InvalidOperationException ex)
        {
            _logger.LogWarning($"No conversion rule for {sourceUnit} to {targetUnit} for {observationDefinitionName}. Returning original value. Error: {ex.Message}");
            return value;
        }
    }

    private async Task<double?> CalculateAverageRateAsync(List<Observation> observations, string standardUnitName, string displayUnitName, string observationDefinitionName)
    {
        if (observations.Count < 2)
            return null;

        double totalRate = 0;
        int intervals = 0;

        for (int i = 1; i < observations.Count; i++)
        {
            var prev = observations[i - 1];
            var curr = observations[i];
            var timeSpan = curr.RecordTime - prev.RecordTime;
            var days = timeSpan.TotalDays;

            if (days > 0)
            {
                var prevValue = await ConvertValueAsync(prev.Value, displayUnitName, standardUnitName, observationDefinitionName);
                var currValue = await ConvertValueAsync(curr.Value, displayUnitName, standardUnitName, observationDefinitionName);
                var valueChange = currValue - prevValue;
                var rate = (double)(valueChange / (decimal)days);
                totalRate += rate;
                intervals++;
            }
        }

        return intervals > 0 ? totalRate / intervals : null;
    }

    private string DetermineTrend(List<Observation> observations, double? averageRate)
    {
        if (observations.Count < 2)
            return "Insufficient data to determine trend.";

        if (averageRate == null)
            return "No valid intervals to determine trend.";

        if (averageRate > 0.01)
            return $"Increasing at an average rate of {averageRate:F3} units/day.";
        else if (averageRate < -0.01)
            return $"Decreasing at an average rate of {Math.Abs(averageRate.Value):F3} units/day.";
        else
            return "Stable.";
    }
}

Refactored ObservationAnalyticsProvider

public class ObservationAnalyticsProvider : IObservationAnalyticsProvider
{
    private readonly AppDbContext _context;
    private readonly ILogger<ObservationAnalyticsProvider> _logger;
    private readonly IUnitConverter _unitConverter;

    public ObservationAnalyticsProvider(AppDbContext context, ILogger<ObservationAnalyticsProvider> logger, IUnitConverter unitConverter)
    {
        _context = context;
        _logger = logger;
        _unitConverter = unitConverter;
    }

    public async Task<ObservationChangeReport> GetObservationChangeReportAsync(int subjectId, string observationDefinitionName, string? displayUnitName = null)
    {
        _logger.LogInformation($"Generating observation change report for SubjectId: {subjectId}, Observation: {observationDefinitionName}, DisplayUnit: {displayUnitName ?? "default"}");

        try
        {
            // Fetch the subject
            var subject = await _context.GetFilteredSubjects()
                .Where(s => s.Id == subjectId)
                .FirstOrDefaultAsync();

            if (subject == null)
            {
                _logger.LogWarning($"Subject {subjectId} not found.");
                throw new KeyNotFoundException("Subject not found or not authorized.");
            }

            // Fetch observation definition with units
            var observationDef = await _context.ObservationDefinitions
                .Where(od => od.DefinitionName == observationDefinitionName && od.IsActive == true)
                .Include(od => od.Units)
                .FirstOrDefaultAsync();

            if (observationDef == null)
            {
                _logger.LogError($"ObservationDefinition '{observationDefinitionName}' not found.");
                throw new InvalidOperationException($"ObservationDefinition '{observationDefinitionName}' not found.");
            }

            // Get supported units as Unit entities
            var supportedUnits = observationDef.Units.ToList();
            if (!supportedUnits.Any())
            {
                _logger.LogWarning($"No units defined for ObservationDefinition '{observationDefinitionName}'.");
                throw new InvalidOperationException($"No units defined for '{observationDefinitionName}'.");
            }

            // Determine standard unit
            Unit standardUnit = GetStandardUnit(observationDefinitionName, supportedUnits);

            // Validate display unit
            Unit selectedDisplayUnit = displayUnitName != null && supportedUnits.Any(u => u.UnitName == displayUnitName)
                ? supportedUnits.First(u => u.UnitName == displayUnitName)
                : standardUnit;

            // Fetch records
            var records = await _context.GetFilteredSubjectRecords()
                .Where(sr => sr.SubjectId == subjectId && sr.ObservationDefinitionId == observationDef.Id && sr.MetricTypeId != null)
                .Include(sr => sr.MetricType)
                .ThenInclude(mt => mt.Unit)
                .OrderBy(sr => sr.RecordTime)
                .ToListAsync();

            if (!records.Any())
            {
                _logger.LogInformation($"No {observationDefinitionName} records found for Subject: {subject.Name}");
                return new ObservationChangeReport
                {
                    SubjectName = subject.Name,
                    ObservationType = observationDefinitionName,
                    StandardUnitName = standardUnit.UnitName,
                    DisplayUnitName = selectedDisplayUnit.UnitName,
                    TrendDescription = $"No {observationDefinitionName} observations available."
                };
            }

            // Process records
            var observations = new List<Observation>();
            for (int i = 0; i < records.Count; i++)
            {
                double? percentChangePerWeek = null;
                if (i > 0)
                {
                    var prev = records[i - 1];
                    var curr = records[i];
                    var timeSpan = curr.RecordTime - prev.RecordTime;
                    var days = timeSpan.TotalDays;

                    if (days > 0 && prev.MetricValue != 0)
                    {
                        var prevValue = await ConvertValueAsync(prev.MetricValue ?? 0, prev.MetricType?.Unit, standardUnit, observationDefinitionName);
                        var currValue = await ConvertValueAsync(curr.MetricValue ?? 0, curr.MetricType?.Unit, standardUnit, observationDefinitionName);
                        var percentChange = ((currValue - prevValue) / prevValue);
                        percentChangePerWeek = (double)(percentChange * (7m / (decimal)days));
                    }
                }

                observations.Add(new Observation
                {
                    RecordTime = records[i].RecordTime,
                    Value = await ConvertValueAsync(records[i].MetricValue ?? 0, records[i].MetricType?.Unit, selectedDisplayUnit, observationDefinitionName),
                    PercentChangePerWeek = percentChangePerWeek
                });
            }

            // Calculate average rate
            double? averageRatePerDay = await CalculateAverageRateAsync(observations, standardUnit, selectedDisplayUnit, observationDefinitionName);

            // Determine trend
            string trendDescription = DetermineTrend(observations, averageRatePerDay);

            return new ObservationChangeReport
            {
                SubjectName = subject.Name,
                ObservationType = observationDefinitionName,
                StandardUnitName = standardUnit.UnitName,
                DisplayUnitName = selectedDisplayUnit.UnitName,
                Observations = observations,
                AverageRatePerDay = averageRatePerDay,
                TrendDescription = trendDescription
            };
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, $"Error generating observation report for SubjectId: {subjectId}, Observation: {observationDefinitionName}");
            throw;
        }
    }

    private Unit GetStandardUnit(string observationDefinitionName, List<Unit> supportedUnits)
    {
        return observationDefinitionName switch
        {
            "WeighIn" => supportedUnits.FirstOrDefault(u => u.UnitName == "Kilograms") ?? supportedUnits.First(),
            "TempCheck" => supportedUnits.FirstOrDefault(u => u.UnitName == "DegreesCelsius") ?? supportedUnits.First(),
            "HeartRate" or "RespiratoryRate" => supportedUnits.FirstOrDefault(u => u.UnitName == "BeatsPerMinute") ?? supportedUnits.First(),
            _ => supportedUnits.First()
        };
    }

    private async Task<decimal> ConvertValueAsync(decimal value, Unit? sourceUnit, Unit targetUnit, string observationDefinitionName)
    {
        if (sourceUnit == null || sourceUnit.UnitName == targetUnit.UnitName)
            return value;

        try
        {
            double convertedValue = await _unitConverter.ConvertAsync(sourceUnit.UnitName, targetUnit.UnitName, (double)value);
            return (decimal)convertedValue;
        }
        catch (InvalidOperationException ex)
        {
            _logger.LogWarning($"No conversion rule for {sourceUnit.UnitName} to {targetUnit.UnitName} for {observationDefinitionName}. Returning original value. Error: {ex.Message}");
            return value;
        }
    }

    private async Task<double?> CalculateAverageRateAsync(List<Observation> observations, Unit standardUnit, Unit displayUnit, string observationDefinitionName)
    {
        if (observations.Count < 2)
            return null;

        double totalRate = 0;
        int intervals = 0;

        for (int i = 1; i < observations.Count; i++)
        {
            var prev = observations[i - 1];
            var curr = observations[i];
            var timeSpan = curr.RecordTime - prev.RecordTime;
            var days = timeSpan.TotalDays;

            if (days > 0)
            {
                var prevValue = await ConvertValueAsync(prev.Value, displayUnit, standardUnit, observationDefinitionName);
                var currValue = await ConvertValueAsync(curr.Value, displayUnit, standardUnit, observationDefinitionName);
                var valueChange = currValue - prevValue;
                var rate = (double)(valueChange / (decimal)days);
                totalRate += rate;
                intervals++;
            }
        }

        return intervals > 0 ? totalRate / intervals : null;
    }

    private string DetermineTrend(List<Observation> observations, double? averageRate)
    {
        if (observations.Count < 2)
            return "Insufficient data to determine trend.";

        if (averageRate == null)
            return "No valid intervals to determine trend.";

        if (averageRate > 0.01)
            return $"Increasing at an average rate of {averageRate:F3} units/day.";
        else if (averageRate < -0.01)
            return $"Decreasing at an average rate of {Math.Abs(averageRate.Value):F3} units/day.";
        else
            return "Stable.";
    }
}