3
\$\begingroup\$

With the code I'll provide my objective is to build a map (table) based on the trips the truckers should do for each week with all of the charge/discharge operations. I do not need a deep analysis, just some ideas on how I could make my code more readable and/or efficient.

@page "/Dispoplan"
@using xizTrucks.Models
@using xizTrucks.Services
@using Color = System.Drawing.Color
@using System.Drawing
@using System.Globalization

@inject MovementsRecordsService MovementsRecordsService;

<style>
    td, th {
      text-align: center;
      border: 1px solid black;
    }
   
    tr:nth-child(even) {
      background-color: #f2f2f2;
    }
    
    .NoMarginText {
        margin: 0;
   }
</style>


<RadzenCard Style="margin-top: -35px">
    <RadzenStack Orientation="Orientation.Horizontal" Style="margin: 5px">
        <div style="display: flex; align-items: center;">
            <RadzenButton Icon="refresh" Size="ButtonSize.Small" Click="UpdateTable" style="margin-right: 10px;"/>
            <a>Semana:</a><RadzenNumeric TValue="int" Min="1" Max="@_yearWeeks" Style="width: 50px; height: 30px; margin: 5px" Value="Week" Change="@(x => { Week = x; UpdateTable(); })"/>
            <div style="height: 15px; width: 15px; background-color: @GetColor("C"); border: 1px solid black; margin-right: 5px;"></div>
            <a style="margin-right: 10px;">Carga</a>
            <div style="height: 15px; width: 15px; background-color: @GetColor("D"); border: 1px solid black; margin-right: 5px;"></div>
            <a>Descarga</a>
        </div>


    </RadzenStack>

    <table class="table" style="width: 100%">
        <thead>
        <tr style="background-color: gainsboro">
            <th>
                <RadzenText Text="Funcionário" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Veículo/Reboque" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Export" class="NoMarginText"/>
            </th>

            <th>
                <RadzenText Text="Segunda-Feira" class="NoMarginText"/>
                <RadzenText Text="@DayOfWeek.Monday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year).toDateString()" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Terça-Feira" class="NoMarginText"/>
                <RadzenText Text="@DayOfWeek.Tuesday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year).toDateString()" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Quarta-Feira" class="NoMarginText"/>
                <RadzenText Text="@DayOfWeek.Wednesday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year).toDateString()" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Quinta-Feira" class="NoMarginText"/>
                <RadzenText Text="@DayOfWeek.Thursday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year).toDateString()" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Sexta-Feira" class="NoMarginText"/>
                <RadzenText Text="@DayOfWeek.Friday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year).toDateString()" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Sábado" class="NoMarginText"/>
                <RadzenText Text="@DayOfWeek.Saturday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year).toDateString()" class="NoMarginText"/>
            </th>
            <th>
                <RadzenText Text="Observações" class="NoMarginText"/>
            </th>
        </tr>
        </thead>
        <tbody>
        @foreach (var item in TableItems)
        {
            <tr style="align-content:center;">
                <td style="text-align:center; vertical-align:middle; background-color: @GetColor(item.EmployeeName + item.VehicleLicensePlate)">
                    <RadzenText Text="@item.EmployeeName" TextStyle="TextStyle.Subtitle1" class="NoMarginText"/>
                    <RadzenText Text="@item.EmployeeContact" class="NoMarginText"/>
                    <RadzenText Text="@item.TripCode" class="NoMarginText"/>
                </td>
                <td style="text-align:center; vertical-align:middle; background-color: @GetColor(item.EmployeeName + item.VehicleLicensePlate)">
                    <RadzenText Text="@(item.VehicleLicensePlate + " / " + item.TrailerLicensePlate)" class="NoMarginText"/>
                </td>
                @for (var i = 0; i < 7; i++)
                {
                    if (item.DaysData.ContainsKey((DayOfWeek)i))
                    {
                        <td style="background-color: @GetColor(item.EmployeeName + item.VehicleLicensePlate); padding: 0">
                            @if (item.DaysData.Any())
                            {
                                @foreach (var record in item.DaysData[(DayOfWeek)i])
                                {
                                    <div style="background-color: @GetColor(record.Type); margin: 5px">
                                        <RadzenText Text="@(record.CountryCode + " " + record.PlaceCode + " - " + record.Place)" class="NoMarginText"/>
                                        <RadzenText Text="@(record.CostumerName)" class="NoMarginText"/>
                                    </div>
                                }
                            }
                        </td>
                    }
                    else
                    {
                   
                        if (((DayOfWeek)i).GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year) >= item.FirstDate && 
                            ((DayOfWeek)i).GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year) <  item.LastDate)
                        {
                            <td style="background-color: @GetColor(item.EmployeeName + item.VehicleLicensePlate)"/>
                        }
                        else
                        {
                            <td/>
                        }
                    }
                }
                <td>
                    @if (item.FirstDate.Date < DayOfWeek.Sunday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year))
                    {
                        <RadzenText Text="@("Começou na semana " + DateHelper.GetWeekOfYear(item.FirstDate) + "\n(" + item.FirstDate.toDateString() + ")")"  class="NoMarginText"/>
                    }
                    @if(item.LastDate.Date > DayOfWeek.Saturday.GetDateTimeFromDayOfWeek(Week, DateTime.Now.Year))
                        {
                            <RadzenText Text="@("Continua até à semana " + DateHelper.GetWeekOfYear(item.LastDate) + "\n(" + item.LastDate.toDateString() + ")")" class="NoMarginText"/>
                        }
                </td>
            </tr>
        }
        </tbody>
    </table>
</RadzenCard>

@code {

    private List<TableItem> TableItems { get; set; } = new();
    private int _yearWeeks;
    private int Week { get; set; }

    protected override async Task OnInitializedAsync()
    {
        await base.OnInitializedAsync();

        var dfi = DateTimeFormatInfo.InvariantInfo;
        var cal = dfi.Calendar;
        var date = new DateTime(DateTime.Now.Year, 12, 31);
        _yearWeeks = cal.GetWeekOfYear(date, dfi.CalendarWeekRule, dfi.FirstDayOfWeek);

        Week = cal.GetWeekOfYear(DateTime.Now, dfi.CalendarWeekRule, dfi.FirstDayOfWeek);

        UpdateTable();

        var timer = new System.Timers.Timer(30000);

        timer.Elapsed += (_, _) => { UpdateTable(); };

        timer.Start();
    }

    private async void UpdateTable()
    {
        TableItems.Clear();

        var loads = await MovementsRecordsService.GetMovementRecords(new MovementRecordRequest
        {
            Type = "C"
        });

        var unloads = await MovementsRecordsService.GetMovementRecords(new MovementRecordRequest
        {
            Type = "D"
        });

        var loadsAndUnloads = loads.Concat(unloads).ToList();

        var trips = await MovementsRecordsService.GetMovementRecords(new MovementRecordRequest
        {
            Type = "V"
        });

        var tableItems = new List<TableItem>();
        foreach (var trip in trips)
        {
            var tableItem = await CreateTableItem(trip, loadsAndUnloads.Where(x => x.Code == trip.Ui).ToList());
           
            if (tableItem != null)
            {
                tableItems.Add(tableItem);
            }
        }
        
        TableItems = tableItems.OrderBy(x => x.EmployeeName).ToList();
    }
    
    private async Task<TableItem> CreateTableItem(MovementRecord record, IReadOnlyCollection<MovementRecord> loadsAndUnloads )
    {
        var tableItem = new TableItem
        {
            EmployeeName = record.EmployeeName,
            EmployeeContact = record.Employee.PhoneNumber,
            VehicleLicensePlate = record.VehiclePlate,
            TrailerLicensePlate = record.TrailerPlate,
            TripCode = record.Id
        };
    
        if (!loadsAndUnloads.Any()) return null;

        var weekLoadsAndUnloads = loadsAndUnloads.Where(x => DateHelper.GetWeekOfYear(x.Date) == Week).ToList();
        if (weekLoadsAndUnloads.Any())
        {
            var groups = weekLoadsAndUnloads.GroupBy(movementRecord => movementRecord.Date?.DayOfWeek);
            foreach (var group in groups)
            {
                if (group.Key != null)
                {
                    tableItem.DaysData.Add(group.Key.Value, group.ToList());
                }
            }
        }
       
        // Calculate the minimum and maximum dates from the collection of MovementRecord objects
        var nonNullDates = loadsAndUnloads.Where(mr => mr.Date.HasValue).Select(mr => mr.Date.Value).ToList();
        if (!nonNullDates.Any()) return tableItem;
        
        tableItem.FirstDate = nonNullDates.Min();
        tableItem.LastDate = nonNullDates.Max();
    
        return DateHelper.IsWeekBetweenDates(tableItem.FirstDate, tableItem.LastDate, Week, DateTime.Now.Year) ? tableItem : null;
    }


    public class TableItem
    {
        public string EmployeeName { get; init; }
        public string EmployeeContact { get; init; }
        public string VehicleLicensePlate { get; init; }
        public string TrailerLicensePlate { get; init; }
        public string TripCode { get; init; }

        public DateTime FirstDate { get; set; }
        public DateTime LastDate { get; set; }

        public Dictionary<DayOfWeek, List<MovementRecord>> DaysData { get; set; } = new();
    }

}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Off, but you were the one who has posted the 10 000th C# question :) \$\endgroup\$ Commented Apr 28, 2023 at 19:37

1 Answer 1

8
\$\begingroup\$

Name what it is, not how it is implemented in C# -

  • public class TableItem Trip
  • private async void UpdateTable() UpdateTrips

No random numbers

  • @for (var i = 0; i < 7 operatingWeekDays ; i++)

"7 "is implementation, not what it is. It is random as far as the reader knows.


Type is nothing?

Type is never declared explicitly. Its value is set and reset three times on every UpdateTable call. Why?

There is no Type for loadsAndUnloads. What is the type value for "there is no type?"

DISCLAIMER: Any mistakes I make regarding purpose and use of Type is solely the fault of the code. Seriously. I am just guessing and that's no way to make code changes.


This looks too much like a method body. The line going off the right margin keeps the reader from realizing it's an illusion

var loads = await MovementsRecordsService.GetMovementRecords(new MovementRecordRequest
    {
        Type = "C"
    });

Try this:

var loads = await MovementsRecordsService
     .GetMovementRecords( new MovementRecordRequest {Type = "C"} );
  • "one line" the curly braces
  • spaces to make the parentheses stand out, especially the closing one

Prefer enums over strings for code values

Type = "C"

  • not type safe. The compiler cannot catch any error
  • different cases (C : c) do not match
  • strings default to null - exception risk
  • The string type has no meaning in your domain model.

enum

  • type safe. Compiler will catch an invalid value
  • defaults to zero - no (well, very low) exception risk
  • a nice, concise way of defining ALL code values; use zero as "NotAssigned" or "Undefined" or "NoneOfTheAbove", etc.
  • Reads as what it is, not its implementation, i.e. its cryptic code value.
  • If you need both the value and its name ( "C" : Type.load ) create a Dictionary for easy-peazy translation.
  • The enum name and member names have meaning in your domain model.
  • Virtually self documenting

Near identical names confusing

TableItems and tableItems - Too much the same. Any CASING typos are hard to see and will really mess things up!

Why create two objects only to add one to the other? Just to sort individual query results? This is a good example showing how 'Separation of Concerns' error can cause code chaos. Make a TableItems.Sort method. ...

OOPs. There is no TableItems class. See below.


A well designed class exposes functionality and hides state

Higher level code will become much more clear when all the busy work of creating, filtering, and sorting the objects it uses is moved to those classes.

TableItems

Make a TableItems class. Make methods for Sorting and the various filtering. Remove that implementation out of the "client" code.

  • A single TableItems collection class will significantly reduce complexity and busy code.
  • Add method should ensure valid collection members.
  • Make filtering methods as needed. .NET provides parameterized find methods for exactly this reason. Of course they take LINQ expressions!
  • .NET collections use your TableItem (in our case) overrides for sorting, filtering, and equality. Its power is amazing:
    • Actual Example: passed in a collection, compared to this collection and returned the difference. That Find method was one line of code.

TableItem

  • Make methods for direct property manipulations, such as casting to DateTime, particular date/time string formats, the DayOfWeek, etc.

  • Make constructors!! Client code manually creating a new TableItem is just bad. The Constructor is THE place to create valid, correct, complete, and robust objects.

    • No null Dates guaranteed. Set default Dates w/ DateTime.Min or DateTime.Max
    • No null strings. Set default to String.Empty to avoid exceptions.
  • Override Equals to define identity equality by TripCode. It looks to me that this is the case.

  • Override CompareTo - this defines default sort order for a collection of TableItem.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback :) \$\endgroup\$
    – davidsi02
    Commented May 11, 2023 at 13:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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