Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tvd to export #1753

Closed
wants to merge 10 commits into from
Closed

Add tvd to export #1753

wants to merge 10 commits into from

Conversation

MiraGeowerkstatt
Copy link
Contributor

@MiraGeowerkstatt MiraGeowerkstatt marked this pull request as draft December 12, 2024 08:42
Comment on lines +203 to +207
var boreholeGeometry = await Context.BoreholeGeometry
.AsNoTracking()
.Where(g => g.BoreholeId == b.Id)
.ToListAsync()
.ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PP: Einrückung korrigieren.

Comment on lines +209 to +211
b.TotalDepthTvd = BoreholeGeometryController.GetTVDIfGeometryExists(b.TotalDepth, boreholeGeometry);
b.TopBedrockFreshTvd = BoreholeGeometryController.GetTVDIfGeometryExists(b.TopBedrockFreshMd, boreholeGeometry);
b.TopBedrockWeatheredTvd = BoreholeGeometryController.GetTVDIfGeometryExists(b.TopBedrockWeatheredMd, boreholeGeometry);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q/REQ: Lässt sich die Methode in die Helper-Klasse, wo auch die Methode GetDepthTVD() liegt auslagern? So rufen sich die Controller nicht gegenseitig auf. GetTVDIfGeometryExists kann zum Beispiel als Extension von List<BoreholeGeometryElement> definiert werden.

Wenn man gerade dabei ist und ich den Namen Helper nicht gut finde: Im Helper hat es einige Extensions auf List<BoreholeGeometryElement> oder BoreholeGeometryElement, welche in eine eigene Klasse BoreholeGeometryElementExtensions ausgelagert werden können. Das macht das Ganze klarer und man packt nicht alles automatisch in diese Helper-Klasse 😁. Man muss dann schauen, was noch übrig bleibt und ob man diese Methoden ebenfalls weg bringt oder den Klassennamen treffender umformulieren kann.

Comment on lines +122 to +125
if (tvd != null) return Ok(tvd);

logger?.LogInformation($"Invalid input, could not calculate true vertical depth from measured depth of {depthMD}");
return Ok();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PP: Die if-Anweisung umkehren, das macht es einfacher lesbar/verständlich.

if (tvd == null)
{
    logger?.LogInformation($"Invalid input, could not calculate true vertical depth from measured depth of {depthMD}");
    return Ok();
}

return Ok(tvd);

/// <summary>
/// Represents a borehole from the csv export.
/// </summary>
public class BoreholeExport
Copy link
Contributor

@danjov danjov Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Kann hier etwas von Borehole geerbt werden? So spart man sich Doppelspurigkeit und muss bei Änderungen an den Borehole-Feldern nicht an diese Klasse denken. Siehe z.B. BoreholeImport-Klasse.

goToRouteAndAcceptTerms(`/${id}`);
});

//add geometry to borehole and verify export tvd changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PP:

Suggested change
//add geometry to borehole and verify export tvd changed
// add geometry to borehole and verify export tvd changed

Comment on lines +614 to +629
if (i < 3)
{
// Boreholes without geometry
Assert.AreEqual(totalDepthMd, totalDepthTvd);
Assert.AreEqual(topBedrockFreshMd, topBedrockFreshTvd);
Assert.AreEqual(topBedrockWeatheredMd, topBedrockWeatheredTvd);
}
else if (i < 5)
{
// Boreholes with geometry
Assert.AreNotEqual(totalDepthMd, totalDepthTvd);
Assert.AreNotEqual(topBedrockFreshMd, topBedrockFreshTvd);
Assert.AreNotEqual(topBedrockWeatheredMd, topBedrockWeatheredTvd);
}
else
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Kann man die Einträge anders identifizieren als über den Index? Das fühlt sich sehr hakelig an und man muss die Queries oben im Kopf haben, um das zu verstehen.

@MiraGeowerkstatt
Copy link
Contributor Author

@danjov habe den PR auf Draft zurückgestellt, da ich beim Umsetzen des Exports der Custom Borehole Ids gemerkt habe, dass ich dort noch einiges ändern muss. Deine Inputs setze ich dann gleich da um 🙏

@MiraGeowerkstatt MiraGeowerkstatt deleted the add-tvd-to-export branch December 17, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants