Please rate my web service

Solarion

Honorary Master
Joined
Nov 14, 2012
Messages
19,018
Hi guys this is literally my first attempt at this (well many epic failures leading up to this point). I've incorporated some of the other things I've been learning like Unity.

I've also used Basic Authentication. There are two tables in the DB (Students and Logins), Logins for the basic authentication. I've also put a Fiddler help document in the project root. The password and Username is Admin:Admin.

Any sergestions please gentlemen!

Project File: https://github.com/GitRep77/APIWebServiceDemo
 
Last edited:

Solarion

Honorary Master
Joined
Nov 14, 2012
Messages
19,018
Why are you using dotnet framework and not dotnet core?
I don't have time to learn .NET Core right now. It does have a learning curve and yes I will get there but considering I have absolutely no previous experience with API's I considered .NET Framework is a good place to start and once I have gotten the basics behind me I can go forward with .NET Core.
 

Solarion

Honorary Master
Joined
Nov 14, 2012
Messages
19,018
I've fixed it sorry guys :X3:. Was busy reorganizing the whole repo and made that one private by accident.

 

Kosmik

Honorary Master
Joined
Sep 21, 2007
Messages
20,702
Just a few general design comments :

API

1. Structure is nice and I see you are leveraging the response codes but for a post, you are creating your own response? Look into the ResponseCreatedAt option.
2. It's never a bad idea to have some xml documentation comments above each method ( parakeets, return types, method descriptions etc). Although your methods are basic CRUD, it's good practice to get into.
3. While you're student api is behind basic authentication, the user is not, so quite a security risk. You can also wrap the whole controller in basic vs each method as you have done, semantics but easy read.

Datalayer

1. In one you use IEnumerable instead of List, I would stick with just list.
2. You can modify your using so that it does the open itself, no need to explicitly state it every time
3. While storeprocs are great, unless you really need to, you can save yourself a lot of code by leveraging dapper ( can't recall if its base or the dapper.contrib.extensions ) but you can do object crud. That saves you have to explicitly define store procedures and parameters. I keep those for db heavy operations. Look into the dapper methods like connection.Insert, .Update . Delete. These simply take a instance of the object as a single parameter.
4. For Bulk operations, which you havent listed, look into leveraging Table Value Parameters in SQL and using dapper to pass lists of those for bulk. There I use procedures and you can also leverage Merge statements.

Model

1. We use the dapper annotation of Key to identify our primary keys to assist the persistence engine.
2. I don't see id fields in your models but your api has id values? I personally like to ensure that the primary key of any table is a simple numeric ID ( one can use GUIDs etc, lots of schools of thought on that ). It makes sense utilizing id fields across queries vs string based fields ( performance )
3. As a design philosophy, I tend to avoid "hard deletes". Although we call it delete in name, it actually runs a update against the table flagging who deleted and when. This keeps good audit and if your record is referenced elsewhere, it doesn't break data integrity by its removal ( yes there are other ways as well but this helps keep a clean audit trail too)

Just a few pointers but hope they help :p @Solarion
 
Last edited:

Solarion

Honorary Master
Joined
Nov 14, 2012
Messages
19,018
Thanks mercurial and thank-you for taking the time Kosmik. There is a lot there. I will go through all of it when I get home :thumbsup: :thumbsup:
 

Solarion

Honorary Master
Joined
Nov 14, 2012
Messages
19,018
2. I don't see id fields in your models but your api has id values? I personally like to ensure that the primary key of any table is a simple numeric ID ( one can use GUIDs etc, lots of schools of thought on that ). It makes sense utilizing id fields across queries vs string based fields ( performance )
Just on this point here, is it common practice to perform api crud against the primary key of the record?

Example: 1 would be the PK.

C#:
        // GET: api/Students/1
        [HttpGet]
        [BasicAuthentication]
        public IHttpActionResult GetStudent(int id)
        {
            var student = _repository.GetStudent(id);

            if (student == null)
            {
                return NotFound();
            }

            return Ok(student);
        }
 

Kosmik

Honorary Master
Joined
Sep 21, 2007
Messages
20,702
Just on this point here, is it common practice to perform api crud against the primary key of the record?

Example: 1 would be the PK.

C#:
        // GET: api/Students/1
        [HttpGet]
        [BasicAuthentication]
        public IHttpActionResult GetStudent(int id)
        {
            var student = _repository.GetStudent(id);

            if (student == null)
            {
                return NotFound();
            }

            return Ok(student);
        }
But there is no id field in your model?


public class Student
{
[Required]
[StringLength(50)]
[System.ComponentModel.DisplayName("Student No")]
public string StudentNo
{
get;
set;
}
[StringLength(50)]
[System.ComponentModel.DisplayName("First Name")]
public string StudentName
{
get;
set;
}
[Required]
[StringLength(50)]
[System.ComponentModel.DisplayName("Last Name")]
public string StudentSurname
{
get;
set;
}
[Required]
[StringLength(1)]
[System.ComponentModel.DisplayName("Gender")]
public string StudentSex
{
get;
set;
}
[StringLength(250)]
[System.ComponentModel.Browsable(false)]
[System.ComponentModel.DisplayName("Address")]
public string StudentAddress
{
get;
set;
}
[Required]
[System.ComponentModel.DisplayName("Date of Birth")]
public DateTime StudentDateOfBirth
{
get;
set;
}
[System.ComponentModel.Browsable(false)]
public byte[] StudentPicture
{
get;
set;
}
[System.ComponentModel.DisplayName("Age")]
public long Age
{
get
{
return Helpers.GetAge(StudentDateOfBirth);
}
}
[Required]
[StringLength(50)]
[System.ComponentModel.Browsable(false)]
[System.ComponentModel.DisplayName("Parent Name")]
public string ParentName
{
get;
set;
}
[Required]
[StringLength(50)]
[System.ComponentModel.Browsable(false)]
[System.ComponentModel.DisplayName("Contact No")]
public string ContactNo
{
get;
set;
}
[StringLength(50)]
[System.ComponentModel.Browsable(false)]
[System.ComponentModel.DisplayName("Email Address")]
public string EmailAddress
{
get;
set;
}
[StringLength(500)]
[System.ComponentModel.Browsable(false)]
[System.ComponentModel.DisplayName("Remarks")]
public string Remarks
{
get;
set;
}
public override string ToString()
{
return StudentName + " " + StudentSurname;
}
}
 

Kosmik

Honorary Master
Joined
Sep 21, 2007
Messages
20,702
@Solarion did you understand when I spoke about the insert/update methods against objects in dapper?

 

Solarion

Honorary Master
Joined
Nov 14, 2012
Messages
19,018
@Solarion did you understand when I spoke about the insert/update methods against objects in dapper?


I found this. I'll check yours out now. Gotta tell you after using storedprocs for many years, this way makes me feel like, I am wearing shoes without socks. I'll do it and sure I'll get used to it, but it feels off :X3:


C#:
public bool UpdateCustomer(Customer ourCustomer)
{
  int rowsAffected = this._db.Execute("UPDATE [Customer] SET [CustomerFirstName] = @CustomerFirstName ,[CustomerLastName] = @CustomerLastName, [IsActive] = @IsActive WHERE CustomerID = " + ourCustomer.CustomerID, ourCustomer);

  if (rowsAffected > 0)
  {
      return true;
  }
  return false;
}
 

konfab

Honorary Master
Joined
Jun 23, 2008
Messages
24,110
Not really anything to do with the web service, but one of the things I will judge you mercilessly about is the readme. If you want to look make your life difficult, put a readme in a project that:
1)Is Empty
2) Doesn't contain instructions on how to get the project up and running.


This is a habit everyone should develop. I have literally wasted months on projects where some f__kwit didn't bother writing a note on how to get their software up and running.

So if I were you, I would add the following to your readme:
a) What systems you run it on (OS and version)
b) What dependencies you need to get it running. (any additional libraries and links)
c) What software you used to get the project up and running (what IDE and version)
 

R4ziel

Expert Member
Joined
Apr 16, 2015
Messages
2,593
Not really anything to do with the web service, but one of the things I will judge you mercilessly about is the readme. If you want to look make your life difficult, put a readme in a project that:
1)Is Empty
2) Doesn't contain instructions on how to get the project up and running.
3) Keep it updated when you find things that other people should know about your project.

This is a habit everyone should develop. I have literally wasted months on projects where some f__kwit didn't bother writing a note on how to get their software up and running.

So if I were you, I would add the following to your readme:
a) What systems you run it on (OS and version)
b) What dependencies you need to get it running. (any additional libraries and links)
c) What software you used to get the project up and running (what IDE and version)
I agree with this, even though I am not a developer I like to check out projects and stuff from Git's too, and the readme on some of them is so horrible you can't make heads or tails of anything!
 

konfab

Honorary Master
Joined
Jun 23, 2008
Messages
24,110
I agree with this, even though I am not a developer I like to check out projects and stuff from Git's too, and the readme on some of them is so horrible you can't make heads or tails of anything!
It is a good life skill in general, that whenever you do something complex, you write down what you did.
 

Kosmik

Honorary Master
Joined
Sep 21, 2007
Messages
20,702
I found this. I'll check yours out now. Gotta tell you after using storedprocs for many years, this way makes me feel like, I am wearing shoes without socks. I'll do it and sure I'll get used to it, but it feels off :X3:


C#:
public bool UpdateCustomer(Customer ourCustomer)
{
  int rowsAffected = this._db.Execute("UPDATE [Customer] SET [CustomerFirstName] = @CustomerFirstName ,[CustomerLastName] = @CustomerLastName, [IsActive] = @IsActive WHERE CustomerID = " + ourCustomer.CustomerID, ourCustomer);

  if (rowsAffected > 0)
  {
      return true;
  }
  return false;
}

Not quite. This is how I would write your same code

Code:
public bool PutCustomer(Customer customer)
{
    using ( var connection = ConectionHelper.GetOpenConnection(databaseConnetionString))
    {
        return connection.Update<Customer>(customer);
    }
}
Connection is a straight SQLConnection, the helper just checks if there is a open one or creates one. The Update is a dapper.contrib.extension that updates the database by using reflection on the object to build its statement.


Makes for very neat repo classes.
 
Last edited:
Top