Introduce constants for 'magic strings'

[BillWagner]

Throughout the code, we use a number of literal string constants. It would be great to replace those globally with defined string constants. See comments on PR #332 for the start of the conversation.

[bcbeatty]

I've used https://github.com/T4MVC/T4MVC in the past for MVC projects. It works great for getting rid of magic strings of controllers and views. Not sure if it's been updated to support tag helpers

[joelhulen]

@MisterJames and I had this conversation a couple of times. We discussed creating a static class named something like "EnumsAndStructs" or "WellKnownStrings", something along those lines. The idea is that some of these once-magic strings may be useful in other areas besides just views.

[dpaquette]

Don't forget about the new NameOf operator in C#6. This can sometimes be used to avoid magic strings for View names in Controller.

[Kritner]

Sorry for the spam... I didn't realize that referencing issues on my own fork would hit the issue on origin. Still learning the flow of git/hub :)

I figured it wouldn't hit the issue log until my PR.

[mheggeseth]

Don't worry about it. We don't have established guidelines for when to reference issues in commits. Actually, I think the information is helpful because it shows an issue is being worked on even if a PR is not imminent.

[ultrabert]

Hi all

I'm looking for a jump-in issue. Is this still alive?

(And if so, does anyone have a better method of finding all string literals than regular expression "Find In Files")

Cheers, Brett.

[tonysurma]

@ultrabert yes still alive, thx in advance

[dangle1]

I'd like to take a stab at this as well. To avoid duplication of effort, I'm letting everyone know I'm working on the ManageController and will be referencing the issue during commits, as per mheggesth's comment from 12/13/15.

[dangle1]

Some things to note,

Not sure how thorough we want to be in converting these magic strings, but Controller names can't take advantage of the nameof operator as a consequence of MVC's convention for expecting the Controller's name without the "Controller". Others have addressed this with extension methods: http://stackoverflow.com/questions/27444121/how-to-use-c-sharp-nameof-with-asp-net-mvc-url-action.

There's a lot of usage of ViewData throughout the controllers and views, any reason for not using viewmodels instead? If there is going to be extensive use of ViewData, JoelHulen's suggestion of implementing a static class with enums and consts would mitigate typos when keying into ViewData.

[ultrabert]

Are you still working on this issue, @dangle1?

[tonysurma]

@ultrabert I think he only did it for ManageController so you can jump in for another controller/area where the strings exist - just a note in this issue so others don't jump on the same set. Thanks!

[ultrabert]

Thanks, @tonysurma. I may tackle magic strings together with unit tests for an area, but I'm hesitant to claim one, as my opportunities to contribute are somewhat sporadic, and I wouldn't want to lock anyone out of something they wanted to do. :)

[nedruk]

Hi team, apologies ahead if I've done something wrong - my first commit/push on git. The update is tiny, but I want to make sure you are ok with the approach before I go ahead with it. Please have a look and let me know what you think. Thanks for the patience.

[stevejgordon]

@nedruk Thanks for your first contribution. Small, focused updates are good. It makes reviewing and merging easier for the project owners. I've taken a quick look and it seems you've followed the correct approach based on the requirements for this.

It might be worth making a formal PR with your code and then it can be reviewed there (might get a bit lost down the bottom of this issue). A couple of initial points from a quick look...

1. I'm not sure the specific Constant class is needed. Looking at the pattern from a prior PR it seems that the constants are to be added at the top of the controller in which they are used. In this case the AccountController.cs
2. You appear to have tweaked the config.json removing the Server=(localdb)\MSSQLLocalDB; in the connection string. Was that intended? I would imagine this could cause issues for other contributors and really shouldn't be changed in the committed code.

I hope that all makes sense. If you need any advise on submitting a PR let us know.

[nedruk]

Thanks for the detailed feedback, @stevejgordon.

Re Constant class - my idea was to propose this as an approach - store constants in a single place instead of having them scattered around the solution. But i see your point, better to follow the general pattern.

Re config change - that shouldn't have been committed. My bad.

Thanks for advice about PR. There is still quite a learning curve ahead of me here on git..

[stevejgordon]

@nedruk You're very welcome. I was new to it all about 7 months ago. There's a little learning curve but it starts to sync in pretty quickly.

I wrote up a blog post about my early experience at http://stevejgordon.co.uk/contributing-to-allready which might help.

@dpaquette also put up a really handy post on PR's http://www.davepaquette.com/archive/2016/01/24/Submitting-Your-First-Pull-request.aspx

[mgmccarthy]

can we please change the title of this Issue to "Introduce nameof() for 'magic strings'". We don't want to use constants, we want to use nameof().

[mgmccarthy]

@BillWagner @tonysurma @MisterJames

Can we update this issue so the formatting of the constants looks like this: private const int EventToDuplicateId = 1; instead of this? const int EVENT_TO_DUPLICATE_ID = 1;

I realize this should be in the docs section under some type of code style, but we don't really have anything there yet. I will be looking into contributing to that repository (as soon as I figure out how to work with Jekyll and markdown).

Also, I'd rather use nameof where we can, but as @stevejgordon pointed out, there are attributes which can override the name of action methods, so using that approach has it drawbacks as well.

[BillWagner]

I like this idea. As we work on the contributing guide, let's adopt this.

[JowenMei]

I will pick this up at the .NET Zuid codeathon!

[EmilyLuijbregts]

Hey @JowenMei :) Just running through the list of open issues and wanted to ask if you had a chance to look at this during the codeathon? Do you have enough information to move forward?

[EmilyLuijbregts]

Looks like this one is up for grabs if other volunteers would like to pick it up

[stevejgordon]

@EmilyLuijbregts - I will add this to our code-a-thon project and perhaps someone can pick it up tomorrow.

[JowenMei]

Hey sorry for my late response. I still have some changed files locally. I've been really struggling with Git(Hub). Perhaps I can send someone my files? ... I know it's not ideal, but then I'm not the bottleneck (and my changes are not lost)

[EmilyLuijbregts]

Hey Jowen. You can send it to @bmaluijb and he'll upload it tomorrow :-)

[johnmwright]

FYI: I'm going to address a few of these and submit PRs as I finish logical chunks.

[joshuahysong]

If there are still strings that need to be converted I'd love to use this as an opportunity to begin contributing.

[mgmccarthy]

@joshuahysong, you'd need to search through the codebase for them. I know @johnmwright knocked out a lot of these (especially when it came to claims and helper/extensions for claims operations).

If you want to float a couple ideas by us on this thread, to make sure we think they need to be changed before you start to contribute, that might be a good way to go here.

Thanks

[mgmccarthy]

@joshuahysong another way to contribute is to filter on the issues and pick the "jump-in" label...

[joshuahysong]

Thanks, I'll take a look at the jump-in labeled issues.