Is this a bad way to use the 'Controller' when using the MVC standard?

Question

0.00/5 (No votes)

See more:

I've got some 'business logic' in the controller method below. I am still trying to understand MVC and I think this is probably really wrong. Where would I put the instructions in the method below?

C#

[HttpPost]
     public ActionResult Create(FormCollection collection, Auction auction)
     {
         SqlCommand command = new SqlCommand("INSERT INTO auctions (title, description, start_date, finish_date) VALUES ('"+ auction.Title +"', '"+ auction.Description +"', GETDATE(), '" + auction.EndTime + "')", connection);
         SqlDataAdapter da = new SqlDataAdapter(command);
         DataSet ds = new DataSet();

         connection.Open();
         da.Fill(ds, "auctions");
         connection.Close();
         return View(auction);
     }

The Auction model class:

C#

public class Auction
  {
      public long Id { get; set; }
      public string Title { get; set; }
      public string Description { get; set; }
      public DateTime StartTime { get; set; }
      public DateTime EndTime { get; set; }

      public void StoreNewAuction()
      {
          //put the connect/store junk here instead and call function from controller instead?
      }
  }

I think I should add a method to the Auctions model called StoreNewAuction() that does all the connecting and storing into the database. Then I would call the method in the controller above.

Posted 25-Dec-12 9:06am

FourCrate

Updated 25-Dec-12 9:08am

v3

Add a Solution

2 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Sergey Alexandrovich Kryukov · Accepted Answer · 2012-12-25T09:30:00

Solution 1

Hold on. First thing which is absolutely unacceptable in this code is that you compose an SQL statement by concatenation of strings, using some user input via the UI.

First, repeated string concatenation is bad because a string is immutable (you would need to use StringBuilder or string.Format, but in this case you should not use any composed string at all. This is extremely dangerous, because it makes the application open to a well-known exploit called SQL injection:
http://en.wikipedia.org/wiki/SQL_injection[^].

This article also explain what should you use instead: parametrized statements.

For further detail, please see my past answer: hi name is not displaying in name?[^].

Stay safe,

—SA

Posted 25-Dec-12 9:30am

Sergey Alexandrovich Kryukov

Comments

Oleksandr Kulchytskyi 25-Dec-12 15:41pm

Yep, completely agree with you, today i have tired to explain people that insertion command represented via string is a bad style, and this can lead to SQL injections...
Nice answer! My five :)

FourCrate 25-Dec-12 15:52pm

Hi, I totally understand about the string thing. It's just while I'm learning the MVC ideas, I am just doing 'quick' concatenating while I try to understand other stuff. This won't be the real thing lol

Sergey Alexandrovich Kryukov 25-Dec-12 16:07pm

OK, great then...:-)
—SA

Sergey Alexandrovich Kryukov 25-Dec-12 16:09pm

Thank you, Oleksandr, дякую, :-)
—SA

Oleksandr Kulchytskyi 26-Dec-12 3:42am

=) Thanks, native strings :-)

Oleksandr Kulchytskyi · Accepted Answer · 2012-12-25T09:38:00

Yes you are completely right!
Your code smell =) This is a bad style to perform some BL inside your Controller.
As concerns advises.
I can recommend you to separate your idea to differ tires.
So as a result it might be 3 tiers (BL, DAL, and your WEB).

As a core of a DAL , it would be convenient to use EF and Repository pattern with conjunction of UnityOfWork.

So as a result it could be something like that:

C#

[HttpPost]
 public ActionResult Create(FormCollection collection, Auction auction)
 {
  BL.DVTire bl=new BL.DVTire();
  if(bl.Validate(auction))
  {
    using(var uow=Dal.UnityOfWork())
    {
      using(var auctionRepo=Dal.RepoFactory.Create<Auction>(uow))
      {
       auctionRepo.Insert(auction);
       uow.Commit();
      }
    }
  }
}
</auction>

Of course this is not a bible, and you might choose another variant =)
But consider this example like starting point =)