Just for fun...

Spire_Jeff

I was showing some code to a programmer-in-training within our company and he was amazed at this statement:

FOR(nFB_CNTR = 1;nFB_CNTR<=MAX_NUMBER_OF_ROOMS; nFB_CNTR++){
	IF(uROOM[nFB_CNTR].nLIGHT_ID && uLIGHT_BUTTON[uROOM[nFB_CNTR].nLIGHT_ID][1].nIS_ACTIVE)
		[dvTP_LIGHT, (100+nFB_CNTR)] = nHWI_BUTTONS[uLIGHT_BUTTON[uROOM[nFB_CNTR].nLIGHT_ID][1].nLINK][uLIGHT_BUTTON[uROOM[nFB_CNTR].nLIGHT_ID][1].nKEYPAD][uLIGHT_BUTTON[uROOM[nFB_CNTR].nLIGHT_ID][1].nBUTTON];
}

I tried to explain that such a statement is not that uncommon in large systems, but he still thinks I'm doing something crazy. So, for fun, I figured I'd post to see if anyone has anyone other lines of code they'd like to post that are as complicated in appearance as this one

Here is the English description of the above bit of code: Make the light graphics on the touch panels indicate the current status of the lights in each room.

Jeff

mpullin

This was the best I could come up with

arrXM_FAVS[SESSION[GET_LAST(arrTP)].USER][GET_LAST(butXM_FAVS)] = TYPE_CAST(arrTUNER_INFO[arrUSERS[SESSION[GET_LAST(arrTP)].USER].SEL_SRC].fTUNE)
arrXM_FAVS_TXT[SESSION[GET_LAST(arrTP)].USER][GET_LAST(butXM_FAVS)] = REMOVE_TRAILING_SPACES(arrTUNER_INFO[arrUSERS[SESSION[GET_LAST(arrTP)].USER].SEL_SRC].XM_STATION)
SEND_COMMAND BUTTON.INPUT.DEVICE, "'^BMF-',itoa(GET_LAST(butXM_FAVS)+butXM_FAVS[1]-1),',1&2,%PXM',itoa(arrXM_FAVS[SESSION[GET_LAST(arrTP)].USER][GET_LAST(butXM_FAVS)]),'.png%T',arrXM_FAVS_TXT[SESSION[GET_LAST(arrTP)].USER][GET_LAST(butXM_FAVS)]"

yuri

i don't do big lines of code, i tend to loose track of what i've just created

ericmedley

I'm an all caps guy because I'm so old-skool. This one's not so big, but I couldn't find any more silly ones right off the top of my head.

IF(LENGTH_STRING(COMCAST_CHANNEL_NUMBER[500+CABLE_MUSIC_X+CABLE_MUSIC_OFFSET]))
	{
	SEND_COMMAND dev_TP_Master_3 ,"  'TEXT',ITOA(CABLE_MUSIC_X+1400),'-',COMCAST_CHANNEL_NAME[500+CABLE_MUSIC_X+CABLE_MUSIC_OFFSET],$0D,$0A,COMCAST_CHANNEL_NUMBER[500+CABLE_MUSIC_X+CABLE_MUSIC_OFFSET]"
	}

DHawthorne

I agree, such constructions are not at all unusual once you get to a certain size project. I'm sure if I hunted around I could find one more convoluted than this, but here we go:

	    nVolume[nControlledZone_TP[AUDIO_MODE][nTP_Count]][1] = (nVauxVolume[nControlledZone_TP[AUDIO_MODE][nTP_Count]][1] + 
		nVauxVolume[nControlledZone_TP[AUDIO_MODE][nTP_Count]][2]) / 2

Big, data-packed arrays and structures are simply a must if you want your system to be modular enough that panels and zones can be changed later in the project's life. I recently had a customer with a 40 zone multi-room audio system go from 8 audio sources to 12 ... if everything wasn't arrays and structures, that would have been a nightmare upgrade. As it was, just a bigger dimension on a few arrays and new index constants for readability, and I was done.

ericmedley

DHawthorne wrote:

I agree, such constructions are not at all unusual once you get to a certain size project.

that's true for sure. I have a system with 20 TPs with as many audio zones. Each zone has up to 15 audio sources. For the devices with storable presets (AM/FM/XM/Siruis, etc...) each panel has 10 presets per source that are user assignable) The presets store channel name/number/genre. Each panel has its own set of presets. I store all the presets in one structure. You have to have those long, convoluted statements to get it done.

It reminds me of the days when I was working on Neve and SSL recording consoles. Newbees would stare at them in horror noting that it had over a thousand buttons and knobs. I'd try to calm them down and show them that it's easy if you break it down by the channel.

jjames

And how big are these systems in question with statements like these? Even in a 20 panel / 36 audio zone, 20 source system - you'd never see such long statements in my code. I am not saying my code is better - but the question is, why would you? Isn't there a "cleaner" way?

Here were the two longest statements with two systems I just skimmed through:

FOR(nLOOP2 = 1;nLOOP2<=MAX_LITE_BTNS[nLOOP][nLITE_PAGE[nLOOP]];nLOOP2++)// Loop through lighting btns
	{
		[dv_TP[nLOOP],LIGHTING_BTNS[nLOOP2]] = nLITE_MAP[nLIGHTING_MAP[nLOOP][nLITE_PAGE[nLOOP]][nLOOP2]][3]
	}

and...

[dv_TP,nXM_FAV_BTNS[nLOOP]]=CHAN_NUM[nNAV[nLOOP+((nSUB_PAGE[nPAGE]*nXM_TXT_LENGTH)-nXM_TXT_LENGTH)]][3]

I should note that the second code posted was written quite some time ago (several years) and I only keep this because it works. If i were re-writing it (which I may) I would do it differently.

I just like to keep things simple, I hate having to scroll to view a statement. Ugh - bad form in my opinion. But there's a gazillion and a half ways to skin a cat.

vining

This isn't really complicated but it might make a newbie scratch their head for a while.

{
     stack_var integer n ;
     
     for (n = 1 ; n <= HVAC_ZONES; n ++)
	  {
	  stack_var integer i ;
	  for (i = 1 ; i <= TSTAT_NUMDAYS ; i ++)
	       {
	       stack_var integer y ;
	       for (y = 1 ; y <= 4 ; y ++)
		    {
		    if (length_string(sVSTStat[n].sVSTDay[i].sVSTSet[y].cVST_Time) == 0)
			 {//if one values is missing, reload all! New zones may have been added!
			 RETURN TRUE ;
			 }
		    }
	       }
	  }
	  
     RETURN FALSE ;
     }

DHawthorne

If you look at my example, it's just a simple conversion from a switcher volume that has separate left/right values to the level range the panel itself uses. It boils down to PanelVolume = (SwitcherVolume(left) + SwitcherVolume(right)) / 2 . It doesn't get much simpler than that in terms of operations, it's the tracking arrays that make it convoluted, and the fact that the array index references are themselves arrays, since different panels could be controlling different zones. It's the price you pay for flexibility.

a_riot42

Gadzooks! I would get confused if I had to read that kind of code. I usually encapsulate all the array accesses into functions. This allows me to have functions with nice meaningful names that make it easy to read. As well, I do all my array bounds checking in these functions so I never have to write any other bounds checking code. I would hate to come back to some of that code above 1 year later and try to remember what I did.

ui = get_last(UIs) 
volumeUp(getOutput(getSource(getZone(ui))))

See? No square brackets
And what's with all the magic numbers up there (500,1400 etc)???
I was looking at some code my boss wrote and it had a line like this:

PULSE[dvHDTV, 11 + GET_LAST(SAT_NUM) - 4 ]

Of course, I asked him what was the significance of the 4 and 11 were, and why he didn't just add 7 rather than add 11 and then subtract 4. He said he couldn't remember what the magic numbers represented any more

Paul

jjames

Nice!!

a_riot42 wrote:

Gadzooks! I would get confused if I had to read that kind of code. I usually encapsulate all the array accesses into functions. This allows me to have functions with nice meaningful names that make it easy to read. As well, I do all my array bounds checking in these functions so I never have to write any other bounds checking code. I would hate to come back to some of that code above 1 year later and try to remember what I did.

ui = get_last(UIs) 
volumeUp(getOutput(getSource(getZone(ui))))

See? No square brackets
Paul

Nice - I like that idea. Kudos to you!

a_riot42

jjames wrote:

Nice - I like that idea. Kudos to you!

Thanks, although I think I am in the minority as far as code style is concerned. People are probably thinking "I hope our company never has to take over one of this guys jobs"
Paul

[Deleted User]

I'll second that

a_riot42 wrote:

Thanks, although I think I am in the minority as far as code style is concerned. People are probably thinking "I hope our company never has to take over one of this guys jobs"
Paul

Nice job Paul!

Code should not only work, but also should be highly readable and efficient. Those of you that have seen a well-dressed equipment rack verses a spaghetti mess knows what I mean.

It make me cringe to see the same function on the same expression called more than once in the same evaluation or event.

Perhaps a few coders could use some neatness lessons.

a_riot42

In case you got notifications but now see no corresponding post, I deleted them so as not to offend anyone. It is unfair to critique code when you don't know the context, how much time the programmer had or any other constraints they might have been under that day. My apologies if you got notified of a post that has been deleted. No offense was meant at all.
Many thanks,
Paul

vining

a_riot42 wrote:

Here is the message that has just been posted:
***************
Not to pick on anyones code, but its look we are competing in the Netlinx Obfuscation Contest

I belive that was the purpose of the thread!

cont.

One think I noticed in Vinings for loop arrangement is that he calls length_string numerous times even though it will always evaluate to the same number, since 'n' only changes after the first for loop. Why not capture what length_string returns to a variable and use that instead?

FYI, the lenght string will evaluate a different element of the structure on every pass.
hence i & y!

cont.
Generally speaking, calling functions in a for loop is bad style if it will always return the same value anyway.

what function? Have you been smoking?

A trebley nested for loop scares me though. That will run in order n squared:
(n * y * 4)

releasing the constant 4 as it is insignificant as n or y get large you have (n * y) iterations.
I am not exactly sure what that code does, so it may be the only way to do it, but usually you can avoid a n*n loop or at least change it to n*log(n).
You can get away with it in this application because it is unlikely that a house would have millions of thermostats, but if it were calculating a value you would see it get exponentially slow for large inputs.
Paul
***************
It is unfair to critique code when you don't know the context, how much time the programmer had or any other constraints they might have been under that day. My

In other words my code sucks but it might not be my fault cuz maybe my dog died that day or something. Pretty arrogant aren't you!

Fill in the blank here_______where my response should be!

vining

a_riot42 wrote:

I don't feel I am a newbie and it is making me scratch my head (and other body parts) One thing I don't like is when people have return true or return false in their code. For the above couldn't you just do


{
stack_var integer n ;

for (n = 1 ; n <= HVAC_ZONES; n ++)
{
stack_var integer i ;
for (i = 1 ; i <= TSTAT_NUMDAYS ; i ++)
{
stack_var integer y ;
for (y = 1 ; y <= 4 ; y ++)
{
return length_string(sVSTStat[n].sVSTDay.sVSTSet[y].cVST_Time
}
}
}
}

No that doesn't work the same because you took out the if statement. I happen to like putting in the return true and return false. To each their own!

GSLogic

B_Clements wrote:

Code should not only work, but also should be highly readable and efficient.

I couldn't agree more!

I use stack_vars to first gather deep nested info:

nPNL = get_last(dvTP)
nIDX = get_last(buttonPressed)
nStatusThis = varLite[nIDX].whatever[j][k][x]

  [dvTp[nPNL],nIDX] = nStatusThis;
OR
  [dvTp[get_last(dvTP)],get_last(buttonPressed)] = varLite[get_last(buttonPressed)].whatever[j][k][x]];

It is easier to read not only for the next guy but for me. I have some code from years ago (that I'm not very proud of), it took me an hour just to follow the worm holes. I also use structures to keep all the info in one spot and to avoid naming many different arrays.

You'll only ever really need to deal with this in large installs, but the key is to plan it out on paper first.
(This is just an example, please don't hold me to the code )

jjames

It seems quite a few of you use structures. Do you think it makes much of a different between structures and arrays? I tried using structures on a couple of jobs - either I didn't get it down right, or whatever - but I felt it was a waste of time and was too difficult to track and try to remember what came next. At least with arrays, I just need to remember the array name, and not need to know if .LOCATION comes before or after .PANEL.

I really wish we'd be able to post our ENTIRE code to actually see what some of us do. Now - I know I can't do that with my code, and I'm sure most - if not all - of you can't either, I think we're missing a whole lot whenever we just post one or two lines.

Next question - when you do have such long statements, PLEEEEEASE tell me you all use comments to remind yourself what is going on.

GSLogic

jjames wrote:

It seems quite a few of you use structures. Do you think it makes much of a different between structures and arrays? I tried using structures on a couple of jobs - either I didn't get it down right, or whatever - but I felt it was a waste of time and was too difficult to track and try to remember what came next. At least with arrays, I just need to remember the array name, and not need to know if .LOCATION comes before or after .PANEL.

I really wish we'd be able to post our ENTIRE code to actually see what some of us do. Now - I know I can't do that with my code, and I'm sure most - if not all - of you can't either, I think we're missing a whole lot whenever we just post one or two lines.

Next question - when you do have such long statements, PLEEEEEASE tell me you all use comments to remind yourself what is going on.

Of course you can use arrays, I just find it easier to use structures, one big fact: with arrays you need to remember all of the names when debugging.
nAD_out_loc[18];
nAD_out_nSrc[18];
nAD_out_eq[18][10];

If you look at my AP18x18 structure you'll see all the info for all locations are in one place, when you use debug everything is there for you, just by typing AD_out. With arrays you'd have to type each variable one at a time. If you need to see location 5, 3rd band of eq: AD_out[5].eq[3]; - It keeps thinks much more organized.

structure _AD_out
  {
	char	 cLoc[24];		// output location name
	integer  nSrc;			// source selected
	sinteger snVol;			// output volume
	sinteger snTR;			// treble
	sinteger snBS;			// bass
	slong 	 snEQ[10];		// 10 band eq
	integer  nMute;			// output mute status
	integer  nPanel[nTOTAL_PANELS];	// hold all panels viewing this output
	integer  nLockOut;		// lockout others from changing your music
  };

I have structure for everything: tp-audio-video-shades-security-event timers-kids-fish-ect.
Hope this help!

DHawthorne

Structures have the advantage of a member name; you don't have to remember what index in the array refers to what. They can also contain different data types, where an array has to all be the same. The disadvantage is you can't pass them to modules. There is a place for both.