Assigning properties

Question

I am leaving my first job because I always worked alone on projects from my day one, which was both useful but also taught me some bad practices some of which I managed to realize but many others are still there. Not being able to learn from colleagues, constantly being pushed with deadlines because it's a really small company with 3 spaghetti code developers. In certain parts of the projects I focus on architecture, I read a lot, trying methodologies and different approaches but once deadlines and grumpy management comes across, my code quality drops tremendously.

One of the things that bother me is that in nearly every project I often end-up writing things like the examples below, filling classes and having a lot of logic in the direct assigning of properties, teritary operators, LINQ and whatnot knowing this is definitely bad but never bothering to look for a solution.

1. Filling a hotel information class that comes from a SOAP service.

   var offersToBind = filteredOffers.Select(h =>
   new HotelInformation
           {
               OtherOffersCount = h.AllOffers.Count,
               PriceType = h.LowestOffer.Rates == null ? "Редовна цена" : h.LowestOffer.Rates[0].SpoInfos.Any() ? GetSpoTypeName(h.LowestOffer.Rates[0].SpoInfos[0].SpoType) : "Редовна цена",
               HotelImage = h.HotelInfo.FirstImage() != null ? GetHotelPicture(h.HotelInfo.FirstImage()) : "http://loremflickr.com/320/240/dog",
               HotelName = h.HotelInfo.HotelDetails.FirstOrDefault(hd => hd.LanguageId == LangId).Name,
               HotelPrice = (h.LowestOffer.TotalPrice * 1.95583).ToString("0.00") + " лв.",
               HotelBoard = h.LowestOffer.BoardName,
               HotelResort = resorts.FirstOrDefault(r => r.SejourId == h.HotelInfo.ResortID)?.ResortDetails.FirstOrDefault(r => r.LanguageId == LangId)?.ResortName,
               ExtrasPrice = (h.LowestOffer.ExtraPrice * 1.95583).ToString("0.00"),
               RoomName = h.HotelInfo.HotelRooms
                      .Where(r => r.SejourRoomType == h.LowestOffer.RoomType
                          && r.SejourRoomTypeName == h.LowestOffer.RoomTypeName
                          && r.SejourRoomName == h.LowestOffer.RoomName
                          && r.SejourRoom == h.LowestOffer.Room
                          && r.LanguageId == LangId)
                      .Select(r => r.Name)
                      .DefaultIfEmpty("[Missing room information]")
                      .First(),
               DateFrom = periodFrom.ToString("dd.MM"),
               DateTo = periodTo.ToString("dd.MM"),
               Nights = (periodTo - periodFrom).Days,
               Identificator = Helper.GenerateRandomString(8)
           }).ToList();
   

2. Another example of the same issue but this time working locally in a similar manner and passing the select result to a repeater.

   promotions.Select(p => new
               {
                   PromotionDateRange = p.DateStart.HasValue & p.Date.HasValue ? p.DateStart.Value.ToString("dd/MM/yyyy") + " - " + p.Date.Value.ToString("dd/MM/yyyy") : string.Empty,
                   PromotionImage = p.Media.Any() ? ConfigurationManager.AppSettings["ProductPictures"] + p.Media.First().FileName : string.Empty,
                   PromotionPoints = p.Points,
                   PromotionName = p.Name,
                   PromotionContent = p.Contents,
                   PromotionPrice = p.Price.HasValue & p.Price.Value != 0 ? p.Price.Value.ToString("G") + Currency : string.Empty,
                   PromotionOldPrice = p.OldPrice.HasValue & p.OldPrice.Value != 0 ? p.OldPrice.Value.ToString("G") + Currency : string.Empty,
                   PromotionPriceVisibility = (p.Price.HasValue & p.Price != 0) && (p.OldPrice.HasValue & p.OldPrice != 0) ? string.Empty : "display: none",
                   DiscountPercent = p.Discount.HasValue ? p.Discount.Value.ToString("#") + "%" : string.Empty,
                   DiscountAmount = p.OldPrice.HasValue & p.OldPrice.Value != 0 & p.Price.HasValue & p.Price != 0 ? $"({(p.OldPrice - p.Price).Value.ToString("G") + Currency})" : string.Empty,
                   PromotionDiscountVisibility = (p.Discount.HasValue & p.Discount.Value != 0) ? string.Empty : "display: none;",
                   PromotionLink = $"/{LangCode}/product/{p.ProductID}"
               });
   

I know this logic has to happen or be written somewhere, even if it's in a different manner, but I am not sure how to approach it. I had some ideas, but my ideas are not always guaranteed to be the common approach the software community is using. Which is why I decided this is the best time to start asking isolated questions about isolated issues.

I was thinking of some object initializers and moving the logic there, but I would still eventually write that kind of logic and checks one way or another and it's really hard to read. There are other bad apples here, the smaller type but they aren't my focus currently.

Answer 1

I know this logic has to happen or be written somewhere, even if it's in a different manner, but I am not sure how to approach it.

Class Design

A good class model manages complexity. A good class hides state and exposes functionality.

This code is deferring all the details of all the construction of all the objects to the very last possible moment and then instantiates the entire HotelInformation universe in a big bang. This is the antithesis of object oriented design.

Instead, a logical hierarchy of objects gets built up in layers if you will. With each class/object encapsulating its own state details the overall effect is that at any given point in a class composition, the construction is simple.

---

Random Observations

• Be wary of using LINQ as an inappropriate device for things that should be encapsulated in a class
• Use real DateTime objects.
• Think real hard about each class and its purpose. Apply the Single Responsibility Principle mercilessly. SRP is the most important "bizz" of the SOLID buzzword.

---

Edit

Refactoring.

It's nice to say do the right thing from the beginning, but it is refactoring that will save your sanity with that codebase. You must get a long term perspective and accept that it will take patience, persistence, and study. But it is your ball of mud now and getting control of it is a very satisfying experience.

• Refactoring: Improving the Design of Existing Code
  • get. this. book.
  • The seminal work on the subject. A recipe cookbook for specific refactoring problems.
• Brownfield Application Development with .NET
• Working with Legacy Code
• Big Ball of Mud
• My recent watch on the subject. Yes, it's about refactoring, not Ruby
  • RailsConf 2016 - Get a Wiff of This
  • Everything in here is applicable. You will get that "OMG there is hope" feeling. The examples will give you LINQ deja-vu.

2 Big Bangs for the Buck

• Put version control on your computer. This will free you to boldly go where no refactoring has gone before.
• Wishful Thinking Design
  • My answer to Avoiding Cascading Refactoring

end Edit

---

I pulled this up from the comments

... what do you mean by real DateTime objects.I was converting to string cause I was passing this to the .aspx page for visualization, similar to a ViewModel but not sure if I can call it this way.

Regarding datetimes as strings: Each differently formatted datetime essentially becomes its own type. I must give each format special handling or else code is blowing up. I cannot pass these things except to other specialized methods; that means duplicate code. Datetime arithmetic is severely hampered. Converting to a real DateTime defaults undefined parts (parts not in the string) which will probably cause errors down stream. And passing a date-as-string is a violation of SRP. Let the client decide how it want's the date to look.

Answer 2

Nitpick - ? : is a ternary operator (also called conditional operator or inline if) not the teritary operator.

edit: I am assuming that both promotions and filteredOffers are IEnumerable<T> rather than IQueryable<T>.

As Mug has commented, there's not enough context to go too deep here but let's look at one line:

PriceType = h.LowestOffer.Rates == null ? "Редовна цена" : h.LowestOffer.Rates[0].SpoInfos.Any() ? GetSpoTypeName(h.LowestOffer.Rates[0].SpoInfos[0].SpoType) : "Редовна цена"

Nesting ternaries is almost always a bad idea anyway. You could introduce a method:

PriceType = GetPriceType(h.LowestOffer),

Another option is to create a constructor for HotelInformation which deals with the mapping but there are numerous things in the mapping that looks odd to me. e.g.

• Why is Rates a collection if you only care about the first one?
• Why are you using & instead of &&? Because & doesn't short circuit, if the value is null you'll get an exception at runtime.
• Why are you concatenating strings inside an interpolated string?
• What do the magic numbers and strings mean? They should be named constants or variables

It also looks like you're missing some services:

h.HotelInfo.FirstImage() ? GetHotelPicture(h.HotelInfo.FirstImage()) : "http://loremflickr.com/320/240/dog"

Should look more like:

hotelPictureService.GetHotelPictureUri(h.HotelInfo)

Mapping code is never pretty, the key is for it to not be in the way! That means at the top level you might have something like:

var offersToBind = filteredOffers
    .ProjectToHotelInformations()
    .ToList();

or

var offersToBind = hotelInformationMapper.CreateFromOffers(filteredOffers);

Now you can

1. Reuse the mappings
2. Test the mappings
3. Read the method that uses the mapping without tripping over all the code for it