C# Rental Program

Question

I knew a little Java, and thought what the hell, I'll help my school and design this program that'll help checkout phone chargers and calculators and things to students. So I got in way over my head, but I got the program working. I wrote it in C#, and while the whole thing needs review, I really feel my checkout function could use immediate improvement.

This is the controller function for creating a checkout record, I have 6 different tables:

• tb_BannedUsersTable
• tb_CheckoutCheckin
• tb_Items
• tb_LabTech (employees)
• tb_Student

It starts by taking a Student ID and up-to 3 items from an HTML view to create a record. it creates a new record per item, because of how everything gets checked in. It's a long garbled mess, so any advice is greatly appreciated.

public ActionResult Create()
{
    return View();
}
[HttpPost]
public ActionResult Create(string StudentID, string upc, string upc1, string upc2)
{
    try
    {
        ViewModels view = new ViewModels();
        Student_Entities ExternalDB = new Students_Entities();
        tb_Student sfh = new tb_Student();

        if (StudentID.Length > 9)
        {
            //Shortens the scanned StudentID to 9 characters
            StudentID = StudentID.Truncate(9);

        }

        // Checks for Student record in the exsisting database

        var queryCount = db.tb_Student.Where(s => s.ID == StudentID).Count();

        if (queryCount < 1)
        {
            // checks external database of students
            try
            {
                var bsh = ExternalDB.v_View.FirstOrDefault(s => s.ID == StudentID);
                if (bsh == null)
                {
                    ViewBag.Message = "Student ID # not found in the system, please add the student from the Home menu";
                    return View("Index");
                }
                //creates student record if its found
                sfh.ID = bsh.ID;
                sfh.EMAIL_ADDRESS = bsh.EMAIL_ADDRESS;
                sfh.UserNAME = bsh.UserNAME;
                sfh.FIRST_NAME = bsh.FIRST_NAME;
                sfh.LAST_NAME = bsh.LAST_NAME;
                sfh.PHONE = bsh.LAST_NAME;
                sfh.SIGNEDWAIVER = null;
                db.tb_Student.Add(sfh);
                db.SaveChanges();
            }
            catch
            {
                ViewBag.Message = "Student ID # not found in the system, please add the student from the Home menu";
                return View("Index");
            }
        }

        var tbs = db.tb_Student.FirstOrDefault(s => s.ID == StudentID);
        bool? x = tbs.SIGNEDWAIVER;

        //Checks for signed Waiver
        //Stores SessionVariables.
        if (x != true)
        {
            SessionVariables.waiverStudentId = StudentID;
            SessionVariables.waiverUPC = upc;
            SessionVariables.waiverUPC1 = upc1;
            SessionVariables.waiverUPC2 = upc2;
            ViewBag.Message = "Records indicate this user has not signed a waiver";
            return View("Waiver", tbs);
        }

        if (upc1 == "") { upc1 = null; }
        if (upc2 == "") { upc2 = null; }

        //checks for UPC Entry
        if (upc == null)
        {
            ViewBag.Message = "Item Number cannot be blank with checkout";
            return View("Index");
        }
        if (upc == upc1 || upc == upc2)
        {
            ViewBag.Message = "Item Number was the same in both fields";
            return View("Index");
        }

        //checks for banned users
        if (!view.isBannedUser(StudentID))
        {
            //user is good, check for vaild Items

            //Item 1 Check
            var itemCheckout = db.tb_Items.Where(s => s.ItemUPC == upc).FirstOrDefault();
            var checkedOut = db.tb_CheckoutCheckin.Where(s => s.ItemUPCFK == upc && s.CheckinDate == null).FirstOrDefault();
            if (itemCheckout == null)
            {
                ViewBag.Message = "Item #1 " + upc + " number not found";
                return View("Index");
            }
            if (checkedOut != null)
            {
                ViewBag.Message = "Our Records indicate that item" + upc + " is checked-out, please check the item in first! " + StudentID;
                return View("Index");
            }

            //Item 2 Check
            if (upc1 != null)
            {
                var itemCheckout1 = db.tb_Items.Where(s => s.ItemUPC == upc1).FirstOrDefault();
                var checkedOut1 = db.tb_CheckoutCheckin.Where(s => s.ItemUPCFK == upc1 && s.CheckinDate == null).FirstOrDefault();
                if (itemCheckout1 == null)
                {
                    ViewBag.Message = "Item #2 " + upc1 + " not found";
                    return View("Index");
                }
                if (checkedOut1 != null)
                {
                    ViewBag.Message = "Our Records indicate that item #2 " + upc1 + " is checked-out, please check the item in first! " + StudentID;
                    return View("Index");
                }
            }
            //Item 3 Check
            if (upc2 != null)
            {
                var itemCheckout2 = db.tb_LabInventoryItems.Where(s => s.ItemUPC == upc2).FirstOrDefault();
                var checkedOut2 = db.tb_CheckoutCheckin.Where(s => s.ItemUPCFK == upc2 && s.CheckinDate == null).FirstOrDefault();
                if (itemCheckout2 == null)
                {
                    ViewBag.Message = "Item #3 " + upc2 + " not found";
                    return View("Index");
                }
                if (checkedOut2 != null)
                {
                    ViewBag.Message = "Our Records indicate that item #3 " + upc2 + " is checked-out, please check the item in first! " + StudentID;
                    return View("Index");
                }
            }


            var checkoutRecord = new tb_CheckoutCheckin();
            var checkoutRecord1 = new tb_CheckoutCheckin();
            var checkoutRecord2 = new tb_CheckoutCheckin();

            var stuRec = db.tb_Student.FirstOrDefault(s => s.ID == StudentID);
            var stuFirst = stuRec.FIRST_NAME + " " + stuRec.LAST_NAME;


            //One item Checkout
            if (upc != null && StudentID != null && upc1 == null && upc2 == null)

            {
                checkoutRecord.Student_IDFK = StudentID;
                checkoutRecord.ItemUPCFK = upc;
                checkoutRecord.ItemIDFK = itemCheckout.ItemId;
                checkoutRecord.CheckoutLabTech = SessionVariables.CurrentUserId;
                checkoutRecord.CheckoutDate = DateTime.Now;
                db.tb_CheckoutCheckin.Add(checkoutRecord);
                db.SaveChanges();
                ModelState.Clear();
                var model = db.tb_CheckoutCheckin.Where(s => s.CheckinDate == null);
                ViewBag.Message = stuFirst + " " + upc + " due back at " + SessionVariables.ItemDueDateTime;
                return View("Index", model);
            }
            //two item Checkout
            if (upc != null && StudentID != null && upc1 != null && upc2 == null)
            {
                checkoutRecord.Student_IDFK = StudentID;
                checkoutRecord.ItemUPCFK = upc;
                checkoutRecord.ItemIDFK = itemCheckout.ItemId;
                checkoutRecord.CheckoutLabTech = SessionVariables.CurrentUserId;
                checkoutRecord.CheckoutDate = DateTime.Now;

                checkoutRecord1.Student_IDFK = StudentID;
                checkoutRecord1.ItemUPCFK = upc1;
                checkoutRecord1.ItemIDFK = itemCheckout.ItemId;
                checkoutRecord1.CheckoutLabTech = SessionVariables.CurrentUserId;
                checkoutRecord1.CheckoutDate = DateTime.Now;


                db.tb_CheckoutCheckin.Add(checkoutRecord);
                db.tb_CheckoutCheckin.Add(checkoutRecord1);
                db.SaveChanges();
                ModelState.Clear();
                var model = db.tb_CheckoutCheckin.Where(s => s.CheckinDate == null);
                ViewBag.Message = stuFirst + " " + upc + " and " + upc1 + " due back at " + SessionVariables.ItemDueDateTime;
                return View("Index", model);
            }
            //three item Checkout
            if (upc != null && StudentID != null && upc1 != null && upc2 != null)
            {
                checkoutRecord.Student_IDFK = StudentID;
                checkoutRecord.ItemUPCFK = upc;
                checkoutRecord.ItemIDFK = itemCheckout.ItemId;
                checkoutRecord.CheckoutLabTech = SessionVariables.CurrentUserId;
                checkoutRecord.CheckoutDate = DateTime.Now;

                checkoutRecord1.Student_IDFK = StudentID;
                checkoutRecord1.ItemUPCFK = upc1;
                checkoutRecord1.ItemIDFK = itemCheckout.ItemId;
                checkoutRecord1.CheckoutLabTech = SessionVariables.CurrentUserId;
                checkoutRecord1.CheckoutDate = DateTime.Now;

                checkoutRecord2.Student_IDFK = StudentID;
                checkoutRecord2.ItemUPCFK = upc2;
                checkoutRecord2.ItemIDFK = itemCheckout.ItemId;
                checkoutRecord2.CheckoutLabTech = SessionVariables.CurrentUserId;
                checkoutRecord2.CheckoutDate = DateTime.Now;


                db.tb_CheckoutCheckin.Add(checkoutRecord);
                db.tb_CheckoutCheckin.Add(checkoutRecord1);
                db.tb_CheckoutCheckin.Add(checkoutRecord2);
                db.SaveChanges();
                ModelState.Clear();
                var model = db.tb_CSUCheckoutCheckin.Where(s => s.CheckinDate == null);
                ViewBag.Message = stuFirst + " " + upc + " and " + upc1 + " and " + upc2 + " due back at " + SessionVariables.ItemDueDateTime;
                return View("Index", model);
            }

        }
        ViewBag.Message = "That user has been banned from checkout! Or you got this page in error, try again. " + StudentID;
        return View("Index");

    }
    catch
    {
        ViewBag.Message = "An error occured during Checkout, please try again, Student Id." + StudentID;
        return View("Index");
    }
}

// GET: tb_Checkout/Edit/5
public ActionResult Edit(int? id)
{
    if (id == null)
    {
        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
    }
    tb_CSUCheckoutCheckin tb_CSUCheckoutCheckin = db.tb_CSUCheckoutCheckin.Find(id);
    if (tb_CSUCheckoutCheckin == null)
    {
        return HttpNotFound();
    }
    return View(tb_CSUCheckoutCheckin);
}

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Edit([Bind(Include = "CheckoutCheckinId,CSU_IDFK,ItemUPCFK,CheckoutLabTech,CheckoutDate,CheckinLabTech,CheckinDate,CheckoutLocationFK,CheckinLocationFK")] tb_CSUCheckoutCheckin tb_CSUCheckoutCheckin)
{
    if (ModelState.IsValid)
    {
        db.Entry(tb_CSUCheckoutCheckin).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index");
    }
    return View(tb_CSUCheckoutCheckin);
}

//alter record for waiver signed
public ActionResult WaiverSign(string id)
{
    var v = db.tb_Student.FirstOrDefault(s => s.CSU_ID == id);
    v.SIGNEDWAIVER = true;
    db.Entry(v).State = EntityState.Modified;
    ViewBag.Message = "Waiver Recorded, and Checkout Complete!";
    return Create(SessionVariables.waiverStudnetID, SessionVariables.waiverUPC, SessionVariables.waiverUPC1, SessionVariables.waiverUPC2);
}

I figure there has got to be a more efficient way to handle all this, and I'm not really sure how to create a 'data access layer' and I'm probably breaking some security rules, so the more help the better. My Checkin function isn't any better.

Answer 1

Disclaimer: if this answer feels like it gets less patient and more "to-the-point" as it goes on, I'm really tired as I am / was writing this and I can't English / logic right now. I promise I mean the very best with this, and I'm having trouble even typing at the moment. This is actually fairly solid code for a C# beginner.

---

Oh boy, there's a lot to talk about here.

Good news is we have plenty of time.

First: you variable naming is not the greatest. I.e. tbs is not self-explanatory, student is. I'm not going to comment on the names of the properties (or fields if you're into that) of the types, since you didn't post the definitions. But if you add a new question with the type definitions I promise I'll be just as brutal (but in a good way) on them as I was on this code. :)

---

    ViewModels view = new ViewModels();
    Student_Entities ExternalDB = new Students_Entities();
    tb_Student sfh = new tb_Student();

    if (StudentID.Length > 9)
    {
        //Shortens the scanned StudentID to 9 characters
        StudentID = StudentID.Truncate(9);

    }

    // Checks for Student record in the exsisting database

    var queryCount = db.tb_Student.Where(s => s.ID == StudentID).Count();

    if (queryCount < 1)
    {
        // checks external database of students
        try
        {
            var bsh = ExternalDB.v_View.FirstOrDefault(s => s.ID == StudentID);
            if (bsh == null)
            {
                ViewBag.Message = "Student ID # not found in the system, please add the student from the Home menu";
                return View("Index");
            }
            //creates student record if its found
            sfh.ID = bsh.ID;
            sfh.EMAIL_ADDRESS = bsh.EMAIL_ADDRESS;
            sfh.UserNAME = bsh.UserNAME;
            sfh.FIRST_NAME = bsh.FIRST_NAME;
            sfh.LAST_NAME = bsh.LAST_NAME;
            sfh.PHONE = bsh.LAST_NAME;
            sfh.SIGNEDWAIVER = null;
            db.tb_Student.Add(sfh);
            db.SaveChanges();
        }
        catch
        {
            ViewBag.Message = "Student ID # not found in the system, please add the student from the Home menu";
            return View("Index");
        }
    }

    var tbs = db.tb_Student.FirstOrDefault(s => s.ID == StudentID);

We're going to start with this block as it made me the least happy when I saw it.

There are four glaring issues here:

1. You search db.tb_Student twice, guaranteed.
2. You use .Count() to check if it exists.
3. You wrap a significant amount of work in a try/catch block, that doesn't need to be.
4. You treat sfh really weirdly.

Obviously we want to cut down on the work we do, always. I'm going to assume that tb_Student is a class. In this case, we can immediately reduce a lot of the work we do right off the bat.

var tbs = db.tb_Student.FirstOrDefault(s => s.ID == StudentID);

We're going to put this before our if statement, since C# and EF behave in a very predictable way. If there is no tb_Student record matching the ID, tbs will be null. We're going to modify our if to account for that.

if (tbs == null)
{
    // Do stuff
}

I don't see you using sfh anywhere other than that if, so move that declaration inside the if block.

    if (tbs == null)
    {
        var bsh = ExternalDB.v_View.FirstOrDefault(s => s.ID == StudentID);
        if (bsh == null)
        {
            ViewBag.Message = "Student ID # not found in the system, please add the student from the Home menu";
            return View("Index");
        }
        //creates student record if its found
        var sfh = new tb_Student();
        sfh.ID = bsh.ID;
        sfh.EMAIL_ADDRESS = bsh.EMAIL_ADDRESS;
        sfh.UserNAME = bsh.UserNAME;
        sfh.FIRST_NAME = bsh.FIRST_NAME;
        sfh.LAST_NAME = bsh.LAST_NAME;
        sfh.PHONE = bsh.LAST_NAME;
        sfh.SIGNEDWAIVER = null;
        db.tb_Student.Add(sfh);
        db.SaveChanges();
    }

Next you can pull a lot of that work outside the try block, and throw a better error. I would, at the very least, change it so that the error message reads "Could not migrate Student ID" et. al.

Then, at the end of that block (after db.SaveChanges()), if you really want, you could add another tbs = ..., or just tbs = sfh; since they're the same type and such.

After we adjust this section, we should extract a method for it.

private tb_Student GetOrMigrateStudent(int studentId)
{
    Student_Entities ExternalDB = new Students_Entities();
    var tbs = db.tb_Student.FirstOrDefault(s => s.ID == StudentID);

    if (tbs == null)
    {
        var bsh = ExternalDB.v_View.FirstOrDefault(s => s.ID == StudentID);

        if (bsh == null)
        {
            throw new ArgumentException("studentId", "Student ID # not found in the system, please add the student from the Home menu");\
        }

        //creates student record if its found
        var sfh = new tb_Student();
        sfh.ID = bsh.ID;
        sfh.EMAIL_ADDRESS = bsh.EMAIL_ADDRESS;
        sfh.UserNAME = bsh.UserNAME;
        sfh.FIRST_NAME = bsh.FIRST_NAME;
        sfh.LAST_NAME = bsh.LAST_NAME;
        sfh.PHONE = bsh.LAST_NAME;
        sfh.SIGNEDWAIVER = null;
        db.tb_Student.Add(sfh);
        db.SaveChanges();

        tbs = sfh;
    }

    return tbs;
}

This may need a little cleanup/adapting on your end.

---

Ok, so now our code looks like:

[HttpPost]
public ActionResult Create(string StudentID, string upc, string upc1, string upc2)
{
    try
    {
        ViewModels view = new ViewModels();

        if (StudentID.Length > 9)
        {
            //Shortens the scanned StudentID to 9 characters
            StudentID = StudentID.Truncate(9);
        }

        var tbs = GetOrMigrateStudent(StudentId);

        ...
    }
}

The next block to examine is:

bool? x = tbs.SIGNEDWAIVER;

//Checks for signed Waiver
//Stores SessionVariables.
if (x != true)
{
    SessionVariables.waiverStudentId = StudentID;
    SessionVariables.waiverUPC = upc;
    SessionVariables.waiverUPC1 = upc1;
    SessionVariables.waiverUPC2 = upc2;
    ViewBag.Message = "Records indicate this user has not signed a waiver";
    return View("Waiver", tbs);
}

This can be cleaned an extracted pretty quick.

public bool MustSignWaiver(tb_Student student)
{
    return student.SIGNEDWAIVER != true;
}

We do this so that we can simply call MustSignWaiver(tbs) instead of declaring our x and whatnot. We end up with:

if (MustSignWaiver(tbs))
{
    SessionVariables.waiverStudentId = StudentID;
    SessionVariables.waiverUPC = upc;
    SessionVariables.waiverUPC1 = upc1;
    SessionVariables.waiverUPC2 = upc2;
    ViewBag.Message = "Records indicate this user has not signed a waiver";
    return View("Waiver", tbs);
}

This makes things much more verbose. We no longer need the comments (the code explains itself).

---

Next we'll look at:

    if (upc1 == "") { upc1 = null; }
    if (upc2 == "") { upc2 = null; }

    //checks for UPC Entry
    if (upc == null)
    {
        ViewBag.Message = "Item Number cannot be blank with checkout";
        return View("Index");
    }
    if (upc == upc1 || upc == upc2)
    {
        ViewBag.Message = "Item Number was the same in both fields";
        return View("Index");
    }

This actually presents an interesting bug: what happens when I fill upc1 but not upc? Well we can fix that pretty easily.

var upcs = new List<string>();
upcs.Add(upc);
upcs.Add(upc1);
upcs.Add(upc2);
upcs = upcs.Where(x => !string.IsNullOrWhiteSpace(x)).ToList();

And now we forget about upc, upc1 and upc2 for the rest of the code. We'll do it with LINQ.

if (upcs.Length == 0)
{
    // No valid UPCs
}

if (upcs.Distinct() != upcs.Length)
{
    // Duplicate found
}

This got longer, but it got more usable later (you'll see how). Next we'll extract it to a method.

public List<string> GetValidUpcs(params string[] upcs)
{
    var result = new List<string>();

    result.AddRange(upcs);
    result = result.Where(x => !string.IsNullOrWhiteSpace(x)).ToList();

    return result;
}

And in our method we just do var upcs = GetValidUpcs(upc, upc1, upc2);. All the heavy lifting is now extracted, and if we add a new UPC it's trivial to add it. (Add a parameter to our ActionResult method, and pass the value to GetValidUpcs.)

---

We're going to look at just a couple lines next:

if (!view.isBannedUser(StudentID))
{
    ...
}
ViewBag.Message = "That user has been banned from checkout! Or you got this page in error, try again. " + StudentID;
return View("Index");

You know what this looks like it needs? Just a quick reorder.

if (view.isBannedUser(StudentId))
{
    ViewBag.Message = "That user has been banned from checkout! Or you got this page in error, try again. " + StudentID;
    return View("Index");
}
...

We want to reduce nested stuff (a.k.a. arrow code).

---

Next we have:

var itemCheckout = db.tb_Items.Where(s => s.ItemUPC == upc).FirstOrDefault();

You put your predicate in FirstOrDefault above, so you know it accepts one here. No need for Where.

var itemCheckout = db.tb_Items.FirstOrDefault(s => s.ItemUPC == upc);

Of course, we can checkout all the items from upcs now with some LINQ.

foreach (var upc in upcs)
{
    var itemCheckout = db.tb_Items.FirstOrDefault(s => s.ItemUPC == upc);
    var checkedOut = db.tb_CheckoutCheckin.FirstOrDefault(s => s.ItemUPCFK == upc && s.CheckinDate == null);

    // Conditions/view update here.
}

---

Ok, so we've taken a lot of this down and cleaned it, just a little bit further to go.

First, StudentID should not be null here. So remove that check altogether.

var stuRec = db.tb_Student.FirstOrDefault(s => s.ID == StudentID);

Second, what are you using a new DB query for? Does tbs not have our student already?

Next, you can write one abstraction now that we have upcs in a list instead of having to write an if block for each combination.

var stuFirst = tbs.FIRST_NAME + " " + tbs.LAST_NAME;
var checkoutDateTime = DateTime.Now;

foreach (var upc in upcs)
{
    var checkoutRecord = new tb_CheckoutCheckin();
    var itemCheckout = db.tb_Items.Where(s => s.ItemUPC == upc).FirstOrDefault();
    checkoutRecord.Student_IDFK = StudentID;
    checkoutRecord.ItemUPCFK = upc;
    checkoutRecord.ItemIDFK = itemCheckout.ItemId;
    checkoutRecord.CheckoutLabTech = SessionVariables.CurrentUserId;
    checkoutRecord.CheckoutDate = checkoutDateTime;
    db.tb_CheckoutCheckin.Add(checkoutRecord);
}

db.SaveChanges();
ModelState.Clear();
var model = db.tb_CSUCheckoutCheckin.Where(s => s.CheckinDate == null);

Lastly, we'll string.Join on our upcs since we want to be dynamice with it.

ViewBag.Message = stuFirst + " " + string.Join(" and ", upcs) + " due back at " + SessionVariables.ItemDueDateTime;
return View("Index", model);

---

And we're done with this method! :) I'm not going to comment on the others, they're not nearly as big of a mess, and you should be able to take a lot of what we learned from here to them. I wrote this entire thing outside an IDE and I'm a little tired at the moment, I'll try to clean this up more when I wake up.

Overall, not bad for coming from java. Hopefully you learned something from all of this. I mean it all in the best of spirits. :)