News site in ASP.NET Core

Question

I am new to ASP.NET Core 3.1 with no background of MVC. I am just moving from Web Forms to ASP.NET Core. I asked elsewhere and I got the following comments regarding my code. The code is working fine for me, but I am not sure how to improve it with the provided feedback.

I want to know how can I clean this code and write more optimized code based on the below code. I would appreciate examples with code.

it's actually a bit buggy. There's no need to explicitly open or close the connection (Dapper will open it), and using will dispose the connection even in cases where finally won't execute.

Another bug - catch(Exception ex){throw ex;} throws a new exception with a different stack trace. It's worse than no catch at all. To rethrow the original exception after eg logging, use throw;, without an exception object

Editor note: the quote says "buggy" or "bug", but the things cited are not actually bugs. They are suggestions to remove redundant code, or to change minutiae side-effects of exception semantics.

NewsController.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using BookListRazor.Model;
using Microsoft.AspNetCore.Mvc;
using Microsoft.EntityFrameworkCore;
using Dapper;
using Microsoft.Data.SqlClient;
using System.Data;
using BookListRazor.Data;

namespace BookListRazor.Controllers
{
    // [Route("api/News")]
     //[Route("api/News/[action]")]
    [ApiController]
    public class NewsController : Controller
    {
        //for EF
        private readonly ApplicationDbContext _db;

        //For  Dapper
        private readonly SqlConnectionConfiguration _configuration;


        public NewsController(ApplicationDbContext db, SqlConnectionConfiguration configuration)
        {
            _db = db;
            _configuration = configuration;
        }
        
        //get all news 
        [HttpGet]
        [Route("api/news/GetAll")]
        public async Task<IActionResult> GetAll()
        {

            //fetch data using EF
            // return Json(new { data = await _db.News.OrderByDescending(x => x.NewsDate).ToListAsync() });

            //Fetch data using Dapper
            IEnumerable<News> newslist;
            using (var conn = new SqlConnection(_configuration.Value))
            {
                string query = "select * FROM News";

                conn.Open();
                try
                {
                    newslist = await conn.QueryAsync<News>(query, commandType: CommandType.Text);
                }
                catch (Exception ex)
                {
                    throw ex;
                }
                finally
                {
                    conn.Close();
                }

            }
            return Json(new { data = newslist });
        } 
        
    }
}

and in the razor/cshtml:

<script>
    $(document).ready(function () {
    
     $.ajax({
                url: "api/news/getallnews/1",
                type: "GET",
                dataType: "json",
                success: function (response) {
                    var len = response.data.length;
                    var table = $("<table><tr><th>Details</th></tr>");
                    for (var i = 0; i < len; i++) {
                        //console.log("i "+i);
                        table.append("<tr><td>Title:</td><td>" + response.data[i].newsHeading + "</td></tr>");
                    }
                    table.append("</table>");
                    $("#news").html(table);
                }
            });

    });
</script>

and:

//get all news 
[HttpGet]
[Route("api/news/GetAllData")]
public async Task<IActionResult> GetAllData()
{

    using (SqlConnection connection = new SqlConnection(_configuration.Value))
    {
        var param = new DynamicParameters();
        //  param.Add("@prodtype", prodtype);
        //return connection.QueryFirst(" select * FROM News");
        string query = "select * FROM News";
        IEnumerable<News> newslist;
        newslist = await connection.QueryAsync<News>(query, commandType: CommandType.Text);
        return Json(new { data = newslist });
    }

}

Answer 1

In GetAll, the feedback you've already had seems pretty valid, to be honest; there is really no value in re-throwing an exception you've caught directly; if this is after some kind of processing, then throw; (without the exception instance) is a good way to re-throw the original exception without damaging it, but in your case the entire thing is redundant, so - just lose the try/catch/finally completely and let the using worry about the rest. Additionally, might as well keep the query "inline", and using AsList() helps make it clear that we're materializing the objects now rather than later (which could cause a problem with deferred execution). This is the same as the defaults, so the AsList() here doesn't change the behavior - just makes it clearer to the reader:

using (var conn = new SqlConnection(_configuration.Value))
{
    var newslist = (await conn.QueryAsync<News>("select * FROM News")).AsList();
    return Json(new { data = newslist });
}

---

In GetAllData you aren't using param, so trimming - it becomes... huh, the same!

using (SqlConnection connection = new SqlConnection(_configuration.Value))
{
    var newslist = (await connection.QueryAsync<News>("select * FROM News")).AsList();
    return Json(new { data = newslist });
}

---

Finally, in the jQuery callback, watch out for XSS - see this post on SO for a very similar example. The problem, to be explicit, is that response.data[i].newsHeading could be malicious - for example it could contain <script>.

Answer 2

Other minor points:

Imports

Sort your usings, and move them to the inside of your namespace. Also consider using StyleCop, which would suggest this.

Underscored privates

Why do these

    private readonly ApplicationDbContext _db;
    private readonly SqlConnectionConfiguration _configuration;

have underscores? Typically that's a Python convention. If you're worried about disambiguating them here:

    public NewsController(ApplicationDbContext db, SqlConnectionConfiguration configuration)
    {
        _db = db;
        _configuration = configuration;
    }
    

then you can simply prefix the destination with this..

Rethrow

This entire block should be deleted:

            catch (Exception ex)
            {
                throw ex;
            }

You will still be able to keep your finally. However, you shouldn't have your finally in there either, because you already have conn in a with. Your explicit close duplicates the close that IDisposable imposes on that connection.

Combined declaration

    IEnumerable<News> newslist;
    newslist = await connection.QueryAsync<News>(query, commandType: CommandType.Text);

does not need to be two separate statements; you can do the assignment on the first.