New Rule S5770: View data dictionaries should be replaced by models

[kidchenko]

ToDo:

Implement S5770

Description

I would like to suggest a new rule. An analyzer to point out the usage of ViewBag and replace it with an model

Expected behavior

MyModel.cs

public string Message { get; set; }

MyController.cs

model.Message = "Hello World;"

### Actual behavior

MyController.cs
````csharp

ViewBag.Message = "Hello World"; // no alert shown

Why I think this is useful?

ViewBag is a dynamic property and you will only catch errors at run time making your code error prone.

[andrei-epure-sonarsource]

hi @kidchenko . thanks for the suggestion - I agree with this rule.

However, I am afraid it would be quite noisy for many, as many devs might just use ViewBag and the rule would not provide a lot of value. Or do you think that using ViewBag is rather a corner case?

From the official documentation:

You can use the collection of weakly typed data for passing small amounts of data in and out of controllers and views.

And indeed

ViewData and ViewBag are dynamically resolved at runtime. Since they don't offer compile-time type checking, both are generally more error-prone than using a viewmodel. For that reason, some developers prefer to minimally or never use ViewData and ViewBag.

@nicolas-harraudeau-sonarsource - WDYT?

[pavel-mikula-sonarsource]

ViewBag is easy to use + Using ViewBag is really bad idea = This rule can bring a lot of value.

While specifing, we should consider also ViewData and TempData as (configurable?) bad ideas.

cc @nicolas-harraudeau-sonarsource

[nicolas-harraudeau-sonarsource]

@andrei-epure-sonarsource @pavel-mikula-sonarsource ViewBag seems indeed to be used a lot (Sourcegraph, Github), including Microsoft projects.

If I understand correctly the main issue for this rule is that ViewBag is not type checked, thus it makes code maintenance more difficult. This would be a code smell rule. Is that it?

If Microsoft documentation mentions that "some developers prefer to minimally or never use ViewData and ViewBag" I guess a rule which is not in "Sonar way" Quality Profile would be possible, but it would have a low priority. My main questions are:

• Will everybody accept this rule and change their code? If yes then we can put this rule in "Sonar way" Quality Profile.
• If not, are there clear use cases where using a ViewBag is ok? If yes:
  • Can we make exceptions for them?
  • Would we be able to help code maintenance by detecting obvious mistakes? We could then create a Bug rule detecting when ViewBag is misused.
• Are there use cases where it is never ok to use a ViewBag? We could then raise issues in these cases.

If we can find a scope in which raising an issue is always ok we can make a great rule out of it.

[pavel-mikula-sonarsource]

@nicolas-harraudeau-sonarsource I would say Code smell+Major+SonarWay.

Main problem is that if you do typo like this ViewBag.Tilte = "Something"; you'll never find out. Usage in view @ViewBag.Title will emit empty string, no exception, no warning.

Using ViewBag is easy + simple => dangerous. It's acceptable for small/demo projects, but it's never OK 🙂 => SonarWay. You can always deactivate the rule, but the bigger project you have, the worse idea it is. There's no clear condition when it's good.

We can consider ignoring some well-known words and/or provide configuration for it (Title, Keywords, Description). I'd prefer to always raise, unless configured by user.

Obvious mistake detecting means go through all cshtml/vbhtml files, find all usages and report on never used. That would not be very precise, unless we build some view-tree for each controller. That would be another high-value bug detection rule, but we don't parse cshtml/vbhtml files yet.

[nicolas-harraudeau-sonarsource]

Thanks for this clear explanation @pavel-mikula-sonarsource. Let's go for the Code smell+Major+SonarWay rule as you suggested. Could you specify it? I can review it. Or I'll specify it in a few weeks and ping you if I have any question.

We analyze cshtml in SonarHTML but there is no priority to do it in SonarDotnet as far as I know. There wouldn't be much value in it anyway if using ViewBag is a bad idea. The simpler the rule the better.

[kidchenko]

sure agreed! +1

[nicolas-harraudeau-sonarsource]

I was saying that to @pavel-mikula-sonarsource but if you want to specify it @kidchenko be my guest ;) Is this what your last message meant?

Our guidelines are here: https://docs.sonarqube.org/latest/extend/adding-coding-rules/. In this case you would only be interested in the "Guidelines applicable to all rules" sub-section.

The end result will be an RSPEC ticket in our Jira. Example: RSPEC-5034 or RSPEC-5429.

[pavel-mikula-sonarsource]

@nicolas-harraudeau-sonarsource I'll write the RSPEC and you'll do the review.

[kidchenko]

@nicolas-harraudeau-sonarsource @pavel-mikula-sonarsource I saw the work is in progress already, but can I help you with something? Just created my account in JIRA (not sure if I will need)

[pavel-mikula-sonarsource]

Hello @kidchenko ,

Thank you one more time for this valuable suggestion. The rule is now specified as RSPEC-5770. There's no ETA to implement it, but specification is the first step on it's way to SonarWay profile.