Showing posts with label Development. Show all posts
Showing posts with label Development. Show all posts

Thursday, July 13, 2017

2E Code Review - pet hates - part II


Back again...

My intention was that the first post would cover my major pet hates...  Being the perfectionist (that I am when it comes to coding) I quickly realised that I see many more coding mistakes/habits/bad practices that irritate me.

So here are another 3 things (there are still others on the list) that "get on my goat".  I laugh inwardly because I can see each and every developers face who I associate with some of these bad habits.  I also frustratingly recall the countless times I've explained why 'xyz' is bad without achieving a behavioral change.  Stubbornness is a very special human trait.

I go back to a previous post, way way back in 2008/2009 in fact.  I state that without a declared mandate/role to reject bad code, things never change, performing code reviews by democracy simply doesn't work.  This is why I am such a fan of 'automated development standards' review technology as it takes away the conflict.

Back to the list of bad habits that are consuming me today :-)

4. Externalising a single function to get over file limits.

Ever hit the RPG 50+ file limit?  Often you will see people externalising a single RTVOBJ (for example) in order to get back to 50 files or less.  This is the most shortsighted piece of coding I ever see and will only damage your code base.  In this instance RPGLE wasn't an option but as you can tell the option for structuring wasn't taken.

Here is a rather extreme example of a (now deprecated) function from my model at work.  I have altered some of the images for privacy reasons.









Associated hates: Not only will the multiple external functions increase the call stack, reduce performance, introduce errors as people forget about passing the *Return Code back (common issue when externalising) it is only ever a short term option as applications expand significantly overtime.

There are numerous other options which I have covered before like RPG to RPGLE conversion, externalising a major block of associated logic i.e. Load, Validate or Update processing or full function refactoring etc.

Tip: My preference is to aim for a mixture of RPGLE migration and also some refactoring options for better function isolation.  This depends on how much user source you have etc.


5. BOTH parameters that are initialised from within.

Often a developer will need to total up some values across multiple records.  In 2E we would typically use a RTVOBJ to work through a set of records and increment a value.

A quick and dirty way to do this is to declare a value that you want to sum up as a both parameter.  Then within the AD (Action Diagram) the code to increment the value is PAR = PAR + DB1 and an average developer would argue that the passing of the value to the output variable via the PAR is automatic.

Their logic is sound if you are trying to keep your action diagram code to the minimum.  Look at the two example below.

The lazy way





The correct way







On the surface it looks like the lazy way is okay.  It ticks the boxes for.

  • Clear function naming
  • Minimal AD code (x lines vs y lines)

Then it fails miserably. 

Interface




The BOTH method's parameter interface is not as easy to decipher as the correct method.  You wouldn't know from the function interface that the field(s) being totaled are being initialised back to Zero from within.


What should happen if a value is passed in.  You would expect that the value passed in, say 100.00 would be included in the total.  Here is the confusion.  The function name and interface should clearly depict what is going in inside. 

6. Leaving the MAP on.

MAP parameters are designed to pass a value passed into a function (via parameters) and place it on a device design and automatically populate the data.  i.e. MAP the parameter value to the screen or report.  There is NO other use of the MAP role within 2E.


Leaving the MAP on internal functions like a CRTOBJ or RTVOBJ. 


Whilst not impacting code, it is a sign that the developer doesn't truly understand the role of MAP and if they do, then by leaving it on they are implying that it is satisfactory to be tardy.  To me it is like not dotting an i or crossing a t.

I am going to give the developer a little out here.  CA could make it easier by NOT setting MAP by default and making it a 'opt in' option rather than something we have to constantly switch off.


Thanks for reading.
Lee.

Thursday, February 9, 2017

2E Code Review - pet hates - part I


Today I thought I'd get a few things off my chest. 

Whenever I review code, look at old code or have the job of maintaining code (not my favourite part of the job by a long shot) I am often left dumbfounded.  I see the same old mistakes being made regardless of where I have worked. You will often hear me saying,"Whoever wrote this should be shot!!".

The most annoying part is that in many places the developers agree about the best way to code but quickly use the excuse of "I don't have enough time", or "Well it's done now, so don't worry" to not code properly.  My experience tells me it is often the most experienced developer(s) who are the hardest to convince of solid development standards with the terms "Old dog" and "new tricks" preeminent in my mind....

IMHO the difference between a run of the mill programmer (I've worked with plenty of these) and a good one (fewer but they are out there) is adherence to the little details that make long term maintainability of your code as easy as it should.  This is especially true considering the inevitable change and enhancement that is required during an applications growth and evolution.  I have espoused the standard development quote that 90% of an applications life cycle is maintenance numerous times before.

Anyhow, my post today is a list of pet hates that I see when people are "unprofessional", "selfish","crude", "Lazy" with their 2E coding.

1. Legacy commented out code.

I recently worked on some maintenance and had to work through some action diagram code.  Using the Find Services option within 2E I started to quickly get frustrated that the majority of the usages that were commented out.

What was worse still is that there were developer comments (from 2002) stating that the code was commented out and I know that this area has been enhanced many times since.  There is simply no reason to keep such old code.  The developer concerned is normally quite good but has a couple of really bad habits like this one.

Here is a snippet of code from an old function (screen print kindly shown with permission).  Note, I have redacted any client or developer or brand comments (hence some empty white space).

 

I am all for commenting out code as part of debugging, rapid prototyping, unit testing and quick wins (hot fixes) where you are not quite sure of the change you are making, but please annotate when and why.  However, I am aware that some of this code was moved to their own program(s) so IMHO should just have been removed.

Associated hates:
  • Having to weed through the chaff to get to the code to change.
  • Any old code will not have been maintained so will not (often) be fit for purpose (if you decided to uncomment it) so why leave it there.
  • Extra impact analysis (especially for internal functions). 
  • Confusion with the impact analysis usage.  A future post is planned for this.
  • Better to take a version.
  • Commenting out code doesn't change the timestamp for the AD line so we don't know when it was commented out.
Tip: Comment out code sparingly and preferably not at all.  Be confident with your code and solution.  After you have completed the work (and tested it), if you are 100% happy with the results revisit the action diagram and remove commented out code.  Commented out code is a maintenance burden and others will not understand why.

Here is the same code with the bad commented out code and legacy comments removed.

Still not perfectly structured but a whole lot more readable.  Perhaps CA can add a hide/show commented out code option for the action diagrammer.

2. "GET All fields" RTVOBJ not doing as described.

Every 2E developer has written the standard RTVOBJ that returns the full record. Yes, we all have forgotten the *MOVE ALL, but my biggest annoyance is when people don't visit these functions when the underlying file structure changes.

Imagine the "*GET", "GET Record", "Get All", "RTV All Fields" function without the new fields as parameter.


The next developer comes along and low and behold, there are a few fields missing.  Then they create a new one called Get All (New) and probably flag the other one with some kind of encoding system like DNU for Do Not Use etc.

Associated hates:

Numerous versions of the same type of function.  Too many choices.
Description vs reality can be misleading and frustrating.

Tip: If you have to change the file, you may as well change the "Full Record" type function as you have to regenerate up everything anyhow.

3. Using all 9 parameter definition lines for functions parameters.

In the old days when we only had *FIELD, Access Path(s) or Structure files for defining parameters it could sometimes get a little busy and we'd fill the 9 lines leaving limited options for the next developer.  We shouldn't create structure files just for the sake of it, after all, their primary purposes was for consistency of repeating data groups like audit stamps etc not for easier passing of parameters.

Since version 4.0 (over 20 years) we've been able to define parameter arrays.  There really is no excuse now for taking up the last line of a functions parameter block unless "I don't have time" or "I'll do it when we get to 10" is a valid reason.

Tip: When maintaining a function and say 6 or more parameter block lines are used consider refactoring with a parameter array.

I still think that even using parameter arrays as being a bastardised solution for this problem and that 2E should just have had more that 9 lines....but it is the lesser or two evils.

This is the tip of the iceberg.  Plenty more to follow I am sure.

Thanks for reading.
Lee.

Thursday, September 8, 2016

Under the covers of the SELRCD function type


Hi,

This blog touches on the design challenges when using select record (SELRCD) function type and makes some recommendations for best practices.  Disclaimer:  These are from personal experience and may not cover all scenarios.  I would be happy to hear of any ideas/improvements that can be made to this post.

SELRCDs can cause quite a few issues if you are not aware of the quirks of how Synon generates the code.

Let's cut straight to the problem domain at hand.  In my sample model you will see that I have two files declared.  You will see that "Lee's File" Refer to "Lee's Reference File" which will mean that if I have a PMTRCD or EDTFIL etc over Lee's file we will be able to prompt for the reference data.


The functions I have over the reference file are as follows, I have removed the default EDTFIL and renamed the database primitive functions inline with our internal development standards.

 The main file has numerous test functions so I have filtered on Edit file only.


The screens (Edit File and Select Records) in the blog post have been tidied (ever so slightly) to ensure that all the fields fit nicely on the screen.

I have generated up the code for the EDTFIL.

Reviewing the generated code.

By default you will see that Synon has generated code to call the SELRCD function upon prompting the field and Synon has chosen my SELRCD based on the reference file.

You will also notice that the parameters for the SELRCD have been defaulted to fields based on the RCD context of the calling program.  In this instance it was an EDTFIL.


This is pretty standard and works a treat.

Now we will add a few additional SELRCDs, you will see these below.  Note the naming of these was deliberate in order to ensure that they appeared after 'Select Lee's Reference Fi'.


In terms of impact analysis you will notice that the SELRCD is showing no usages although we have generated code above that proves the object is called.


This is due to implicit code generation that is automatically added if the field is a result of a foreign key constraint AND is input capable AND a select record exists on the reference file.  If you deleted the SELRCD at this stage and generated the code again the prompt and call logic will not be generated.  Give it a try!


Above is a screen image of the default Synon setup.  Notice the two subfile options at the bottom of the screen.  S allows you to select the SELRCD that should be used, T clears this value and leaves the generator to choose the default SELRCD.


After taking the S option and selecting my SELRCD from the reference file you will see that this is indicated above.  Because we have made an explicit reference to the SELRCD in the device design it will now appear as a usage within Synon.


You will see below that the generated code is as before which is great :-)


In order to help with the blog narrative, please now deselect the SELRCD by taking the subfile option T for default behaviour.   This will clear the reference to the dedicated SELRCD.


Now we will create another SELRCD (again deliberately named).  You will see that it sits above the current (default) select record for the EDTFIL.


If we generate the EDTFIL again we will see that the outcome is no longer the same. The generator now puts a call into the higher ordered SELRCD on the reference file.  We didn't intend for this to happen and a developer doing a regeneration could easily pick this up without knowing.  Quite dangerous....


In this instance, as the keys on the SELRCD are identical there is NOT too much to worry about.  However, in the real world 2nd and subsequent SELRCD functions will typically be over an alternate access path with a different key structure or may have additional parameters or specialist action diagram functionality which may not be the preferred default behaviour.

In order to demonstrate this let's add some fields to the model which we will then associate with a new SELRCD function.


Create yet another SELRCD function over the referenced file.  Note again I have named it deliberately so it appears at the top of a refreshed EDIT FUNCTIONS panel.


I have extended the parameters for this SELRCD to add our five new fields.


Let's generate up the EDTFIL once again and look at the generated code.

Ouch!!  Houston we have a problem.

You will see below that all of the new parameters have been defaulted to CON.  This is because the fields were not found in context for the relationship. i.e. in this case, not part of the RCD context. And because this is a warning message only it may go unnoticed.  Preferably Synon should mark these as errors and only allow the correct context ones to generate correctly i.e RCD example above.

Also we have now picked up yet another SELRCD.  One can conclude that anytime a new SELRCD is added and it appears higher up in the EDIT FUNCTION panel the next time a referencing function is generated it will generate code to call the new SELRCD and not the previous one.  Imagine a highly reference file and the chaos this could bring.


We can do somehing to help here.  Whilst we cannot get at the parameters at the point the implicit call to the SELRCD is made, we can put these fields in the context and populate them accordingly.

Add the fields to the 'Subfile Record' format RCD.  You will notice I set these to hidden.


Then in the action diagram you can set these values using a standard *MOVE built-in function.



Generate the function once more and the generated code now shows the moved and also defaults to the RCD context meaning that you no the parameters are populated with your values and NOT CON blank or Zero.



But I remind you at this stage that these were warnings in the code only.  How many people check the generated source for warnings.  I don't, do you?

Another way to do this is to switch off default processing and take control of the prompting yourself in the action diagram coding.  Back in the EDTFIL function again set the relation checking to 'USER'.




Note: This switches off all referential integrity checking so in the real world you'll also need to add action diagram code for the OPTIONAL/MANDATORY as well as foreign key constraints.  For the purposes of this blog I am only focused on the prompting and calling of the select record.

You can then monitor for prompting on your field and call ANY SELRCD function you think is appropriate.


With this method you get a nice parameter interface for the SELRCD and more importantly it is obvious to another developer that you are initialising the fields for an explicitly called SELRCD rather than an implicitly called on.  Some comments might be useful if you choose the other method.


This highlights the many options you have for calling a SELRCD and should help you decide what is best for your model, team etc.

Once last issue with SELRCDs.  I mentioned earlier that Synon (by default) will chose the highest SELRCD based on the referenced file.  Due to no usages showing it is possible that code will be generated to call an object that hasn't been created or has been removed by a developer thinking it is not used.  This will mean you'll get a 'resolve to object' call error which isn't a good look.


Best tip is to generate up a standard SELRCD for each reference file and implement it at the time the file is created.  If you decide that a file doesn't need prompting, then remove ALL SELRCDs from the file so this code doesn't get generate implicitly.  As you can see from above the code will get generated if the function exists regardless of whether the object exists or not.

Best Practices Summary

  1. Come up with a naming convention for the default SELRCD so that it is first in the list on the EDIT FUNCTIONS panel.  We use '*SLT xxxxxxxxxxxxxxx'.
  2. Always ensure you generate up and implement ALL SELRCD functions.
  3. Always keep the default SELRCD keys the same at the RTV access path.
  4. Always name 2nd and subsequent SELRCD so they appear lower in the EDIT FUNCTIONS panel.
  5. It is recommended you set explicitly the SELRCD you want to call either by the 'S' option or via action diagram coding.
  6. Never delete any SELRCD objects without checking the generated source files or by exploring all file references.

Thanks for reading.
Lee.