Skip to main content
Code Review

Return to Answer

added 46 characters in body
Source Link
RobH
  • 17.1k
  • 6
  • 38
  • 73

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

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

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

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
added 46 characters in body
Source Link
RobH
  • 17.1k
  • 6
  • 38
  • 73

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

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

Nitpick - ? : is the ternary operator not the teritary operator.

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

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

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
Source Link
RobH
  • 17.1k
  • 6
  • 38
  • 73

Nitpick - ? : is the ternary operator not the teritary operator.

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
lang-cs

AltStyle によって変換されたページ (->オリジナル) /