A question of style and design

[Kumosan]

Hi,
I'd like to hear some opinions. Usually I only use Linux and the gcc. And I always make sure that it compiles cleanly. So does the code below. Now I wanted to try to compile my program with Visual Studio 2005. The compiler gave me the following warning:

'ImageScaler::scaleFactor' : not all control paths return a value

In principle the compiler is right, but I wonder if this counts as bad design. The control path Visual Studio complained about can never be executed. Of course, when I compile as release, the assertion will go away and this might open the undefined path.

I just wonder why this piece of code is ok for the gcc.

double ImageScaler::scaleFactor(RubberBands::lineType type) const{
  switch(type){
  case RubberBands::horizontal:
	 return _scaleFactorX;
  case RubberBands::vertical:
	 return _scaleFactorY;
  default:
	 assert(false);
  }
}

double ImageScaler::scaleFactor(RubberBands::lineType type) const{
  switch(type){
  case RubberBands::horizontal:
	 return _scaleFactorX;
  case RubberBands::vertical:
	 return _scaleFactorY;
  default:
	 assert(false);
  }
}

To copy to clipboard, switch view to plain text mode 

[high_flyer]

but I wonder if this counts as bad design.

I would not go as far as calling it bad design.
You can add the following, which should apease both compilers:

deefalut: assert(false);
                return 0.0; //or -1.0 or what have you that means error

deefalut: assert(false);
                return 0.0; //or -1.0 or what have you that means error

To copy to clipboard, switch view to plain text mode 

[Kumosan]

I would not go as far as calling it bad design.
You can add the following, which should apease both compilers:

deefalut: assert(false);
                return 0.0; //or -1.0 or what have you that means error

deefalut: assert(false);
                return 0.0; //or -1.0 or what have you that means error

To copy to clipboard, switch view to plain text mode 

True, thought of that. However, if I 'invent' a dummy error value good design would demand that I test for it, but I just tested what happens when I change the assertion to 'assert(true)'. When I did this I got the same warning the Visual Studio compiler gave me. So the gcc really seems to see that this path cannot be executed. In that case it probably would remove the dead code. I'd say it is pointless to add code, which the compiler removes and test for an impossible return value.

Too bad I have to do it anyways. It does not compile as release (-Werror) with NDEBUG defined.

[high_flyer]

I'd say it is pointless to add code, which the compiler removes and test for an impossible return value.

Why impossible?
Its very possible to call ImageScaler::scaleFactor(RubberBands::lineType type) with a type that is not handled in the any of the 'case' segments.
And the error return value doesn't have to be a dummy one, just defind it as such - you don't have to use it - but it'll never the less be defined

[wysota]

If you only want to distinguish between horizontal and vertical, you can use the following:

double ImageScaler::scaleFactor(RubberBands::lineType type) const{
  return type == RubberBands::horizontal ? _scaleFactorX : _scaleFactorY;
}

double ImageScaler::scaleFactor(RubberBands::lineType type) const{
  return type == RubberBands::horizontal ? _scaleFactorX : _scaleFactorY;
}

To copy to clipboard, switch view to plain text mode 

Without any compiler warnings

[Kumosan]

True, but this would not work anymore should there ever appear the need for a third value, e.g. if I go 3 dimensional.

[wysota]

You can always modify the function later. You can achieve the same by returning _scaleFactorY from a "default" case in your switch statement.

[Kumosan]

You can always modify the function later.

This of course is always true.

You can achieve the same by returning _scaleFactorY from a "default" case in your switch statement.

This would not help at all. I am 100% sure that this method won't be called with a wrong parameter in my code. However, blind faith like this is always an invitation to disaster and I have been wrong before (one or two times ). If I am willing to accept the risk, I would not have to do anything. If I am not willing to risk it, your code is probably worse than mine, because it potentially obfuscates bugs. A wrong parameter would translate to _scaleFactorY, which probably would be within a plausible range, but wrong. I would not want to find the random bugs, which could result from that.

The way I see it now it that if I want to be 100% correct I'd have to remove the assertion and replace it with a permanent runtime check.