3
\$\begingroup\$

I recently wrote a really long one-liner, it was really just for fun, but I would like to know if it's bad practice or not. Can you tell me?

Here's my full class, but it's about the ToString() method.

namespace HGroupBadge.Classes
{
    public class BadgeDetail : BadgePart
    {
        public DetailType Type { get; set; }

        public BadgeDetail(DetailType type, BadgeColor color, BadgePosition position)
        {
            Type = type;
            Color = color;
            Position = position;
        }

        public override string ToString() 
            => ((int)Type > 99 ? "t" : "s") 
            + ((int)Type > 99 ? ((int)Type).ToString().Remove(0, 1) : ((int)Type < 10 ? "0" + (int)Type : "" + (int)Type)) 
            + ((int)Color < 10 ? "0" + (int)Color : "" + (int)Color) 
            + (int)Position;
    }
}

It has to return a string similar to s01137, when Type or Color are lower than 10, a leading 0 has to be added because the final string always has to end in 5 digits. When Type is bigger than 99, the first digit has to be cut off. In the end, Position has to simply be added to the string.

DetailType, BadgeColor and BadgePosition are enums.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Welcome to Code Review! I'd be great if you could tell us more about your class/methd. What does it exactly do? You can also add some usage example. \$\endgroup\$
    – t3chb0t
    Commented Feb 17, 2017 at 20:25
  • 1
    \$\begingroup\$ @t3chb0t Added some description, hope it helps. \$\endgroup\$
    – Metoniem
    Commented Feb 17, 2017 at 20:30
  • \$\begingroup\$ @DainIIIronfoot The only reason I didn't use any intermediate variables is because it would end up being quite a few, which looks really.. long \$\endgroup\$
    – Metoniem
    Commented Feb 17, 2017 at 20:31

1 Answer 1

1
\$\begingroup\$
public override string ToString() 
    => ((int)Type > 99 ? "t" : "s") 
    + ((int)Type > 99 ? ((int)Type).ToString().Remove(0, 1) : ((int)Type < 10 ? "0" + (int)Type : "" + (int)Type)) 
    + ((int)Color < 10 ? "0" + (int)Color : "" + (int)Color) 
    + (int)Position;

You definitely should split it in a exactly four methods/variables:

Method 1: GetPrefix

((int)Type > 99 ? "t" : "s")

Method 2: FormatSomething, sorry, I don't know what.

((int)Type > 99 ? ((int)Type).ToString().Remove(0, 1) : ((int)Type < 10 ? "0" +  (int)Type : "" + (int)Type)) 

Method 3: FormatColor:

((int)Color < 10 ? "0" + (int)Color : "" + (int)Color)

Method 4: FormatPosition - this might be unnecessary but because all other formattings are method let's make this consistant.

(int)Position

Finally your ToString would look like this:

public override ToString() => 
    $"{GetPrefix()}{FormatSomething()}{FormatColor()}{FormatPosition()}"

and nobody asks any more questions about what your method is doing.

\$\endgroup\$
4
  • \$\begingroup\$ Upvoted (for as far as I can do that with this reputation) and accepted. Thanks alot! Here's what my code looks like now: http://ã.co/i/2vf6bk.png and in case you're interested, here's my documentation on how this string is supposed to be constructed: github.com/notmika/group-badges#badge-structure \$\endgroup\$
    – Metoniem
    Commented Feb 17, 2017 at 21:08
  • \$\begingroup\$ Oh yay, IDN domain names are not supported on stackexchange :[ \$\endgroup\$
    – Metoniem
    Commented Feb 17, 2017 at 21:09
  • 1
    \$\begingroup\$ @Metoniem perfect :-) oh, you should also do something about the magic numbers: 99 and 10. If they have any meaning, make them constants. \$\endgroup\$
    – t3chb0t
    Commented Feb 17, 2017 at 21:11
  • \$\begingroup\$ Good point. Will do! \$\endgroup\$
    – Metoniem
    Commented Feb 17, 2017 at 23:44

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.