Click here to Skip to main content
12,950,618 members (64,189 online)
Rate this:
Please Sign up or sign in to vote.
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?
        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();
            da.Fill(ds, "auctions");
            return View(auction);

The Auction model class:
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
Updated 25-Dec-12 9:08am
Rate this: bad
Please Sign up or sign in to vote.

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:[^].

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,
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...:-)
Sergey Alexandrovich Kryukov 25-Dec-12 16:09pm
Thank you, Oleksandr, дякую, :-)
Oleksandr Kulchytskyi 26-Dec-12 3:42am
=) Thanks, native strings :-)
Rate this: bad
Please Sign up or sign in to vote.

Solution 2

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:

 public ActionResult Create(FormCollection collection, Auction auction)
  BL.DVTire bl=new BL.DVTire();
    using(var uow=Dal.UnityOfWork())
      using(var auctionRepo=Dal.RepoFactory.Create<Auction>(uow))

Of course this is not a bible, and you might choose another variant =)
But consider this example like starting point =)
FourCrate 25-Dec-12 15:59pm
Thanks for your answer, I have never heard of this Unity Of Work so I am looking into it now.

Is there a more simpler way of re-adjusting my code without using design patterns although ultimately that's the way to go.
Oleksandr Kulchytskyi 25-Dec-12 16:08pm
Yep of course there are a lot of ways to do it, and maybe for learning purpose it would be more prefarable for you as a starting point, but i have formed my idea from consideration of extensibility and maintability of code.
What if in future decide to extend your app, and add one more tab which will access to db, and furthermore, if you familar with IoC and DI , for example you can easily replace your SQL repo with another one, for example Mongo Repo...
Sergey Alexandrovich Kryukov 25-Dec-12 16:09pm
Right, a 5.
FourCrate 25-Dec-12 16:18pm
cheers guys

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

    Print Answers RSS
Top Experts
Last 24hrsThis month
OriginalGriff 5,829
CHill60 3,460
Maciej Los 2,953
Jochen Arndt 1,975
ppolymorphe 1,820

Advertise | Privacy | Mobile
Web02 | 2.8.170525.1 | Last Updated 25 Dec 2012
Copyright © CodeProject, 1999-2017
All Rights Reserved. Terms of Service
Layout: fixed | fluid

CodeProject, 503-250 Ferrand Drive Toronto Ontario, M3C 3G8 Canada +1 416-849-8900 x 100