Click here to Skip to main content
Rate this: bad
good
Please Sign up or sign in to vote.
See more: ASP.NET MVC
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?
   [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:
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
Edited 25-Dec-12 9:08am
v3
Rate this: bad
good
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:
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
  Permalink  
Comments
Oleksandr Kulchytskyi at 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 at 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 at 25-Dec-12 16:07pm
   
OK, great then...:-)
—SA
Sergey Alexandrovich Kryukov at 25-Dec-12 16:09pm
   
Thank you, Oleksandr, дякую, :-)
—SA
Oleksandr Kulchytskyi at 26-Dec-12 3:42am
   
=) Thanks, native strings :-)
Rate this: bad
good
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:
 
[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 =)
  Permalink  
v2
Comments
FourCrate at 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 at 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 at 25-Dec-12 16:09pm
   
Right, a 5.
—SA
FourCrate at 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
0 CPallini 405
1 Sergey Alexandrovich Kryukov 272
2 OriginalGriff 165
3 George Jonsson 159
4 Richard MacCutchan 110
0 OriginalGriff 6,344
1 Sergey Alexandrovich Kryukov 5,890
2 CPallini 5,175
3 George Jonsson 3,559
4 Gihan Liyanage 2,522


Advertise | Privacy | Mobile
Web02 | 2.8.140916.1 | Last Updated 25 Dec 2012
Copyright © CodeProject, 1999-2014
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