2
\$\begingroup\$

I am maintaining an application and have seen something like this

Permission p = new UserModel().GetPermission(userToTestPerrmission, permissionWeCheck);
if(p.CanChange)
   //sometihing
else
   //something else

This approach looks a little "dirty" to me. The part I have problems with is this new UserModel().GetPermission(). Is this a typical way to write something like this or should it be like:

var model = new UserModel();
model.GetPermissions(...);
...
\$\endgroup\$
5
\$\begingroup\$

Is this a typical way to write something like

If that’s all the information that is required – yes, why not? I see no problem at all with this approach.

On the other hand, if code lik this is prolific this is a sure sign that the original class design is broken. But from your code it looks like this is outside of your control anyway, since UserModel is a framework class.

In general, if a class is used in such a fashion then it is over-engineered: a static method could have been used just as well (on the other hand, this may make unit testing harder because it prevents mocking).

\$\endgroup\$
  • \$\begingroup\$ Nicely said. If a class is used for initialization and a method call, why is it a class in the first place? (of course, this is harder to obey in a purely OO language, which quite further proves that OOP is not a silver bullet) \$\endgroup\$ – Zirak Aug 8 '12 at 17:55
1
\$\begingroup\$

Sometimes this does stink. Consider the following:

byte[] data = { ... };

File.OpenWrite("filename.dat").Write(data, 0, data.Length);

File.OpenWrite implements IDisposable and therefore should be deterministically disposed as such:

byte[] data = { ... };

using (var stream = File.OpenWrite("filename.dat"))
{
    stream.Write(data, 0, data.Length);
}

So, keep an eye out.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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