|
View previous topic :: View next topic
|
| Author |
Message |
pullaiah.cts
New User
Joined: 02 Sep 2010
Posts: 50
Location: Pune
|
|
|
|
Hi,
I have a table with 26 fields.
Table declaration:
| Code: |
10 WP-SECURITY OCCURS 26 TIMES.
15 WP-TSECTA PIC X(15).
15 WP-NGIDN2 PIC X(12).
15 FILLER PIC X(14).
01 SUBSCRIPTS COMP.
05 IN-FILE-SUB PIC S99.
|
I need to search for the string 'CUSIP' in each record,if CUSIP is found then we need to write it's corressponding value to o/p record.
In case,CUSIP is not found then we need to check for 'INTERNAL' and write the corressonding value to o/p value.
If both are not present then read the next record.
Program Logic:
| Code: |
3000-READ-CUSIP-PRICE.
MOVE "N" TO ITEM-FOUND-SWITCH.
PERFORM 3010-SEARCH-PRICE-FLDS
WITH TEST AFTER
VARYING IN-FILE-SUB FROM 1 BY 1
UNTIL ITEM-FOUND
OR IN-FILE-SUB = 26.
IF ITEM-FOUND
PERFORM 4000-WRITE-OUTPUT-FILE THRU 4000-EXIT.
3000-EXIT. EXIT.
*SEARCHING FOR CUSIP FIELD IN I/P RECORD
3010-SEARCH-PRICE-FLDS.
IF WP-TSECTA (IN-FILE-SUB) = 'CUSIP'
THEN MOVE "Y" TO ITEM-FOUND-SWITCH
ELSE IF WP-TSECTA (IN-FILE-SUB) = 'INTERNAL'
MOVE "Y" TO ITEM-FOUND-SWITCH
END-IF.
|
But the program us not working as expected.It's checking for both 'CUSIP' and iINTERNAL' and taking whichever exists first.
But,it should not be like that.First it should check for 'CUSIP',if not found then only it will go for 'INTERNAL'.
Can anyone please let me know what are all the changes I need to make in my coding? |
|
| Back to top |
|
 |
Bill Woodger
Moderator Emeritus
Joined: 09 Mar 2011
Posts: 7309
Location: Inside the Matrix
|
|
|
|
| Well, first you need to check for CUSIP in all entries of the table that are populated. Only if you don't find CUSIP should you check for INTERNAL. If you test for both at the same time, you will find INTERNAL on occasion when a later table entry contains CUSIP which hasn't yet been looked at. |
|
| Back to top |
|
|
pullaiah.cts
New User
Joined: 02 Sep 2010
Posts: 50
Location: Pune
|
|
|
|
Hi Bill,
I have made the changes as below but still facing the same problem.
| Code: |
* CHECKING FOR CUSIP PRICE FIELD.
3000-READ-CUSIP-PRICE.
MOVE "N" TO ITEM-FOUND-SWITCH.
PERFORM 3010-SEARCH-PRICE-FLDS
WITH TEST AFTER
VARYING IN-FILE-SUB FROM 1 BY 1
UNTIL ITEM-FOUND
OR IN-FILE-SUB = 26.
IF ITEM-FOUND
PERFORM 4000-WRITE-OUTPUT-FILE THRU 4000-EXIT.
3000-EXIT. EXIT.
* SERCHING FOR CUSIP FIELD IN I/P RECORD
3010-SEARCH-PRICE-FLDS.
IF WP-TSECTA (IN-FILE-SUB) = 'CUSIP'
THEN MOVE "Y" TO ITEM-FOUND-SWITCH
ELSE
PERFORM 3020-SEARCH-INTER-FLDS THRU 3020-EXIT.
* SERCHING FOR INTERNAL
3020-SEARCH-INTER-FLDS.
MOVE "N" TO ITEM-FOUND-SWITCH.
PERFORM 3030-SEARCH-INTERNAL
WITH TEST AFTER
VARYING IN-FILE-SUB FROM 1 BY 1
UNTIL ITEM-FOUND
OR IN-FILE-SUB = 26.
3020-EXIT. EXIT.
* SERCHING FOR INTERNAL
3030-SEARCH-INTERNAL.
IF WP-TSECTA (IN-FILE-SUB) = 'INTERNAL'
THEN MOVE "Y" TO ITEM-FOUND-SWITCH
END-IF.
|
|
|
| Back to top |
|
|
Bill Woodger
Moderator Emeritus
Joined: 09 Mar 2011
Posts: 7309
Location: Inside the Matrix
|
|
|
|
| Because you have to complete the search of the table for CUSIP before starting the search of the table for INTERNAL. You are still doing each, for each increasing subscript. You'll still find INTERNAL at one (if there) even if CUSIP at 26. |
|
| Back to top |
|
|
pullaiah.cts
New User
Joined: 02 Sep 2010
Posts: 50
Location: Pune
|
|
|
|
Hi Bill,
I am assuming that I am searching for 'INTERNAL' only after finishing searching for 'CUSIP'. Can you please let me know where I am wrong in the code? |
|
| Back to top |
|
|
Bill Woodger
Moderator Emeritus
Joined: 09 Mar 2011
Posts: 7309
Location: Inside the Matrix
|
|
|
|
You perform 3010 from 1 to 26.
You test entry 1 for CUSIP, then if not, you go for and test for INTERNAL, before entry 2-26 are looked at for Cusip.
You need:
| Code: |
1-26 for CUSIP
IF not CUSIP found
1-26 for INTERNAL |
You have
| Code: |
1 for CUSIP, if not
1-26 for INTERNAL, if not
2 for CUSIP, then 1-26 for INTERNAL again,
etc. |
|
|
| Back to top |
|
|
Jose Mateo
Active User
Joined: 29 Oct 2010
Posts: 121
Location: Puerto Rico
|
|
|
|
Good day to all!
I really think that you first logic would work with one minor change and that would be to move the statement 'move 'n' to ITEM-FOUND-SWITCH' after the label '3010-SEARCH-PRICE-FLDS' because once you found a true condition the rest would be true due to you didn't reset the flag ITEM-FOUND-SWITCH to 'N'. Another thing is that your WP-TSECTA is 15 bytes long, could it contain 'CUSIP' or 'INTERNAL' plus some other characters as filler to the field? |
|
| Back to top |
|
|
Marso
REXX Moderator
Joined: 13 Mar 2006
Posts: 1353
Location: Israel
|
|
|
|
| pullaiah.cts wrote: |
| Code: |
3000-READ-CUSIP-PRICE.
MOVE "N" TO ITEM-FOUND-SWITCH.
PERFORM 3010-SEARCH-PRICE-FLDS
WITH TEST AFTER
VARYING IN-FILE-SUB FROM 1 BY 1
UNTIL ITEM-FOUND
OR IN-FILE-SUB = 26.
IF ITEM-FOUND
PERFORM 4000-WRITE-OUTPUT-FILE THRU 4000-EXIT.
3000-EXIT. EXIT.
*SEARCHING FOR CUSIP FIELD IN I/P RECORD
3010-SEARCH-PRICE-FLDS.
IF WP-TSECTA (IN-FILE-SUB) = 'CUSIP'
THEN MOVE "Y" TO ITEM-FOUND-SWITCH
ELSE IF WP-TSECTA (IN-FILE-SUB) = 'INTERNAL'
MOVE "Y" TO ITEM-FOUND-SWITCH
END-IF.
|
|
That is an ugly piece of code. 
I hope you do better on the mainframe than on the forum.
Do you? |
|
| Back to top |
|
|
Bill Woodger
Moderator Emeritus
Joined: 09 Mar 2011
Posts: 7309
Location: Inside the Matrix
|
|
|
|
| Jose Mateo wrote: |
Good day to all!
I really think that you first logic would work with one minor change and that would be to move the statement 'move 'n' to ITEM-FOUND-SWITCH' after the label '3010-SEARCH-PRICE-FLDS' because once you found a true condition the rest would be true due to you didn't reset the flag ITEM-FOUND-SWITCH to 'N'. Another thing is that your WP-TSECTA is 15 bytes long, could it contain 'CUSIP' or 'INTERNAL' plus some other characters as filler to the field? |
I think you've completely misunderstood what is required and the existing code.
If there is any occurence of CUSIP in the table the first occurence should be located and a record written.
If there is no CUSIP, then the first occurence of INTERNAL should be located and a recored written.
If neither CUSIP nor INTERNAL are found, no record is written.
The change of location of the piece of code you suggest would cause the setting of the flag from the previous time through the code to be used, assuming there is no CUSIP in table-entry-1. The chance that this is what is wanted I would guess to be Nil.
In my experience it is perfectly normal for a field to be used which can contain values of different length followed by spaces. |
|
| Back to top |
|
|
Jose Mateo
Active User
Joined: 29 Oct 2010
Posts: 121
Location: Puerto Rico
|
|
|
|
Hi, Bill!
Reading his requirement for the 2nd, 3rd and 4th time, yes I misunderstood what he wants to do. This second code is worst than the first and I agree with Marso. Now, by what I understood is that this table is in each record. First, he searches for the Literal 'CUSIP' in all the entries of the table. Once 'CUSIP' is found then he want to write the corresponding WP-NGIDN2 to the o/p and read another record. If the first search fails then he wants to search for the literal 'INTERNAL' in all the entries of the same table. Once 'INTERNAL' is found then he want to write the corresponding WP-NGIDN2 to the o/p and read another record. If the second search fails then he wants to read another i/p record and start the process again. Is this correct? |
|
| Back to top |
|
|
Marso
REXX Moderator
Joined: 13 Mar 2006
Posts: 1353
Location: Israel
|
|
|
|
Can you show us the paragraph 4000-WRITE-OUTPUT-FILE ?
Just in case, that's why I said ugly:- You should use indentation. It helps at lot when reading a program.
- You should use END-PERFORM when writing an in-line PERFORM.
- You should be consistent (in paragraph 3010 you have two IFs, one END-IF and one period).
|
|
| Back to top |
|
|
Bill Woodger
Moderator Emeritus
Joined: 09 Mar 2011
Posts: 7309
Location: Inside the Matrix
|
|
|
|
I don't know where the table is, Jose, but I think the rest is right. We have to wait (and it may be some time) for pullaiah.cts for confirmation.
To add to Marso's list:
One THEN (don't have any, or use them always, not a mix); a "flattened" nested IF (could be made EVALUATE); literals scattered instead of datanames with a value (there is no XREF for literals); MOVE to switches, could use 88s and SET; 88s not used for CUSIP/INTERNAL; COMP PIC S99; WITH TEST AFTER with no good reason. I'm not going to look at the second code sample. |
|
| Back to top |
|
|
pullaiah.cts
New User
Joined: 02 Sep 2010
Posts: 50
Location: Pune
|
|
|
|
Hi All,
Sorry for the late reply,I was sick.
I have resolved my issue by replacing my logic with SEARCH technique.
Here is my latest code:
| Code: |
* CHECKING FOR CUSIP FIELD.
3000-CUSIP-SEARCH.
MOVE "Y" TO CUSIP-FOUND-SWITCH.
SET WS-SEC-TABLE-INDEX TO 1.
SEARCH WP-SECURITY VARYING WS-SEC-TABLE-INDEX
AT END PERFORM 3020-INTERNAL-SEARCH THRU 3020-EXIT
WHEN WP-TSECTA (WS-SEC-TABLE-INDEX) = 'CUSIP'
PERFORM 4000-WRITE-OUTPUT-FILE THRU 4000-EXIT
END-SEARCH.
3000-EXIT. EXIT.
3020-INTERNAL-SEARCH.
MOVE "Y" TO INTERNAL-FOUND-SWITCH.
SET WS-SEC-TABLE-INDEX TO 1.
SEARCH WP-SECURITY VARYING WS-SEC-TABLE-INDEX
AT END MOVE 'N' TO INTERNAL-FOUND-SWITCH
WHEN WP-TSECTA (WS-SEC-TABLE-INDEX) = 'INTERNAL'
PERFORM 4000-WRITE-OUTPUT-FILE THRU 4000-EXIT
END-SEARCH.
3020-EXIT. EXIT.
* WRITING CUSIP PROCE TO O/P FILE
4000-WRITE-OUTPUT-FILE.
MOVE WP-NGIDN2 (WS-SEC-TABLE-INDEX) TO WS-CUSIP-VALUE.
MOVE WP-AUSDP TO WS-CUSIP-PRICE.
MOVE WS-CUSIP-FLD TO OUT-PRICE-FLE-REC.
WRITE OUT-PRICE-FLE-REC.
COMPUTE REC-WRITTEN-CNT = REC-WRITTEN-CNT + 1.
4000-EXIT. EXIT.
|
Thank you all for your great support. |
|
| Back to top |
|
|
Bill Woodger
Moderator Emeritus
Joined: 09 Mar 2011
Posts: 7309
Location: Inside the Matrix
|
|
|
|
pullaiah.cts,
Sorry to hear you were sick. Thanks for the update.
You've not really solved your problem by using SEARCH instead of PERFORM. You've solved your problem by searching for CUSIP in all the occurences first, then searching for INTERNAL after that.
We've had a gentle "dig" at your code, not for fun, but for constructive criticism. If you code cosistently, using indentation to show hierachy and avoid "overloading" your performs, you code will look better, be easier to understand and easier to maintain. Errors will be much easier to spot, and much more difficult to introduce in the first place.
To your current code: you are setting one flag on, and never setting it to any other value - if used elsewhere, it is a bug, if not used, ditch it (and the other flag, although that one is set to a different value); your paragraph 3000-SEARCH-CUSIP, despite the name, does three things (search for CUSIP, search for internal, write to output file for CUSIP) - someone lookting through your code further up will be frustrated by that, and will have to learn not to trust your paragraph names; you are still using literals - you may feel that is OK because it is not like you'll ever have to change the value of CUSIP to something else, in that they're not going to rename the term, but what if you "clone" the program to do some similar processing by ISIN? That is more difficult than need be. SET with a well-named 88 does a much better job for a flag than a MOVE of a literal does; you also have two very similar pieces of code - using the value of a field in a SEARCH/PERFORM, and using it with two different values, removes the need to keep two similar but slightly different pieces of code working; why no visible error-checking on the write?
| Code: |
SET W-NOT-FOUND-IN-WP-TSECTA TO TRUE
MOVE W-CUSIP-LIT TO W-ITEM-TO-SEARCH-FOR
PERFORM 3000-SEARCH-TSECTA
IF W-NOT-FOUND-IN-WP-TSECTA
MOVE W-INTERNAL-LIT TO W-ITEM-TO-SEARCH-FOR
PERFORM 3000-SEARCH-TSECTA
END-IF
IF W-FOUND-IN-WP-TSECTA
PERFORM 4000-BUILD-OUTPUT-RECORD
PERFORM 4010-WRITE-OUTPUT-FILE
END-IF
[...]
.
3000-SEARCH-TSECTA.
SET WS-SEC-TABLE-INDEX TO 1
SEARCH WP-SECURITY VARYING WS-SEC-TABLE-INDEX
AT END CONTINUE
WHEN ( WP-TSECTA ( WS-SEC-TABLE-INDEX )
EQUAL TO W-ITEM-TO-SEARCH-FOR )
SET W-FOUND-IN-WP-TSECTA TO TRUE
END-SEARCH
.
4000-BUILD-OUTPUT-RECORD.
MOVE WP-NGIDN2 (WS-SEC-TABLE-INDEX) TO WS-CUSIP-VALUE
MOVE WP-AUSDP TO WS-CUSIP-PRICE
MOVE WS-CUSIP-FLD TO OUT-PRICE-FLE-REC
.
4010-WRITE-OUTPUT-FILE.
WRITE OUT-PRICE-FLE-REC
[ERROR CHECKING]
COMPUTE REC-WRITTEN-CNT = REC-WRITTEN-CNT + 1
. |
To see the "logic" you don't need to look inside the performs, you can trust that the names relate to the one-to-one task that they carry out.
PS. If it were my code, I'd like to format it differently as well, and have nicer prefixes for the paragraphs/sections :-) |
|
| Back to top |
|
|
dbzTHEdinosauer
Global Moderator
Joined: 20 Oct 2006
Posts: 6966
Location: porcelain throne
|
|
|
|
if i was coding this routine,
i would make only one pass of the table using a PERFORM VARYING
with two compound (yet very simple to read and follow) IF statements.
also, the flags would be comp fields:
ws-occurance-of-CUSIP
and
ws-occurance-of-INTERNAL
initialize both comp fields as zero.
I would also use an INDEXED BY phrase in the definition of the COBOL INTERNAL TABLE
(since we want this to be good code)
| Code: |
PERFORM VARYING TABLE-IDX
FROM 1
BY 1
UNTIL TABLE-IDX GT 26
IF ws-occurance-of-CUSIP-ZERO
THEN
IF WP-TSECTA-CUSIP
THEN
SET ws-occurance-of-CUSIP TO TABLE-IDX
END-IF
END-IF
IF WS-OCCURANCE-OF-INTERNAL-ZERO
THEN
IF WS-TSECTA-INTERNAL
THEN
SET WS-OCCURANCE-OF-INTERNAL TO TABLE-IDX
END-IF
END-IF
END-PERFORM
|
1. until the COBOL INTERNAL TABLE exceeds 256 occurances
it is actually faster to do a perform instead of search
2. since the table is only 26 items,
there is no need to delimit based on finding something,
as the extra time needed to pound and compare all 26 items is negligable.
3. at the end of the above perform loop, you will have both values,
if save-cusip-occurance GT zero then write that, you have saved the occurance,
if save-cusip-occurance equal zero, and save-internal-occurance gt zero,
then write that.
if both are zero, read next record.
doing two searches/perform loops is unnecessary.
and yes, ws-occurance-of-CUSIP-ZERO (and INTERNAL)
are level 88's on the save occurance comp fields. |
|
| Back to top |
|
|
Bill Woodger
Moderator Emeritus
Joined: 09 Mar 2011
Posts: 7309
Location: Inside the Matrix
|
|
|
|
pullaiah.cts, hope you are still attending.
With dbz's code you can see how something which can match your requirement doesn't have to be "ugly" and can be really rather attractive*.
I've not used THENs in Cobol, and don't like to see them used as the last thing on the line containing the IF, but dbz's layout works really well.
Even if you find yourself getting very, very tired typing all this by hand (in which case become "lazy", as put forward by Don, and invest some of your time in learning how to use the editor better) it saves time each time someone else has to look at the code (or when you do, after two weeks).
* relative to code |
|
| Back to top |
|
|
dbzTHEdinosauer
Global Moderator
Joined: 20 Oct 2006
Posts: 6966
Location: porcelain throne
|
|
|
|
a correction to my code is in order (thx Bill)
the two IF's
IF WP-TSECTA-CUSIP
and
IF WP-TSECTA-INTERNAL
must be:
IF WP-TSECTA-CUSIP (TABLE-IDX)
and
IF WP-TSECTA-INTERNAL (TABLE-IDX) |
|
| Back to top |
|
|
|
|
