check_snmp_raid.pl: code refactoring, clean up and some cosmetic work are needed

[giner]

1) some pieces of code are duplicated 2) tabs and spaces are mixed, it should be cleaned up (I think tabs should be replaced by spaces) 3) variable declaring should be vertical and not horizontal, for example: my { var1, var2, } instead of my {var1, var2}

[willixix]

1) If you see clearly duplicated code point it and maybe even fix it. But remember this is a script, it is not an object oriented program. So sometimes its easier to just duplicate (in fact that' what people have been doing to my code when adding support for battery check, etc). But the code use more refactoring and separating into functions, that's for sure. I don't have the time for that though, already done what I could when preparing 2.0 version, it was much worse before as core of the code was not originally mine. 2) I've replaced tabs with spaces now. Have got tired of that too. 3) I prefer vertical over horizontal too so most variables are declared vertical. But when there is a bunch of related variables being declared and initialized to undef it is easier and saves space to declare them on one line. Variable declaration was worse couple years ago too, I cleaned most of it already and added comments to option variables.

[giner]

But remember this is a script, it is not an object oriented program. So sometimes its easier to just duplicate

I would agree, it often make sence, but, for example, there is a converting "code into description" part that is repeated many times. When somebody (for example me) would like to add a new "check" into the script then much easier and faster to use "code into description" function instead of: 1) looking through the code and find what part of code do exactly what you want 2) comare your findings, choose one and adapt for your needs

But when there is a bunch of related variables being declared and initialized to undef it is easier and saves space to declare them on one line

"one line" way is more difficult to read and it is not diff friendly, that's why I would prefer 1 var -> 1 line

[willixix]

Feel free to add this function to plugin code and convert everywhere where there is this common code to using it. If it all looks clean, I'll be happy to take it.

In general these type of functions to make it easier to write new plugins is what is intended to have in Naglio library. But I've not yet added SNMP support in there yet and don't have enough time (at least two full weeks) to finish it for release. And this plugin actually is quite different then my other plugins, it probably will not use the library anyway.

BTW, you may not believe it but I don't actually use this plugin. I haven't setup any server that needed it in the last five years (from mid 2008) as I've been dealing with cloud systems. But there are people regularly sending me requests and patches so plugin continues to be maintained with regular code updates and improvements. However this depends more on those like you who are willing to contribute than on what I do myself.

In regards to variable declarations, the way it is now (after last update earlier today) is how I want it to stay. Like I said there are reasons to group these variables together. And I want to notice when new ones are added into their mix in patches I receive. So there are arguments on both sides for diff too.

[willixix]

I've done more code re-factoring yesterday, so code and logic should be cleaner now and yours and another similar function are now in use where appropriate, important variable's declarations are also properly commented.

Since there were important changes to core part of logical and physical check parts please get the latest version and verify it is working properly. As I mentioned, I don't use this plugin myself and can't test it. Also one example of nagios configuration and use for adaptec would be appreciate. Please verify the plugin is working and fix any issues you may still see as soon as possible next week - I want to be done with next version and release it by end of the month.

[giner]

I can test only adaptec (IBM ServeRAID). Seems to work for me.

[willixix]

I'm closing this issue as requested code improvements are complete and version 2.2 of check_snmp_raid is now released. Thank you for your help on this release.

[giner]

Great, thanks for your work.