Ancient ASP.NET structure, tell me how to do it right

Question

I'm pretty new to ASP.NET and I think im not using it they way it's meant to be used with all the features packed into the latest .NET framework. I'm currently using .NET framework 4,0. There are some error in the code, don't mind them mind they way I seem to be using ancient techniques.

I have structured everything like this.

I file called webservice.cs, that file is packed with webmethods like this:

[WebMethod]
public string laggtillprodukt(string pro1, int pro2)
{
    int sqlstatus;
    string sqlinsertstringfull = "INSERT INTO t_produkter (produkt_namn) VALUES ('" + pro1 + "');" +
                                 "SELECT produkt_id FROM t_produkter WHERE (produkt_id = SCOPE_IDENTITY()); " +
                                 "INSERT INTO t_produktegenskaper (produkt_id, egenskaps_id) " +
                                 "SELECT SCOPE_IDENTITY(), egen.egenskap_id " +
                                 "FROM t_kopplingmallegenskaper as egen " +
                                 "WHERE egen.mall_id = " + pro2 + ";";

   sqlstatus = executeWriteSqlQuery(sqlinsertstringfull);

   return "These values has been added to the db" + pro1 + " and " + pro2 + " SQL STATUS:" + sqlstatus;
}

In my code behind i do this to call the correct function(the one below has nothing to do with the webmethod before it was just to illustrate one of many SQL QUERIES.

 protected void laggtillnymallbutton_Click(object sender, EventArgs e)
{
    WebService globalwebservice = new WebService();

    if (string.IsNullOrWhiteSpace(laggtillnymall.Text))
    {
        Label1.Text = "String cannot be empty or just whitespaces!";
    }
    else
    {
        globalwebservice.laggtillmall(laggtillnymall.Text.Trim());
        Label1.Text = "Template added";
    }

Can't I be doing this in a more effiecient way. I have constructed a general method that all webmethods either use to insertdata or to read data, saved me some code, but I've seen things like LINQ. That has far less code than I have. Please aid me or point me in a towards a not so ancient way of coding ;)

Your data access method is vulnerable to [SQL Injection](http://en.wikipedia.org/wiki/SQL_injection) - you should be using a [parameterized query](http://www.codinghorror.com/blog/2005/04/give-me-parameterized-sql-or-give-me-death.html) instead. — Oded, Jul 21 '12 at 10:06
I am finding it very difficult to find an actual question in your post - except from a general "help make this better", which is not what Stack Overflow is about - if that is the case, [code review](http://codereview.stackexchange.com) may be a better home for this question. Please clarify. — Oded, Jul 21 '12 at 10:11
@Oded - I agree, if there is a problem besides the SQL Injection issue we would need to also see `executeWriteSqlQuery` and `laggtillmall` to answer it. — Hogan, Jul 21 '12 at 10:25

walther · Answer 1 · 2012-07-21T14:17:20.400

Well, let me see....

you fail to decouple business logic from DAL, that's a VERY bad approach. Data access should be in a completely separate layer, it doesn't belong to webservice.
your code is terribly vulnerable to SQL injection https://www.owasp.org/index.php/SQL_Injection . Instead, you should be using parameterized queries http://www.techrepublic.com/article/shorten-development-time-by-using-parameterized-queries-in-adonet/6093390 or stored procedures, depending on your needs.
your naming convention isn't great as well... In .NET we usually prefer to use camelCase, not "thisismysuppaduppamethod", and your variable names aren't really clear. Have in mind, that you're not writing the code for the machine, but rather for people to read. Code should be easily readable and anyone who looks at your code must immediately understand, what's its purpose. I highly doubt, that you'll see immediately what does string pro1, int pro2 actually mean, without studying the code deeper, if you ever need to revise your code in the future, let's say in 2-3 years later.
I'd recommend using English instead of your native language. If you'll ever need help with the code (or introduce a new co-worker to your team), it's usually considered a superior practice, because you won't have to explain "and what does this stand for??".

Thank you! Very constructive critisism! One question though, how should i go about to decouple business login from DAL. For my login I have created a stored procedure which in turn return an int which then determines if user can log in or not. Should I work with only stored procedures and call them from the codebehind? I thought datafetching was the job of a webservice? — 8bitcat, Jul 21 '12 at 18:51
@CarlPalsson, some people would say that you should use ONLY stored procedures, however I wouldn't agree with them. What I'm saying is, that webservice isn't a place for your DAL. You should have separate classes or even better a class library, that you can swap any time you'd need. Lower layers should never know of anything above them. You create some webservice, but you call it like regular methods in code-behind and I'm utterly puzzled, what problem have you solved by that approach. I don't see any logic in your DAL architecture at all, but if EF solves your problems, good for you! — walther, Jul 22 '12 at 08:45

score 1 · Accepted Answer · answered Jul 21 '12 at 13:51

1

What I think you're asking is a better way to access your database. You should look into Linq-To-SQL and Entity Framework for starters. These technologies provide a more modern and easier-to-work with approach in dealing with your data stores.

answered Jul 21 '12 at 13:51

Brad Rem

6,036
2
25
50

1

I don't agree, Parameterized Queries, Linq-To-SQL and Entity framework can all be abused and elegantly used. In fact, the newer items tend to be abused by new users because the pitfalls and problems have not been well documented yet. I've seen many horribly slow Entity framework solutions in the last few years. – Hogan Jul 21 '12 at 15:23
Hey brad! That was exactly what I was looking for! Thanks alot i upped your answer! And marked it's as an answer! – 8bitcat Jul 21 '12 at 21:00
@Hogan, so true! Shame many people still don't understand, that newer or easier doesn't necessarily mean better... – walther Jul 22 '12 at 08:35

Ancient ASP.NET structure, tell me how to do it right

2 Answers2