Click here to Skip to main content
15,881,172 members
Please Sign up or sign in to vote.
3.75/5 (4 votes)
See more:
Hello everybody,

This is more like an aesthetical issue, than technical so maybe this is the wrong place. Sorry, if that's the case.

I'm writing a little application, where I've got this class:
C#
public class MyService : IService

        public IService OtherService { get; set; }

        public override byte[] Get()
        {
          
          ...

          var result = OtherService.Get();

          var data = result.ConvertToDifferentFormat();
         
          return data;
        }
}


Other components call only the Get function, which always need the OtherService property set and if somebody adds somewhere a new instance of this MyService, should notice that.

What is the best thing I can replace the triple dots with?

1. nothing - when somebody runs the program first time, they get NullReferenceException and notice OtherService is null

2. add
C#
if (Cache == null) throw new ArgumentNullException("property OtherService", "Property OtherService is null");
despite properties not being parameters.

3. create a new class PropertyNullException to copy the ArgumentNullException

4. something else :)


Thanks in advance!

EDIT:

I thought, that this is more theoretical question, so I rewrote the the code, which was a mistake. In my application I access remote image tiles (MapQuest map imagery) and it's nice to have these tiles also stored locally, so they deal with less traffic. This class should be a little API class, which handles caching for the remote tiles, and probably should be called the same way. That's why it needs a remote service, because without that, there's nothing to store.

Currently, I have this code:

C#
public class CachedTileService : TileService
    {

        public CachedTileService(TileCache cache, TileService remoteService)
        {
            Cache = cache;
            RemoteService = remoteService;
        }

        private TileCache cache;
        public TileCache Cache
        {
            get { return cache; }
            set
            {
                if (value == null)
                {
                    throw new InvalidOperationException("Cache property cannot be set to null");
                }
                else
                {
                    cache = value;
                }
            }
        }

        private TileService remoteService;
        public TileService RemoteService
        {
            get { return remoteService; }

            set
            {
                if(value == null)
                {
                    throw new InvalidOperationException("RemoteService property cannot be set to null");
                }
                else
                {
                    remoteService = value;
                }
            }
        }

        public override bool Get(int level, int x, int y, out byte[] data)
        {
            bool isCached = Cache.IsAvailable(level, x, y);

            if (isCached)
            {
                //we get the data from cache
                data = Cache.Get(level, x, y);
                return true;
            }
            else
            {
                //we try to get data from the remote service
                if(RemoteService.Get(level, x, y, out data))
                {
                    //if we succeed we store it in the cache
                    Cache.Put(level, x, y, data);
                    return true;
                }
                else
                {
                    //if we didn't get remote data, we return false
                    return false;
                }
            }
        }
    }


The cache is not a real cache so far - it just stores the tiles to different files.

Actually, I didn't expect so many detailed and awesome answers and thank you all of you. I hope, you don't feel as you have wasted time for somebody, who can't even write a clear question.

Thank you again :) .

What I have tried:

I'm using the first one - no checking at all - but it just doesn't feel right. I like the third approach, but this seems to be more common and I think I'm missing some feature in C# that handles this, but I haven't found it, yet. :)
Posted
Updated 29-Apr-16 7:41am
v3
Comments
Philippe Mori 29-Apr-16 12:42pm    
For #4, assertion might be used to help debugging as you could detect the error before an exception is thrown which might allows you to use EnC (edit and continue) or immediate windows to try a fix. Thus it can help to loose less time fixing errors while not having extra impact at run time and it also make clear expected conditions. In many cases since an exception is raises anyway if you access a null pointer, raising our own exception does not gave much benefit but sometime that might be useful to make it easier to know the exact problem particulary if you do libraries for others without source code.

Debug.Assert(OtherService != null)
macika123 29-Apr-16 13:48pm    
Thank you!
Philippe Mori 29-Apr-16 14:25pm    
If you have a constructor that take service and pointer, it would be preferable to remove corresponding public properties (particulary public set access) if you don't need it (and require those to be not null in constructor).

If it is a valid state for OtherService to be null then you should throw an InvalidOperationException in Get() if it is null.

If it is not a valid state for OtherService to be null then you shouldn't let it come to that in the first place: Change the property from an auto-property to an explicitly implemented one and throw an InvalidOperationException when it is attempted to set OtherService to null (and require it to be set through the constructor as well).

Whether or not it is a valid state for OtherService to be null can mostly be determined by asking if there's anything the class can still do if it is null: If not then it probably shouldn't be a valid state. If yes then consider the possibility that your class violates the single-responsibility design principle (not neccessarily but give it a thought).
 
Share this answer
 
v2
Comments
macika123 28-Apr-16 18:14pm    
Thank you! :)
Sascha Lefèvre 28-Apr-16 18:39pm    
You're welcome! :)
macika123 28-Apr-16 19:54pm    
As a current-final solution, I replaced the default constructor with a parametrised one - I must set the OtherService object when MyService is created.

Also replaced the auto-property with guarded ones, which throw InvalidOperationException when somebody tries to set them null.

Thanks again for your response :) .
Sergey Alexandrovich Kryukov 28-Apr-16 18:25pm    
Not sure. What throwing of InvalidOperationException should add? If the null is dereferenced, null exception will be thrown, and it's not less informative than InvalidOperationException.

I think that the problem does not have universal solution, and it should not have it. Please see my Solution 2 for some thoughts.

—SA
Sascha Lefèvre 28-Apr-16 18:59pm    
I agree that throwing an InvalidOpEx wouldn't add a whole lot of value but, (as it appears to be an API implementation) it would allow to provide a to-the-point error description, as a NullRefEx from within an API-method is somewhat unexpected, IMO.

But, actually, I'm currently implementing an IDataReader-class and asking myself if I really should check all the required conditions within the GetXXX(..)-methods, as it will certainly noticeably impact performance and an exception will be thrown for most fault-cases anyway along the way (array-bounds etc.), just not very descriptive. So I can see how it can be a design-consideration whether or not to do it, but if performance isn't a concern I would always do it for clarity.

Sascha
It all makes no sense at all. Your code sample won't even compile, by apparent reasons.

The question "What is the best thing I can replace the triple dots with?" makes no sense, because you should not write any "triple dot" at all. :-) It sounds like a joke, but this is not quite a joke. It should tell you that the question is purely artificial and cannot be correct. Instead, you should explain your goal. Perhaps, you meant to discuss how to deal with the situations when OtherService is null at the time of calling Get(). Note that the lack of parameters in Get() suggests that you should rather have some property doing the same stuff.

At the same time, there is some serious background behind this piece of code.

One problem of modern programming is that many students have been taught to use defensive programming. In fact, the principle is very questionable and should not be used without critical thinking. You can find a lot of products where some condition is "over-guarded": people check up the validity of data both at the call and in the method implementation. So, your first approach has its reasons.

There are languages where declaration of preconditions and post-conditions is an integral part of the method declarations. Such languages did not get popularity. I think one the reason is that the static constraints may resemble the use of magic numbers, and the check of dynamic constraints should be a part of the method code anyway.

I think there is no an ambiguous solution of this problem, cannot be, and, essentially, the solution is not very important. After all, it all boils down to the solutions made by a developer of the type of the library; depends on what this person has between ears. Universal cookbook recipes hardly can help. In all cases, thorough check up in all possible preconditions in the method would be bet. The "garbage in, garbage out" principle still looks very reasonable. Moreover, presence of structural exception handling eliminates difference between just invalid parameters and parameters causing exception. One important principle is not to handle exceptions too locally.

The presence or absence of a condition check does is not the only set of alternatives you have. Another option is to guarantee that your OtherService is never null. This is really simple. This is the property. You can set some default object in the only constructor, and prevent setting null. Such approaches are related to the programming concept of invariant.

And so on…

—SA
 
Share this answer
 
Comments
Sascha Lefèvre 28-Apr-16 19:09pm    
> "Note that the lack of parameters in Get() suggests that you should rather have some property doing the same stuff."

I'm not sure whether I got this from the Microsoft C# style guide or from somewehere else: Properties should return rather quickly, for heavier computation methods should be used. As it's basically irrelevant if there's a getter-only property or a method (if databinding isn't a goal) I think this is an intuitive design guideline.

> "presence of structural exception handling eliminates difference between just invalid parameters and parameters causing exception."

I don't quite get it - can you please elaborate?

Sascha
Sergey Alexandrovich Kryukov 28-Apr-16 19:54pm    
I totally disagree with this advice. Property is nothing but a set of 1 to 2 methods with the syntax of assignment and/or reading. I'm sure you know it and agree. That it, it's a pure syntactic sugar. There is no a grain of rationale behind the notion of "quick"; the only "rationale" I can see is the old habits of some people. Any rational objections? :-)

Here is what I think: if the choice between method and property is technically possible, as in this case, the choice should be done on purely semantic considerations. Roughly speaking, if it is possible and if you can find a pretty much self-explaining word which can be naturally understand as the property of the type's instance (or the type itself, for static properties), it should be used. Look at this case: the verb "Get" provokes the question "get what?!". That is, it calls for a noun. If the developer can understand what should be that noun, the noun should be used.

Is, I'll try to elaborate on "elimination". In obsolete technologies, some garbage-in data produced garbage on output, or crash of the whole application. Huge difference which calls for safety checks of all precondition. With exception handling, there is no crash (in non-nonsense software). It works like a time machine: you end up to the situation as if the method was never called, with, hopefully, explanation of what was wrong. Much less difference; now you can consider this condition as a kind of garbage. Note that it's important not to handle exceptions locally, but handle them in the points which I call "points of competence", points where the exception can be analyzed, explained, condition corrected, and so on...

—SA
macika123 28-Apr-16 19:22pm    
Thank you!

I'm quite guilty for overthinking stuff and yet forgetting about smarter solutions. Your solution clarified lots of things and I really appreciate it :) .
Sergey Alexandrovich Kryukov 28-Apr-16 19:55pm    
You are very welcome. Please, don't be guilty. "Overthinking" is the door to rational and productive thinking; I really appreciate it.
—SA
BillWoodruff 29-Apr-16 3:50am    
+5 Please see my initial comments in my post below. I'll appreciate any response.
Since everyone has provided their views on this topic, I want to give mine too. The nicest way has not yet been discovered, once it has been, I will let you know. Until then, I would try to provide you with a "good" way of checking if the objects exist, or they don't. Well, if the objects don't exist, they don't and there is no way that the user would be able to create them. Typically, these things (as Sergey said in their answer) are managed by the developer to ensure that the programs don't crash unexpectedly*.

My recommendation simply is, to do this:
C#
public IService OtherService { get; set; }
 
        public override byte[] Get()
        {
          if(OtherService == null) { return null; }

          var result = OtherService.DoStuff();
          var data = result.ConvertToDifferentFormat();
          return data;
        }

I understand that the null is still being returned but that is because returning an empty byte array is not similar to returning a problem. Let the error go back to the caller. Then, on that thread, where you are not returning (because either you return something, or you throw exception), you can show the message. Something like:
C#
if(obj.Get() == null) {
    // Show message
} else {
    // Work!
}

Remember, user won't be able to work around here, because you already have compiled the binaries and you are going to give him with an application. What you can do is, ensure that the program logic is fulfilled. The program runs through the qualified paths to ensure that either error message is shown if the service cannot be instantiated, or the program runs.

Since C# 6 has a new feature for null check, and people are loving it too, I want to warn you that there is no benefit of that either; because that resolves down to what I shared above. For more on that and other features of C# 6, please read this article of mine: Experimenting with C# 6’s new features[^]

* There is a difference between programs crashing unexpectedly and expectedly, on the consent of the programmer.
 
Share this answer
 
Comments
macika123 28-Apr-16 20:21pm    
Thank you!

I'm quite astonished, that I got three answers ( so far :) ).

To be honest, I've bad memories about doing this, because of one of the projects I worked on (but that's because that project was nightmare anyway ...).
When we got NullReferenceEx, we ended up craving through multiple function calls, because every function returned null, when it has some null variable in them. Again, it was probably the projects fault and in your answer it looks much more cleaner :)
Afzaal Ahmad Zeeshan 30-Apr-16 15:23pm    
You're most welcome!
BillWoodruff 29-Apr-16 3:51am    
+5 please see my initial comments in my post below.

I'll appreciate any response.
Afzaal Ahmad Zeeshan 30-Apr-16 15:22pm    
Thank you, Bill. I will definitely give it a look. :-)
Sergey Alexandrovich Kryukov 29-Apr-16 8:40am    
That's another way worth mentioning, a 5.
—SA
+5 for the question: I think this a very interesting question, and the discussions in the responses fascinating.

First, as noted here by Sascha L.: if the use of 'Get in 'MyClass absolutely depends on 'OtherClass being a ready-to-go instance, then: test for that immediately in the 'Get method, and throw an error if it's not usable !

The code I show here takes that one step further by making the 'GetBytes method (my name for your 'Get method) the only public method that can be called on the class. Note that to do that, assuming 'DoStuff, and 'ConvertToFormat are defined in IService, you'll need to use explicit interface implementation so those methods are hidden from external consumers of your code ... unless they cast their instance to an IService, of course.

I'd like to add my own ideas here, but, first, I'd like to ask you:

1. 'MyClass, instance of 'IService, contains a reference to an instance of 'IService named 'OtherClass, and you invoke its method 'DoStuff ... but, your instance 'MyClass defines no 'DoStuff method:

Therefore, your IService interface does not declare 'DoStuff, since, if it did, 'MyClass would also have to define 'DoStuff.

But, that's not possible because you call 'DoStuff on an instance of IService.

The simplest explanation for this apparent contradiction would be that you simply omitted defining 'DoStuff in the code for 'MyClass that you show here ... if that's correct, then why not edit your original post, and show the full code for MyClass, and show the IService interface code.

Now, let me see if I can add something to this thread by a code example (that will compile, and run as is but is not fully tested:
C#
using System;
using System.Collections.Generic;
using System.Linq;

namespace QuestForNice // revision #1
{
    public enum ServiceStates
    {
       Undefined,
       Valid,
       DataEmpty,
       OtherServiceUnusable,
       DataAccessError,
       DataTransformError
    }

    public interface IService
    {
        ServiceStates ServiceState { get; }

        IEnumerable<byte> Data { get; }

        IService OtherService { get; }

        ServiceStates GetBytes(ref byte[] bytes);

        IEnumerable<byte> DoStuff(ref IEnumerable<byte> bytes);

        IEnumerable<byte> ConvertToDifferentFormat(ref IEnumerable<byte> bytes);
    }

    public class Service : IService
    {
        private ServiceStates _serviceState;
        public ServiceStates ServiceState {
            get { return _serviceState; }
        }

        private IEnumerable<byte> _data;
        public IEnumerable<byte> Data{
            get { return _data; }
        }

        private IService _otherService;
        public IService OtherService{
            get { return _otherService; }
        }

        public Service(Service otherservice = null)
        {
            // assume no result
            _serviceState = ServiceStates.Undefined;

            if (otherservice != null)
            {
                _otherService = otherservice;
                _serviceState = ServiceStates.Valid;
            }
            else
            {
                _serviceState = ServiceStates.OtherServiceUnusable;
            }
        }

        public ServiceStates GetBytes(ref byte[] bytes)
        {
            if (bytes.Length == 0) return ServiceStates.DataEmpty;

            if (_serviceState == ServiceStates.OtherServiceUnusable)
            {
                return ServiceStates.OtherServiceUnusable;
            }

            // byte[] => IEnumerable<byte>
            IEnumerable<byte> _data = bytes as IEnumerable<byte>;

            // 'DoStuff works ?
            try
            {
                _data = OtherService.DoStuff(ref _data);
            }
            catch (Exception)
            {
                {
                    return ServiceStates.DataAccessError;
                    // leave the passed-in byte[] as it is here ?
                }
            }

            // data is null now ?
            if (_data == null)
            {
                return _serviceState = ServiceStates.DataEmpty;
            }

            // conversion okay ?
            try
            {
                // note required cast to IService here
                _data = (this as IService).ConvertToDifferentFormat(ref _data);
            }
            catch (Exception)
            {
                {
                    return ServiceStates.DataTransformError;
                    // leave the passed-in byte[] as it is here ?
                }
            }

            // data is null now ?
            if (_data == null)
            {
                return ServiceStates.DataEmpty;
            }

            // IEnumerable<byte> => byte[]
            bytes = _data.ToArray();
            return ServiceStates.Valid;
        }

        // explicit implementation allows these methods
        // to be restricted to use in-class
        IEnumerable<byte> IService.DoStuff(ref IEnumerable<byte> bytes)
        {
            // do whatever

            return bytes.Select((byt, ndx) => (ndx % 2 == 0) ? (byte)((char)(byt + 32)) : byt);
        }

        IEnumerable<byte> IService.ConvertToDifferentFormat(ref IEnumerable<byte> bytes)
        {
            //do whatever

            return bytes;
        }
    }
}
Here's a sample test-case:
C#
using System;
using System.Collections.Generic;
using System.Text;
using QuestForNice;

private Service Service1;
private Service Service2;
private byte[] servicetest;

private void TestQuestForNice(object sender, EventArgs e)
{
    Service2 = new Service();
    Service1 = new Service(Service2);
     
    servicetest = Encoding.ASCII.GetBytes("ABCDEFGHIJKLMNOPQRSTUVWXYZ");
    
    ServiceStates testresult = Service1.GetBytes(ref servicetest);
    
    Console.Write("test complete, status: {0}", testresult);
    
    if (testresult == ServiceStates.Valid)
    {
        string result = System.Text.Encoding.Default.GetString(servicetest);
    
        Console.WriteLine(" result value: {0}", result);
    }
}
The outputL:
C#
test complete, status: Valid result value: aBcDeFgHiJkLmNoPqRsTuVwXyZ
Notes:

0. remember that the code shown here, the transform function, etc. is meant to serve only as an example of a certain strategy, and, hopefully, another p.o.v. in the discussion, here; you should never take code like this and put it into production without extensive analysis, testing, etc.

1. the degree (the strength) to which you try to implement systematic error-trapping, and detailed result logging, as shown here, should reflect a reasonable analysis of how you might expect current and future consumers of your code will expect, want, wish for, your code to "behave." Don't put on a suit of armor if your opponents are always mere ants :)

2. in this example, the test data-transform function is hard-wired into the 'DoStuff method; I doubt that's what you will really want, and, I'd re-work this to make that method a delegate that can be set at run-time by a consumer of the code (or fallback on a default implementation, if it is not set, externally).

3. finally, I anticipate re-writing some other code I am using now, based on some ideas in this code experiment, and, will revise this code if I have have more ideas. On first "smell," I think this code may be a little too clever.

4. note the rather elaborate lengths gone to here ... making the interface properties implementation 'readonly ('get only) ... to prevent the external user from setting them. Over-kill ? Maybe, depend on context.

5. To my mind, your use of the function name 'ConvertToDifferentFormat implies some kind of possible type-conversion, and that makes me think that you will need to use generics, here.
 
Share this answer
 
v6
Comments
macika123 29-Apr-16 13:39pm    
Thank you for the nice answer! I really like the ServiceState enum. It's interesting that I use enums in different parts of the application, but forgot about, that I can use them here too. It looks cool in your code :) .

You were right, I should have added the original code or at least write an appropriate code snippet. I also modified the original question, which now has the original code, too.
Afzaal Ahmad Zeeshan 1-May-16 3:54am    
Your method is also interesting, using the enumerations to check if everything is going great or not. +5.

However, checking the null can be much simpler, than checking against an error type in the list. :-)
BillWoodruff 1-May-16 4:07am    
Hi Afzaal, I appreciate your vote.

My answer does assume that there is a valid reason for getting a "detailed" status report after the code is run, rather just null, or non-null. But, you're right, in many cases that may not be required.

I am "influenced," I believe, by .NET's many 'TryParse/'TryGetValue style methods that deal with a value parameter passed by reference and return a boolean result ... in this case I've extended that idea to returning an Enum result.

Whether or not this technique is appropriate, or useful, depends, as usual, on context, requirements, who the end-users are.

cheers, Bill
Afzaal Ahmad Zeeshan 1-May-16 10:44am    
Definitely, the context really does matter in many cases and the way application is designed. :-)

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900