current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

As a test of my C# skills, I have been asked to create a simple web service which will take a message from a queue table in a SQL database and send it to a web application when a button is pressed on the application. I have never written a web service before so I am going in a little blind here so was after some advise on whether I had done this correct or not.

The stored procedure usp_dequeueTestProject gets a value from the top of the list in a table and then deletes the row in that table. At the moment I do not have this being archived anywhere, would it be better practice to instead of delete this, to just mark it as sent instead?

[WebService(Namespace = "http://tempuri.org/")]
[WebServiceBinding(ConformsTo = WsiProfiles.BasicProfile1_1)]
[System.ComponentModel.ToolboxItem(false)]
// To allow this Web Service to be called from script, using ASP.NET AJAX, uncomment the following line. 
// [System.Web.Script.Services.ScriptService]
public class Service1 : System.Web.Services.WebService
{
    [WebMethod]
    public string GetDataLINQ()
    {
        try
        {
            TestProjectLinqSQLDataContext dc = new TestProjectLinqSQLDataContext();
            var command = dc.usp_dequeueTestProject();
            string value = command.Select(c => c.Command).SingleOrDefault();
            return value;
        }
        catch (Exception ex)
        {
            return ex.ToString();
        }
    }
}

I've opted for using LINQ for starters, I am not sure if this is the best way to do it or not? I am only passing through the one string as well... In reality I guess you would normally want to send more than one field, such as datetime sent, message type etc, but wasn't sure what data type to use for this? I have seen this done using a Strut, but wasn't sure if this was correct? Any guidance would be greatly appreciated.

share|improve this question
1  
What is the nature of this "skills test"? Seems like asking the internet for a better solution won't be a good indicator of your skills. –  Ocelot20 May 21 at 18:48
    
Just to see if I'm able to do a web service really.. I want to impress though so don't want to muck it up by doing something glaringly obvious. –  user3284707 May 21 at 18:52
    
I mean who is administering the test? A potential employer, a teacher, yourself? If it's an employer who wants to know if you know how to properly write a web service, then getting help from the internet isn't really an indicator of your skillset. –  Ocelot20 May 21 at 18:56
    
It's a work project where a number of us have been asked to produce this simple web service. My boss knows I have very limited experience with web services so she will be expecting me to get help. I am more after some feedback on the code I have written and if it's the correct way to go about it, rather than someone giving me a solution which is better. Some quick suggestions on the questions I have asked would be greatly appreciated. –  user3284707 May 21 at 19:10
add comment

2 Answers

up vote 4 down vote accepted

  • A Linq-to-SQL DataContext is an IDisposable, your code isn't calling its Dispose() method. Wrap it in a using block to ensure proper disposal.

  • I would use var in the TestProjectLinqSQLDataContext instantiation as well, but that's just personal preference. I find Foo bar = new Foo() redundant.

  • Using SingleOrDefault assumes that the SP is returning only 1 record. That very well be the case now, but the SP being outside of the code, I wouldn't make that assumption. Use FirstOrDefault to grab only the first record - this way the day the SP is modified to return more than 1 row, your code won't break.

Something like this:

string result;

using (var dc = new TestProjectLinqSQLDataContext())
{
    var command = dc.usp_dequeueTestProject();
    result = command.Select(c => c.Command).FirstOrDefault();
}

return result;

I agree with @Jesse's comment about exceptions usage: the calling code has no way of easily telling a valid response from an error message (exception type, message and stack trace), since your code returns a string in every case.

Let it blow up!

share|improve this answer
    
Thank you for your comment. Should I wrap all my linq code in using so that it properly disposes of the code after use? Thank you for your comment about the FirstOrDefault as well I will change this in my code. Could you advise whether a strut would be best to do a web service which returns multiple fields? I am going to look at pulling through more fields as I imagine this is what we will be doing in a future project. –  user3284707 May 21 at 20:30
    
You mean a struct? I'd use a class instead, a POCO (Plain Old CLR Object) - a simple class that merely exposes auto-properties should do the trick. –  Mat's Mug May 21 at 20:33
add comment
up vote 5 down vote

LINQ is a good choice. You're querying something, so it lends itself to the task.

That being said, the exception handling is atrocious. Making your return values null, some command's value from the database or some big honkin' exception text is abusing the string data type. I'd remove the try..catch altogether and let your client app handle the exception if it knows what to do. In the best case, your catch could log the error somewhere then re-throw.

share|improve this answer
    
Thank you very much for your reply. I will take out the try catch as you say. Can you advise what you think I should use if I transfer more than just a string over the webservice? I've done it with a strut, but I wasn't sure if that was the most elegant way to do it? –  user3284707 May 21 at 19:58
    
What do you want to transfer? Use any data type you like and it will be serialized as you expect. –  Jesse C. Slicer May 21 at 20:29
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.