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.

• Mathieu Guindon: Welcome to CR! Rule of thumb, if your question title reads like a question, it's probably a bad title for a CR post. You may have noticed the watermark in the title box, asking you to tell us the purpose of your code - please edit as such. See How to Ask and how to get the best value out of CR for more information.
• Mathieu Guindon: Reading your post, the snippets are completely stripped of the surrounding context, which makes it very hard for reviewers. What are promotions? Where to they come from? What's a HotelInformation? Where are the filteredOffers and why do you need offersToBind? Tell us more about your code, show us more of it, and less of your bio.
• radarbob: Ditto, Mat. We're here to judge the code, not the author.
• Troublesome Junior: I should have read those posts first, I realize after your remarks that I went too much into my personal story and self-excusing rather than providing relevant information.

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.

• Troublesome Junior: Your big-bang analogy is eye-opening, you have no idea. I got a quick question though, 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.
• Troublesome Junior: Scratch that, I know what mislead you, I wrote it needs to be sent to a SOAP service. That is not correct.
• radarbob: 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

• Servy: ? : is the conditional operator, not the ternary operator. Also pretty much everything in this answer only applies if the LINQ queries are on an IEnumerable; almost none of them would apply to an IQueryable.
• RobH: There are already method calls in the first example @Servy so at leats the first must be on an in memory collection.
• RobH: @Servy I've also clarified the nitpick. ?: is a ternary operator but you are right that it's more correct to call it the conditional operator.
• Troublesome Junior: Thank you for your input @RobH, I appreciate your input and this has sent me on the right track to read and is consistent with some ideas I had. We are (were) not a traditional software company and similar code was to be seen in the code of devs with 10 years of experience, because no one controls quality. It just has to work. Your questions are mostly remarks some of which I know but I will investigate. To the Rates questions, that's part of the API that isn't mine and which is written in similar way to my code by another company (I have the source), those Rates only had 1 result always.
• Troublesome Junior: The most important thing I learned here is your bottom post suggestions, this code is one year old and I started learning and trying to implement a better overall structure for my project with service layer, DI and trying to hide the pesky logic so the code is readable. I believe I am on the right track but this should speed things up a lot.