Please Review My Coding Style

[zb0S216C]

There is no way I'd let one of my developers implement something like this

I'm not one of your developers. Everybody is different, everybody has their own coding style, everybody creates there own custom libraries to use in their projects. It's perfectly simple: I don't use Boost, and I never plan on using Boost.

Why would you not use an industry accepted standard container?

My version is far different than that of Boosts version of smart_ptr.

What makes you think your one is better?

I never said my version was better.

more stable? more tested?

I've already extensively tested the structure and all is well.

@JoR
I always separate decelerations from definitions in headers. I just used a source file for the original post's example.

P.S: Using NULL in the destructor is still valid( Source: Stack Overflow ).

[3hM2Nwbp]

I can see one reason for not using boost's pointers. Ever have a library that you were trying to use, but then had to link in library 1, library 2, library 3, library n-1, etc in order for it to work properly? Then come to find out in order to make it work you need library version 2.2.9a of library 2 or else it wouldn't work properly? If you're making a library and don't want to have any external dependencies (OGRE, Irrlicht, for example that have rolled their own smart pointers), then I could see rolling your own. That, or forcing another dependency on your end-user.

[Zaita]

@Framework

It's perfectly simple: I don't use Boost, and I never plan on using Boost.

This to be says a lot about you as a developer. The reality is that Boost is really a defacto standard for offering extended functionality to the C++ language. MANY MANY employment opportunities will require you to have knowledge of using Boost.

Many employers, and other developers, will look negatively on your work if you are re-implementing existing objects that are accepted as an industry defacto standard. The reasons for this are typically around cost, time and stability. It costs more and takes longer for a developer to re-write functionality and unit tests for objects that already exist in 3rd party libraries. Every new line of code you write into an application increases the chance of introducing a bug (or bugs).

I've already extensively tested the structure and all is well.

So, You've used unit tests with 100% code coverage? multi-platform and performance tests? memory leak analysis?

You asked for us to review your coding style. I've reviewed your coding style and given you feedback as requested. Whether you agree with this or not is up-to you. There is much more to consider when developing software than just spitting out code.


@Luc Lieber: This is completely inaccurate of Boost.
Boost's libraries for the most part are self-contained. Many of them do not even require linking, only the inclusion of a header file.

Some require linking to 1 or 2 boost libraries, and very very occasionally linking to a standard C++ library (e.g pthreads). It'd be appreciated that you keep your feedback to factual information.

You're argument about forcing dependencies on your end user is irreverent, unless you're only going to distribute the source code for your application. If you're distributing a binary then it doesn't matter how many dependencies your build system has.

Ogre for example uses boost. You get boost 1.44 when you download Ogre 1.7.2. You just never notice this because it's not packaged with the binary download, only with the source download.

[jsmith]

I use boost daily in my job, and I always ask candidates if they've heard of/used boost before.

[zb0S216C]

multi-platform

I only work with Microsoft Windows.

There is much more to consider when developing software than just spitting out code

If you're referring the the source code in the original post then you haven't been reading my posts. The code within the original post is an example source code. I never said review the code, only the style in which the code is written.

Many employers, and other developers, will look negatively on your work if you are re-implementing existing objects that are accepted as an industry defacto standard

Programming is a hobby of mine. I'm not looking for a job in programming.

[Zaita]

jsmith: Yea, I use boost daily in my job too; and all developers I hire must have knowledge of boost.

@framework: multi-platform can also refer to variants of Windows.

I never said review the code, only the style in which the code is written.

Firstly, you said review your coding style. Which covers everything from the actual layout of your braces through to how you solved the problem (or how the code was written).

You said don't worry about functionality, so I've not said whether you're code was right or wrong in it's application of the solution.

If you wan't people to review the way you write code, then you should've asked them to review the way you write code; or simply provide a better example of code for them to review that didn't have so many obvious flaws.

Some simple problems (IMO of course):
- Way too many comments
- Comments should be in a consistent format (e.g Doxygen compatible)
- Class declaration and implementation inline
- Constructor is ambiguous (this shouldn't really even compile)
- C I/O not C++ I/O
- No encapsulation
- Using a naked pointer
- Using a pointer to store a single value
- Poor naming convention for members and methods
- Poor naming - abbreviating names of variables (InitValue)

I suggest you take the advice many of the people have posted on this thread, instead of coming back with disputing comments. You have to remember that you did ask us for our opinion, and many of us are Snr level professional developers or software architects with many years experience in development.

I am happy to elaborate on any of my listed points above, but again remember that my comments, and everyone else's, are our own opinions.

[jsmith]

I just wrote a function similar to this, using four boost libraries:

#include <algorithm>
#include <boost/array.hpp>
#include <boost/lambda/lambda.hpp>
#include <boost/lexical_cast.hpp>
#include <boost/static_assert.hpp>
#include <string>

enum Fruit {
    Apple = 0,
    Banana = 1,
    Grape = 2,
    Pear = 3,
    NUM_FRUITS   // Must be last
};

struct FruitDesc {
    Fruit fruit;
    const char* name;
};

std::string fruit_as_string( Fruit f )
{
    BOOST_STATIC_ASSERT( NUM_FRUITS == 4 );

    typedef boost::array< FruitDesc, NUM_FRUITS > FruitList;

    FruitList fruit_names = { {
        { Apple, "Apple" }, { Banana, "Banana" }, { Grape, "Grape" },
        { Pear, "Pear" }
    } };

    FruitList::const_iterator i = std::find_if( fruit_names.begin(),
        fruit_names.end(), &boost::lambda::_1->*&FruitDesc::fruit  == f );

    return i != fruit_names.end() ? std::string( i->name ) :
        boost::lexical_cast<std::string>( f );
}

This function is very hard to get wrong because the logic of it will never change -- it is completely data driven. The worst that can happen is someone adds a new fruit after the NUM_FRUITS enum value, though the comment, placed right there, should be enough to ward off even the blindest of programmers. Second to worst is they add a new fruit but don't update the function -- then a compile error results. (The hardest part about ostreamers for user defined types is keeping them in sync with the data -- its easy to forget to update them after adding new data/values).

[PanGalactic]

@jsmith: The worst that will happen is that Boost introduces a bug or changes the interface to one of those libraries.

[ne555]

@jsmith: that looks way too overkill.

[jsmith]

@PanGalactic: unlikely, though there have been some interface changes in the past.

@ne555: how so?

[ne555]

//somewhere over the rainbow
std::string FruitList[NUM_FRUITS]; 
//hardcode initialization (ways to avoid it? maybe parsing the enum declaration)

std::string fruit_as_string( Fruit f ){
//BOOST_STATIC_ASSERT( NUM_FRUITS == 4 ); //I don't understand this limitation
//static const std::string FruitList[NUM_FRUITS] = {/*hardcode initialization*/};

  if( between(0, f, NUM_FRUITS-1) )
    return FruitList[ f ]; //O(1) 
  else 
    return boost::lexical_cast<std::string>( f );
}

Only if you don't corrupt the assignment in the enum (but why assign them in the first place?) however you could use a map O(log n).

[jsmith]

@ne555:

How would one go about parsing the enum declaration in a language that does not allow that kind of introspection?

They are assigned in the enum to help document the fact that the values do matter, because the value of NUM_FRUITS matters. Yes, a comment can be added there stating that as well. Though not all programmers read the comments anyway.

The BOOST_STATIC_ASSERT is there in the event that someone adds a new fruit to the enum and forgets to update this function. Since NUM_FRUITS would bump by one, the assert would trigger a compile error inside the function to be updated, where a comment would be placed stating what to do. This turns a runtime bug into a compile error, which is always a plus since compile errors are 100% reproducible.

While line 10 is constant time execution, it requires that the programmer maintain proper sorted order when updating the initialization of the array. Again, this would manifest itself as a runtime bug.

I could have used a map, but that would have required writing more code to populate the map. But it's unlikely that the enum would ever grow to a size where linear lookup performed noticeably slower than logarithmic or even constant time, and it's unlikely that this function would be called that often to notice the difference.

[ne555]

While line 10 is constant time execution, it requires that the programmer maintain proper sorted order when updating the initialization of the array. Again, this would manifest itself as a runtime bug.

std::string fruit_as_string( Fruit f ){
  void initialise(std::string []);
  static bool first_time=true;
  static std::string fruit_names[ NUM_FRUITS ];
  if(first_time){
    initialise(fruit_names);
    first_time=false;
  }
  //...
}

void initialise(std::string fruit_names[]){
  BOOST_STATIC_ASSERT( NUM_FRUITS == 4 );
  fruit_names[Apple] = "Apple";
  fruit_names[Pear] = "Pear";
  fruit_names[Grape] = "Grape";
  fruit_names[Banana] = "Banana";
}

But it's unlikely that the enum would ever grow to a size where linear lookup performed noticeably slower than logarithmic or even constant time, and it's unlikely that this function would be called that often to notice the difference.

I was thinking exactly the opposite.

Thanks for the assert explanation.

[jsmith]

I was thinking exactly the opposite.

That's because the example I gave is too general. In my specific case (which I cannot post), I know that it wouldn't grow that large to make a difference, and I also know that the function would only be called once during the lifetime of the process (at least for now -- in the future, it may be called more often for the purposes of debug output).

[rocketboy9000]

I only use comments when it's not obvious why I did what I did. Or when it's not obvious WHAT I did.

void draw_block_back(int f,int i,int j,int c){//implements typedef draw_part_of_block.
	float px1=perx[i];//perx and pery are a perspective lookup table.
	float px2=perx[i+1]-1;//border INSIDE space.
	float py1=pery[j];
	float py2=pery[j+1]-1;
	int c2=(c>>1)&0x7f7f7f;//make each component of color half as bright fasthack
	rect(buf,px1,py1,px2,py2,c2);
}

[PanGalactic]

@rocketboy9000: take a look at jsmith's coding style. All of the comments in your example become redundant with such a literate programming style. That's not to say that it's not necessary to comment a tricky piece of code. But if you can say it with constant and variable names rather than comments, do that.