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

[Blazor] Logging - comments for "magic numbers" #32455

Merged
merged 2 commits into from Apr 29, 2024
Merged

Conversation

hakenr
Copy link
Contributor

@hakenr hakenr commented Apr 28, 2024

The code, although intended to demonstrate function calls using integers, should avoid containing magic numbers. I've added a comment typical for such cases, similar to what you can find here:
https://github.com/dotnet/aspnetcore/blob/333629e6c3d4ee301bb5985f0de66f447f6226ba/src/Components/test/testassets/ComponentsApp.Server/Pages/_Host.cshtml#L20-L24

(In production code, we might want to minify the script, but here in the demo code, including a comment seems appropriate.)

cc @guardrex


Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/fundamentals/logging.md ASP.NET Core Blazor logging

@guardrex
Copy link
Collaborator

guardrex commented Apr 29, 2024

Thanks for sending this in. I agree ... and we should keep an 👁️ out for any more gremlins 😈 like this.

It's probably a good idea to mention this in a short note after Example 2. How do you feel about something akin to the following? The following is just draft language, so feel free to adjust it to your liking ...

This is a markdown cut-n-paste, and it probably should go after Line 729 ...

> [!NOTE]
> Using an integer to specify the logging level in Example 2, often referred to as a *magic number* or *magic constant*, is considered a poor coding practice because the integer doesn't clearly identify the logging level when viewing the source code. We recommend using a named constant instead of an integer, as Example 1 demonstrates.

@guardrex guardrex self-assigned this Apr 29, 2024
@hakenr
Copy link
Contributor Author

hakenr commented Apr 29, 2024

I'm somewhat undecided about explicitly recommending this approach in this specific context. Typically, using a magic number in compiled code is considered poor practice. However, it's unclear whether such strong disapproval is warranted here, since we are dealing with bytes sent in an HTTP response, where minification might be beneficial.

Regarding Example1, it does not utilize a named constant; instead, it employs a string value because the configureLogging() method can accept either a string or an integer (ah, JavaScript 🙄).

If we're to offer guidance (I'd probably leave it without an explanation), consider this:

Note

Using an integer to specify the logging level in Example 2, often referred to as a magic number or magic constant, is considered a poor coding practice because the integer doesn't clearly identify the logging level when viewing the source code. However, if minimizing the bytes transferred to the browser is a priority, this approach might be justified (consider removing the comment in such cases).

@guardrex
Copy link
Collaborator

guardrex commented Apr 29, 2024

Regarding Example1, it does not utilize a named constant; instead, it employs a string value because the configureLogging() method can accept either a string or an integer (ah, JavaScript 🙄).

Fair enough. I didn't recall that detail.

Yes, I think your text would be great here with two small NITs ... just use a plain "if" because "however-if" isn't necessary ... and let's clarify "this approach" to avoid all possible confusion ...

> [!NOTE]
> Using an integer to specify the logging level in Example 2, often referred to as a *magic number* or *magic constant*, is considered a poor coding practice because the integer doesn't clearly identify the logging level when viewing the source code. If minimizing the bytes transferred to the browser is a priority, using an integer might be justified (consider removing the comment in such cases).

I recommend this appear right after Line 729 on the {BLAZOR SCRIPT} placeholder and before the last line with the cross-link to Blazor.start guidance.

@guardrex
Copy link
Collaborator

I dropped it in. I'll merge this now.

@guardrex guardrex enabled auto-merge (squash) April 29, 2024 15:05
@guardrex guardrex merged commit 0c03008 into dotnet:main Apr 29, 2024
3 checks passed
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.

None yet

2 participants