3
\$\begingroup\$

Question might be more appropriate in StackOverflow, however I'd also like a review.

I'd like to make my code inline as I feel like this could be possible, I'm just not sure how.

int highestWeightOfParcel  = 0;
if (collo.WeightGrammes.HasValue)
{
    if (collo.WeightGrammes > highestWeightOfParcel)
    {
        highestWeightOfParcel = collo.WeightGrammes.Value;
    }
}

Could I apply the same technique as this? :

int heigth = collo.HeightMm.HasValue ? collo.HeightMm.Value < 10 ? 1 : (collo.HeightMm.Value / 10) : 0,

Is there a 'better'/simpler way of writing this?

\$\endgroup\$
1
  • \$\begingroup\$ This question severely lacks context. The code also does not make any sense because when highestWeightOfParcel = 0 then it's the same as writing highestWeightOfParcel = collo.WeightGrammes ?? 0. This meas if it's not-null then it'll always be the highest value. Both ifs are completely unnecessary. I bet this is a part or a loop... \$\endgroup\$ – t3chb0t Jul 17 '18 at 18:00
19
\$\begingroup\$

There is really no need to check HasValue in this situation. It is sufficient to compare with highestWeightOfParcel because if collo.WeightGrammes is null the comparison is false for any value of highestWeightOfParcel:

int highestWeightOfParcel  = 0;

if (collo.WeightGrammes > highestWeightOfParcel)
{
    highestWeightOfParcel = collo.WeightGrammes.Value;
}
\$\endgroup\$
5
  • 2
    \$\begingroup\$ @Paramone Nullable<T> will NEVER throw NullReferenceException if you access its Value property when it has not a value. If this happen then to be null is collo (assuming it's a reference type) and for that you have ?.. That said THIS is the right code, compiler does everything correctly (not a surprise). If you REALLY want a single line then your first snippet MIGHT be rewritten as highestWeightOfParcel = Math.Max(highestWeightOfParcel, collo.WeightGrammes ?? 0) (assuming highestWeightOfParcel cannot be negative). \$\endgroup\$ – Adriano Repetti Jul 17 '18 at 15:51
  • \$\begingroup\$ Same comment as under the accepted answer: this code assigns twice to the same variable. That’s code smell and, although it might be seen as justified in this code, it’s really unnecessary. Treat all variables as readonly, this dramatically simplifies code flow. \$\endgroup\$ – Konrad Rudolph Jul 17 '18 at 17:31
  • \$\begingroup\$ @konrad it's a reasonable guess to assume that =0 is fictional otherwise the whole code is a smell. Single assignment is a golden rule (especially if you do not force it chaining complex expressions and multiple ternary operators like in the other OP's example). It has however, other disadvantages (often it's about readability and quick review of test coverage). Of course to introduce a function is even better. \$\endgroup\$ – Adriano Repetti Jul 17 '18 at 17:48
  • \$\begingroup\$ @AdrianoRepetti Exactly. Just encapsulate complex assignment expressions. But the one in this answer isn't complex enough to warrant this. This should be one expression, one line of code. \$\endgroup\$ – Konrad Rudolph Jul 17 '18 at 18:16
  • \$\begingroup\$ @KonradRudolph I'd agree, unfortunately this question is too much fictional to suggest something useful (Is Math.Max() enough? Is highestWeightOfParcel truly initialized with 0? Is it part of a more complex context which is (or should be) already in a separate function?) \$\endgroup\$ – Adriano Repetti Jul 18 '18 at 13:13
1
\$\begingroup\$
int highestWeightOfParcel  = (collo.WeightGrammes.HasValue && (collo.WeightGrammes > highestWeightOfParcel))? collo.WeightGrammes.Value:0;

That line code can perhaps be written as above. However, ternary operator tends to become unreadable fairly quickly. Alternate option could be this:

int highestWeightOfParcel  = 0;
if ((collo.WeightGrammes.HasValue) && 
    (collo.WeightGrammes > highestWeightOfParcel)) {
        highestWeightOfParcel = collo.WeightGrammes.Value;
}
\$\endgroup\$
4
  • \$\begingroup\$ Awesome, thank you! I have to agree with you, though. It does get somewhat unreadable as it goes on and on.. \$\endgroup\$ – Paramone Jul 17 '18 at 11:27
  • 1
    \$\begingroup\$ I disagree with your assessment of the conditional operator. Your proposed code is worse, for a number of reasons — but foremost because you’re assigning to highestWeightOfParcel twice, which I would immediately flag as code smell in a code review. Even though C# doesn’t generally support this, it simplifies code tremendously if you consider every variable as readonly. That’s why things like ?. and ?? were added to the language. \$\endgroup\$ – Konrad Rudolph Jul 17 '18 at 17:29
  • \$\begingroup\$ @KonradRudolph erm..you need to declare highestWeightOfParcel ahead of if block. to use it outside the if block. Null conditional operator (?./?[]) are to eliminate null checks and keep code thread safe. While null coalesce operator again is to be used in case of null checks. The if statement in sample code it not limited to null check only and thus disqualify these operators. As for ternary operator being unreadable, that is purely subjective so to each one his own. \$\endgroup\$ – danish Jul 18 '18 at 4:45
  • \$\begingroup\$ @danish You don't need to declare it beforehand, since you don't need to use it inside the conditional. In fact this comparison makes absolutely no sense here. Compare to the literal value. And use Math.Max instead. \$\endgroup\$ – Konrad Rudolph Jul 18 '18 at 6:46
0
\$\begingroup\$

Depending on the version of C# you are using...

collo?.WeightGrammes > 0 ? collo.WeightGrammes.Value : 0

?. is null propagation. If 'collo' is null, then 'collo?.X' is also null. null is not >0

If you are working your way through a list of values... then you can use linq

List<Collo> list= new List<Collo> (){... some values ... };
var max = list.Max ( ( c ) => c?.WeightGrammes ) ?? 0;
\$\endgroup\$
1
  • \$\begingroup\$ collo.WeightGrammes is of type int?. The first if is to check whether the property is null, not collo as a whole. collo?.WeigthGrammes could be replaced with collo.WeightGrammes ?? 0 though. \$\endgroup\$ – JAD Jul 17 '18 at 14:23

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