Is it good practice to wrap trivial functions in wrappers?

Question

I'm doing a code review and I've noticed that the developer has done this:

UserSession.LocationId = CheckInteger(elementValue);

with this wrapper

    private int CheckInteger(string elementValue)
    {
        int outNumber;
        int.TryParse(elementValue, out outNumber);
        return outNumber;
    }

I can't see that this brings very much to the party. Should I push back, or just let sleeping dogs lie? I don't think there's any particular company policy that covers this.

depends, could having an invalid `elementValue` being turned into null be a bad thing or is it something that should be handled better? — user1666620, Sep 10 '15 at 13:22
The appropriate site for such questions is http://codereview.stackexchange.com — Panagiotis Kanavos, Sep 10 '15 at 13:23
Personally, I'd rather they do this: `int.TryParse(elementValue, UserSession.LocationId)` unless the rules for checking integers are likely to change. — Daniel, Sep 10 '15 at 13:23
If it repeats a lot, maybe it would be better to move it to a extension methods. But it's ok to keep with that — Thiago Custodio, Sep 10 '15 at 13:24
@PanagiotisKanavos No, it isn't. Not at all. That's not the OP's code, for a start. It's also just a tiny code snippet. Completely off-topic for Code Review. — itsbruce, Sep 10 '15 at 13:25
@itsbruce they're the maintainer of the code. However, I think this question might be too opinion-based even for CR. There's also very little context here. CR loves context. — Simon Forsberg, Sep 10 '15 at 13:26
i think http://programmers.stackexchange.com is where this question should really be — user1666620, Sep 10 '15 at 13:27
@DanielCook if `LocationId` is a property (as it should be) then you cannot pass it as an `out` parameter. — juharr, Sep 10 '15 at 14:08
@juharr Good to know. Unfortunately, my work environment is fairly anti-properties(for no good reason), so I'd never had an occasion to experience that. I just tested this in VB.Net and it certainly works with properties. However... C# may be throwing you into the pit of success. :-) Cheers — Daniel, Sep 10 '15 at 14:17
As an interesting note, [Jon Skeet recommended a helper function like this in an answer](http://stackoverflow.com/a/1370256/1316573), but his was static. — Daniel, Sep 10 '15 at 14:21
@user1666620 when referring other sites, it is often helpful to point that [cross-posting is frowned upon](http://meta.stackexchange.com/tags/cross-posting/info) — gnat, Sep 10 '15 at 15:52

Panagiotis Kanavos · Answer 1 · 2015-09-10T13:31:53.590

Are they sleeping dogs or wolves? This code effectively discards non-integer values and returns 0 for failed cases.

This may or may not be appropriate, but the name certainly is misleading - no check is performed at all. Errors are simply discarded. It's almost equivalent to hiding exceptions.

This can hurt you when invalid values aren't detected and 0s are set in unexpected places.

Such a method should at least be renamed to GetIntegerOrDefault so it's explicit that it will return a default value on error.

You should also ensure that 0 is a valid value for LocationId or at least an expected default. Otherwise you may encounter some very hard to diagnose bugs.

I've encountered bugs in a Stock Exchange application that send portfolio performance ratios of -100%, because someone 10 levels bellow decided to return 0 on invalid exchange rate values, and nobody remembered 5 years later.

score -1 · Answer 2 · answered Sep 10 '15 at 13:25

-1

Given that you could just, in this case, write int.TryParse(elementValue, out UserSession.LocationId) I would say this is not a good idea.

Now, if you needed to add logging to it or deal with special cases/errors in a consistent way, then sure go for it.

answered Sep 10 '15 at 13:25

Tom

796
7
7

2

`LocationId` looks like a property, and I don't think you can pass a property as an `out` parameter. – Rawling Sep 10 '15 at 13:27
Good point... I just tested it and for properties no you cannot. Member vars you can, but `A property, indexer or dynamic member access may not be passed as an out or ref parameter` – Tom Sep 10 '15 at 13:37

Is it good practice to wrap trivial functions in wrappers?

2 Answers2