8
\$\begingroup\$

I have to show a select control of students with event information where Monday, Tuesday, Wednesday, Thursday and Friday are Events. (Independent buttons). The second columns are Events too, but you can add these Events using the "Add" button.

enter image description here

Data flows in like this:

I have to maintain the state the state of every student, and I thought that I would create a simple structure like this:

Note: I have implemented a little ShopCart just to make the code works.

DTOs

public class AfterSchoolProgramInformation
{
    public int TotalShopCartItems { get; set; }
    public IEnumerable<StudentInfo> StudentInfo { get; set; }
    public IEnumerable<ShopCartInfo> ShopCartInfo { get; set; }
}
public class StudentInfo
{
    public int StudentId { get; set; }
    public String Name { get; set; }
    public IEnumerable<DayInfo> DayInfo { get; set; }
}
public class DayInfo
{
    public string DayName { get; set; }
    public decimal Fee { get; set; }
    public bool Added { get; set; }       
    public int EventId { get; set; }
    public IEnumerable<DayInfoDetail> DayInfoDetail { get; set; }
}
public class DayInfoDetail
{
    public string ClassName { get; set; }
    public decimal Fee { get; set; }
    public bool Added { get; set; }
    public int EventId { get; set; }
}
public class ShopCartInfo
{
    public int StudentId { get; set; }
    public int[] EventIds { get; set; }
}

DataClass

public class AfterSchoolService
{
    public IEnumerable<DayInfo> GetAfterSchoolInformation()
    {
        DataTable result = new FakeDatabase().GetAfterSchoolInformation();

        var dayInformation = (from data in result.AsEnumerable()
                              group data by new
                              {
                                  DayWeek = data.Field<string>("DayOfWeek"),
                                  DayEventId = data.Field<int>("DayEventID"),
                                  DayFee = data.Field<decimal>("DayFee")
                              }
                                  into provisional
                                  let info = provisional.ToList()
                                  select new DayInfo
                                  {
                                      DayName = provisional.Key.DayWeek,
                                      Added = false,
                                      EventId = provisional.Key.DayEventId,
                                      Fee = provisional.Key.DayFee,
                                      DayInfoDetail = info.Select(s => new DayInfoDetail()
                                      {
                                          Added = false,
                                          ClassName = s.Field<string>("AddOnName"),
                                          EventId = s.Field<int>("AddOnEventID"),
                                          Fee = s.Field<decimal>("AddOnFee")
                                      }).ToList()
                                  })
                           .OrderBy(s => s.DayName == "Fr")
                           .ThenBy(s => s.DayName == "Th")
                           .ThenBy(s => s.DayName == "We")
                           .ThenBy(s => s.DayName == "Tu")
                           .ThenBy(s => s.DayName == "Mo")
                           .Select(s =>
                           {

                               s.DayName = Utils.GetDayNameBySuffix(s.DayName);
                               return s;
                           }).ToList();

        return dayInformation;
    }

    public IEnumerable<StudentInfo> GetStudents()
    {          
        return new List<StudentInfo>()
        {
            new StudentInfo(){
                StudentId = 1,
                Name= "Lee Burridge"
            },
             new StudentInfo(){
                StudentId = 2,
                Name= "Marco Carola"
            }
        };
    }
}

Presentation class

namespace WebApp
{
public partial class Home : System.Web.UI.Page
{        
    private static ShopCart _ShopCart;
    protected void Page_Load(object sender, EventArgs e)
    {
        _ShopCart = ShopCart.Current;
    }

    [WebMethod]
    [ScriptMethod(UseHttpGet = true, ResponseFormat = ResponseFormat.Json)]
    public static AfterSchoolProgramInformation GetAfterSchoolInformation()
    {
        return new Home().GetAfterSchoolProgramInformation();
    }
    [WebMethod]
    [ScriptMethod(UseHttpGet = true, ResponseFormat = ResponseFormat.Json)]
    public static string AddToCart(int eventid, decimal fee, int studentid)
    {
        Home instance = new Home();
        instance.AddEventToCart(eventid, fee, studentid);
        return instance.GetTotalShopCartItems().ToString();
    }
    [WebMethod]
    [ScriptMethod(UseHttpGet = true, ResponseFormat = ResponseFormat.Json)]
    public static string RemoveFromCart(int eventId, int studentid)
    {
        Home instance = new Home();
        instance.RemoveEventFromCart(eventId, studentid);
        return instance.GetTotalShopCartItems().ToString();
    }
    private IEnumerable<StudentInfo> GetStudentInfo()
    {
        return new AfterSchoolService().GetStudents().Select(s =>
        {
            s.DayInfo = IsShopCartEmpty() ? GetDayInfo() : GetDayInfoWithShopCartInfo(s.StudentId);
            return s;
        });
    }

    private bool IsShopCartEmpty()
    {
        return GetTotalShopCartItems() == 0;
    }
    private IEnumerable<DayInfo> GetDayInfo()
    {
        return new AfterSchoolService().GetAfterSchoolInformation();

    }
    private Dictionary<int, int[]> GetEventAndStudentsInShopCart()
    {
        var shopCartItems = _ShopCart.ShopCartItems;
        var lookupItems = shopCartItems.AsEnumerable().ToLookup(s => s.StudentId, s => s.EventId);
        Dictionary<int, int[]> studensWithEvents = lookupItems.ToDictionary(s => s.Key, s => s.ToArray());
        return studensWithEvents;
    }
    private IEnumerable<DayInfo> GetDayInfoWithShopCartInfo(int studentId)
    {          
        var currentShopCartItems = GetEventAndStudentsInShopCart();
        var dayInfo = new AfterSchoolService().GetAfterSchoolInformation().Select(s =>
        {
            s.Added = ContainsKeyValue(currentShopCartItems, studentId, s.EventId);
            s.DayInfoDetail = s.DayInfoDetail.Select(t =>
            {
                t.Added = ContainsKeyValue(currentShopCartItems, studentId, t.EventId);
                return t;
            });
            return s;
        });
        return dayInfo;

    }
    public bool ContainsKeyValue(Dictionary<int, int[]> dictionary,
               int expectedKey, int expectedValue)
    {
        int[] actualValue;
        if (!dictionary.TryGetValue(expectedKey, out actualValue))
        {
            return false;
        }
        return actualValue.Contains(expectedValue);
    }
    private AfterSchoolProgramInformation GetAfterSchoolProgramInformation()
    {
        return new AfterSchoolProgramInformation()
        {
            StudentInfo = GetStudentInfo(),
            TotalShopCartItems = GetTotalShopCartItems(),
            ShopCartInfo = GetShopCartInfo()
        };
    }
    private IEnumerable<ShopCartInfo> GetShopCartInfo()
    {
        return GetEventAndStudentsInShopCart().Select(s => new ShopCartInfo()
        {
            StudentId = s.Key,
            EventIds = s.Value
        }).ToList();
    }
    private int GetTotalShopCartItems()
    {
        return _ShopCart.TotalNumberOfItems;
    }
    private IEnumerable<ShopCartItem> GetShopCartItems()
    {            
        return _ShopCart.ShopCartItems;
    }        
    private void AddEventToCart(int eventId, decimal fee, int studentId)
    {
        _ShopCart.AddItem(eventId, studentId, fee);
    }                      
    private void RemoveEventFromCart(int eventId, int studentId)
    {            
        _ShopCart.RemoveItem(studentId, eventId);

    }
}

public class ShopCart
{       
    private List<ShopCartItem> _ShopCartItems;
    public static ShopCart Current
    {
        get
        {
            var cart = HttpContext.Current.Session["Cart"] as ShopCart;
            if (cart == null)
            {
                cart = new ShopCart();
                HttpContext.Current.Session["Cart"] = cart;
            }
            return cart;
        }
    }
    public IEnumerable<ShopCartItem> ShopCartItems
    {
        get
        {
            return _ShopCartItems;
        }
    }
    public int TotalNumberOfItems
    {
        get
        {
            return _ShopCartItems.Count();
        }
    }
    public ShopCart()
    {
        _ShopCartItems = new List<ShopCartItem>();
    }
    public void AddItem(int eventId, int studentId, decimal fee)
    {
        _ShopCartItems.Add(new ShopCartItem(eventId, studentId, fee));
    }
    public void RemoveItem(int studentId, int eventId)
    {
        ShopCartItem itemToRemove = _ShopCartItems.FirstOrDefault(s => s.StudentId == studentId && s.EventId == eventId);
        _ShopCartItems.Remove(itemToRemove);
    }        
}
public class ShopCartItem
{
    public int EventId { get; set; }
    public int StudentId { get; set; }
    public decimal Fee { get; set; }

    public ShopCartItem(int eventId, int studentId, decimal fee)
    {
        this.EventId = eventId;
        this.StudentId = studentId;
        this.Fee = fee;
    }
}
}

HTML File

<!DOCTYPE html>

<html xmlns="http://www.w3.org/1999/xhtml">
<head runat="server">
    <title>After-School Demo</title>
    <link href="Content/bootstrap.min.css" rel="stylesheet" />
    <link href="Content/site.css" rel="stylesheet" />
</head>
<body>
<form id="form1" runat="server">
<div class="container-fluid col-md-8 col-md-offset-2" id="afterschool-enrollment">
<div class="row">
        <h4 class="label-danger" id="totalCartItems">You have 0 items</h4>
    </div>
    <div class="row">
        <div class="page-header">
            <h2>Welcome</h2>
        </div>
    </div>
    <div class="row">
        <div class="jumbotron col-md-4">
        </div>
        <div class="col-md-8">
            <p>
                Lorem Ipsum is simply dummy text of the printing and typesetting industry. 
            Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, 
            when an unknown printer took a galley of type and scrambled it to make a type specimen book. 
            </p>
        </div>

    </div>
    <div class="row">
        <div class="col-md-3">
            <select class="form-control" id="select-students" data-bind="options: students,
                                                                        optionsText: 'name',
                                                                        optionsValue: 'studentId', value: selectedChoice">                                                                                        
            </select>
        </div>
    </div>
    <hr />
    <div class="row">
        <div class="col-md-12">               
            <table id="afterschool-table" data-show-header="false" class="table table-bordered table-hover">                  
                <!-- ko foreach: students -->
                <tbody data-bind="template: { name: 'enrollment-template', foreach: dayInfo }">
                </tbody>
                <!-- /ko -->                     
            </table>
            <input type="button" value="Checkout" class="btn btn-default pull-right" />
        </div>
    </div>

</div>    
    <script type="text/html" id="enrollment-template">
        <!-- ko foreach: dayInfoDetail -->
        <tr data-bind="visible: $root.shouldShowRow($parents[1].studentId)">
            <!-- ko if: ($index() % $parent.itemsByDay == 0) -->
            <td data-bind="click: function () { $root.addToCart($parent) }, text: $parent.dayName, attr: { rowspan: $parent.itemsByDay, class: $parent.added() ? 'selected hasRowSpan col-md-2' : 'hasRowSpan col-md-2' }"></td>
            <!-- /ko -->
            <td class="col-md-6" data-bind="text: className"></td>
            <td class="col-md-1" data-bind="text: fee"></td>
            <td class="col-md-3"><a href="#" data-bind="click: function () { $root.addToCart($data, $parent) }, text: added() ? 'In Cart' : 'Add'"></a>
                <a class="btn-remove" href="#" data-bind="click: $root.removeFromCart, visible: added()">Remove</a>
            </td>
        </tr>
        <!-- /ko -->
    </script>
    <script src="Scripts/jquery-2.1.4.min.js"></script>
    <script src="Scripts/underscore-min.js"></script>
    <script src="Scripts/bootstrap.min.js"></script>
    <script src="Scripts/knockout-3.3.0.js"></script>
    <script src="Scripts/after-school.js"></script>
    <script type="text/javascript">
        $(function () {
            Demo.AfterSchool.init();
        });
</script>

JavaScript file

var Demo = Demo || {};

Demo.AfterSchool = (function () {
var settings =
{
    cartNumberSelector: 'totalCartItems'
},

_AfterSchoolInformation = [];

function loadAfterSchools() {
    var afterSchoolInformation = getAfterSchoolInformation();
    $.when(afterSchoolInformation).done(function (data) {
        var initialData = data.d;
        ko.applyBindings(new StudentModel(initialData));
    });
}

function getAfterSchoolInformation() {
    return $.ajax({
        type: 'GET',
        contentType: 'application/json',
        dataType: 'json',
        cache: false,
        url: "Home.aspx/GetAfterSchoolInformation"
    });
}

var StudentModel = function (afterSchoolInformation) {
    var self = this;

    self.cartItems = ko.observableArray(ko.utils.arrayMap(afterSchoolInformation.ShopCartInfo, function (shopCartInfo) {
        return {
            studentId: shopCartInfo.StudentId,
            eventIds: ko.observableArray(shopCartInfo.EventIds)
        }
    }));

    self.totalShopCartItems = afterSchoolInformation.TotalShopCartItems;       

    self.selectedChoice = ko.observable();

    self.shouldShowRow = function (parameter) {
        return self.selectedChoice() == parameter;
    };

    self.students = ko.observableArray(ko.utils.arrayMap(afterSchoolInformation.StudentInfo, function (student) {
        return {
            studentId: student.StudentId,
            name: student.Name,
            dayInfo: ko.observableArray(ko.utils.arrayMap(student.DayInfo, function (dayinfo) {                  
                return {
                    added: ko.observable(dayinfo.Added),                        
                    dayName: dayinfo.DayName,
                    itemsByDay: (dayinfo.DayInfoDetail.length),
                    eventId: dayinfo.EventId,
                    fee: dayinfo.Fee,
                    dayInfoDetail: ko.observableArray(ko.utils.arrayMap(dayinfo.DayInfoDetail, function (detail) {
                        return {
                            added: ko.observable(detail.Added),
                            className: detail.ClassName,
                            eventId: detail.EventId,
                            fee: detail.Fee
                        }
                    }))
                }
            }))
        };
    }));

    self.addToCart = function (dayInfo, parentItem) {          
        if (dayInfo.added()) {
            return self.removeFromCart(dayInfo);
        }
        var eventId = dayInfo.eventId;          
        var studentId = self.selectedChoice();

        var cartPromise = addCartItem(eventId, studentId);

        $('#afterschool-table').css({ opacity: 0.5 });
        $.when(cartPromise).done(function (data) {
            $('#afterschool-table').fadeTo("normal", 1);
            updateCartItemsText(data.d);
            dayInfo.added(true);              
        });
    }      

    self.removeFromCart = function (dayInfo) {
        var eventId = dayInfo.eventId;
        var studentId = self.selectedChoice();

        var cartPromise = removeCartItem(eventId, studentId);

        $('#afterschool-table').css({ opacity: 0.5 });

        $.when(cartPromise).done(function (data) {
            $('#afterschool-table').fadeTo("normal", 1);
            updateCartItemsText(data.d);
            dayInfo.added(false);
        });
    }

    function addCartItem(eventId, studentId) {
        return $.ajax({
            type: 'GET',
            contentType: 'application/json',
            dataType: 'json',
            cache: false,
            url: "Home.aspx/AddToCart?eventId=" + eventId + "&fee=12.12" + "&studentid=" + studentId
        });
    }

    function removeCartItem(eventId, studentId) {
        return $.ajax({
            type: 'GET',
            contentType: 'application/json',
            dataType: 'json',
            cache: false,
            url: "Home.aspx/RemoveFromCart?eventId=" + eventId + "&studentid=" + studentId
        });
    }

    function updateCartItemsText(currentNumber) {
        var elementToUpdate = document.getElementById(settings.cartNumberSelector);
        var textToReplace = elementToUpdate.innerHTML;
        elementToUpdate.innerHTML = textToReplace.replace(new RegExp("[0-9]+", "g"), currentNumber);
    }
};

return {
    init: loadAfterSchools
}}());

Utils

public class Utils
{
    public static Dictionary<string, string> GetDayOfWeeksStructure()
    {
        return new Dictionary<string, string>(){
            { "Mo", "Monday" },
            { "Tu", "Tuesday" },
            { "We", "Wednesday" },
            { "Th", "Thursday" },
            { "Fr", "Friday" }
        };
    }
    public static string GetDayNameBySuffix(string suffix)
    {
        return GetDayOfWeeksStructure().GetValue<string, string>(suffix).ToString();
    }
}

Extensions

public static class DictionaryExtensions
{
    public static object GetValue<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)
    {
        if (!dictionary.ContainsKey(key))
        {
            return default(TValue);
        }

        return dictionary[key];
    }
}

Github repository

https://github.com/EdsonAlcala/code-review-question

\$\endgroup\$
8
\$\begingroup\$

Why did you write that GetValue DictionaryExtension when Dictionary<TKey, TValue>.TryGetValue already exists?


I'm not a fan of properties like IEnumerable<StudentInfo> StudentInfo where the name of the property doesn't reflect that it's an IEnumerable<T>. Sure, StudentInfos is not exactly great, which probably means the class name StudentInfo isn't that great and perhaps should change. Quite frankly I can't see why that class isn't simply called Student?


Another naming issue: from data in result.AsEnumerable(): data is a plural and isn't telling me anything about what it contains.


Also, why does new FakeDatabase().GetAfterSchoolInformation() return a DataTable which you then immediately have to convert using .AsEnumerable()? Avoid working with the likes of DataTable and instead convert data from your database to custom classes as soon as possible. Why not use an ORM like Entity Framework or NHibernate etc.? That way you also don't need to have ugly code like data.Field<string>("DayOfWeek") (yay, magic strings!).


Also, be consistent in naming : DayWeek = data.Field<string>("DayOfWeek"). I would expect the database field DayOfWeek to map to the property DayOfWeek, instead it's called DayWeek.


I have no words for this:

.OrderBy(s => s.DayName == "Fr")
.ThenBy(s => s.DayName == "Th")
.ThenBy(s => s.DayName == "We")
.ThenBy(s => s.DayName == "Tu")
.ThenBy(s => s.DayName == "Mo")

It's obvious DayName doesn't contain the name, but instead an abbreviation, so the property is named incorrectly. Which you then "fix" by doing this: s.DayName = Utils.GetDayNameBySuffix(s.DayName);.

And this sort is just... Why not have a numeric field -- e.g. DayId -- and sort on that?


You should move a lot of code from Home to one or more separate classes. Code like this just isn't right when you're in the Home class already:

Home instance = new Home();
instance.RemoveEventFromCart(eventId, studentid);

Avoid magic strings. Things like the names of session variables -- e.g. "Cart" -- should be constants, preferably in a separate class.


_ShopCartItems is a List<T> so you could use its Count property, not the extension method Count();


Why is EventIds on ShopCartInfo an array and not an IEnumerable<T>?


Quite frankly your whole structure feels unwieldy and needlessly hierarchical. Why is there a Fee on DayInfo and also on DayInfoDetail? Ditto EventId. StudentId is both in StudentInfo and ShopCartInfo. To me it feels like this structure should be more about relationships between various objects.

I would also encourage you to separate your layers, e.g. have a data layer that deals with the db, have a business layer where you keep all the logic, have a presentation layer that is as slim as possible,...

\$\endgroup\$
  • \$\begingroup\$ The extension method Count() attempts to cast the passed in IEnumerable to an ICollection, and, if successful, calls the ICollection's Count property. Since a List is an ICollection, whether you use Count() or Count makes little difference. \$\endgroup\$ – Risky Martin Jul 14 '15 at 10:38
  • \$\begingroup\$ Hey thanks for your comments, I appreciate it so much. 1. I added the GetValue extension for readability purpose, for me it is easier to read. 2. new FakeDatabase().GetAfterSchoolInformation() returns a datatable because it is how the data layer is working now, I know there are many ORM and Dapper but what can I say, sometimes we cannot change the whole system hehe. So I'm trying to do what I can with the tools I have. I'm going to fix the code :) thanks man \$\endgroup\$ – Edson Alcalá Jul 16 '15 at 0:10
4
\$\begingroup\$

Great points already covered by BCdotWEB's answer but there's a couple things I think that are worth adding:

public class Utils
{
    public static Dictionary<string, string> GetDayOfWeeksStructure()
    {
        return new Dictionary<string, string>(){
            { "Mo", "Monday" },
            { "Tu", "Tuesday" },
            { "We", "Wednesday" },
            { "Th", "Thursday" },
            { "Fr", "Friday" }
        };
    }
    public static string GetDayNameBySuffix(string suffix)
    {
        return GetDayOfWeeksStructure().GetValue<string, string>(suffix).ToString();
    }
}

The first problem is that Utils is a really bad name.

  1. It's an abbreviation of utilities
  2. It doesn't describe what the class does
  3. It will certainly just keep being added to until it is a big mess of random functionality.

I see this occassionally and it becomes a magnet for stuff which doesn't quite fit somewhere else.


As all of the members are static the class should be static too.


public static class DayOfTheWeekUtilities // Still don't like the separate utilities class

Your next name is also a bit off GetDayOfTheWeeksStructure:

  1. Should be GetDaysOfTheWeekStructure
  2. Why is this a "Structure" and not a lookup or mapping or anything more descriptive?

Your mapping between the two strings should actually be a field, you're creating new dictionaries needlessly.

private static IDictionary<string, string> DayOfTheWeekMapping = new Dictionary<string, string> 
{

Notice that I've removed the unecessary () from the declaration too.


Another name, another slight problem. A suffix is at the end, you're looking for the word prefix :)


There's a problem on your next line that I also want to explain before I finish up:

return GetDayOfWeeksStructure().GetValue<string, string>(suffix).ToString();

Now, I'm supplying two type arguments to GetValue (string, string) why do I also have to call ToString() on the result?

It's because your signature for that method is incorrect:

public static object GetValue<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)

should be

public static TValue GetValue<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)

So, now we have all of those points covered, how does Utils look now?

public static class DayOfTheWeekUtilities
{
    private static IDictionary<string, string> DayOfTheWeekMapping = new Dictionary<string, string> 
    {
        { "Mo", "Monday" },
        { "Tu", "Tuesday" },
        { "We", "Wednesday" },
        { "Th", "Thursday" },
        { "Fr", "Friday" }
    };

    public static string GetDayOfTheWeekByPrefix (string prefix)
    {
        string dayOfTheWeek;
        if (DayOfTheWeekMapping.TryGetValue(prefix, out dayOfTheWeek))
        {
            return dayOfTheWeek;
        }
        return string.Empty;
    }
}

I would still prefer to see this functionality somewhere else.

\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for your comments. I really appreciate it , can I ask you what you mean with "I would still prefer to see this functionality somewhere else" \$\endgroup\$ – Edson Alcalá Jul 16 '15 at 0:10
  • \$\begingroup\$ I find that I very rarely write "utitlities" classes. When I do, I often realise later that I was missing a central concept. E.g. you might need a DayPrefixClass with properties for Prefix and Name. The class could have a static property to get them all, or get one by prefix. You could also use the collection to populate a drop down list. Make sense? \$\endgroup\$ – RobH Jul 16 '15 at 12:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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