Cobol - Re: My First C# (warning

Author

Re: My First C# (warning - long post)

andrewmcdonagh

2007-01-31, 6:55 pm

On Jan 31, 1:00 am, LX-i <lxi0...@netscape.net> wrote:
> Here 'tis. It looks like there's a little word-wrapping going on - I
> wasn't really attentive to ensuring that I didn't go past 80 on some lines.
>
> What this does...
> - Counts lines of code, along with "executable" and "commented" lines
> - Creates a cross-reference between programs and procs (copybooks).
> All our procs are named Annn-something (where A is a letter and nnn is a
> three-digit number). For programs, we parse copy statements, and for
> procs, we parse perform statements. (This gives us the ability, from
> our CM system, to generate a list of proc dependencies.)
> - Creates a cross-reference between programs/procs and reject codes.
> The system we support uses a table of reject codes with associate
> narratives. When a code is changed, all program owners who are affected
> must sign off. This xref enables that, as well as finding rejects that
> are not used by any programs - these numbers can be reused.
> - Creates a cross-references between programs/procs and called
> procedures. It can be used for many different things.
> - [TODO] Creates a cross-reference between programs/procs and database
> records/tables and fields/columns. (This is the missing "cull" stuff.)
>
> Some notes as well...
> - the "cull" stuff isn't done yet, so if the method has "cull" in the
> name, ignore it. (There are a few utility methods under the cull stuff.)
> - I've replaced our server names and passwords with placeholders, for
> obvious reasons.
> - I really, REALLY, *REALLY* dislike the auto-formatting that VS2005
> does with putting the braces on the line under the declaration.
> However, that's our shop standard, so that's what this looks like.
> - I've tried to comment things as I go along, but this isn't a
> finished product. If you see a spot where more comments would be
> helpful, feel free to identify them to me...
> - Having to have a "break" after the "default" case on a switch is
> BS... but I digress...
>
> Here we go - 824 lines....
>

Snipped code....

(Take these comments as good will ones, not character destroying..etc)

Ok so a quick scan through tells me that this class....

Whilst 'doing' one job of analysing the code files, is actually
several (currently) hidden classes working together. This is backed up
by the large number of IFs Switch statements, loops and repeated code.

The style of coding is more procedural than OO. This is evident when
we have large methods, containing multple 'if' blocks or/and 'Switch'
statements.

A general guideline to use for OO methods, is to keep their size to no
more than 10 lines (including whitespace, empty lines etc).

Lots of string literals embedded within the code, making the
maintenance of the program difficult. - These strings (esp db ones)
are better moved into a property file or constants.

However, all is not lost... I'll try and briefly show how using a
series of small changes, we can kepp your code compiling, running and
working, whilst slowly changing the design to be OO instead of
Procedural....

So lets start with the large methods... within your class there are
numerous occurrences of code duplication...

for example, several places you create an SQLCommand, insert some data
then close and dispose:

SqlCommand scCheck = new SqlCommand("....
... dbConn);

SqlDataReader drCheck = scCheck.ExecuteReader();
if (!drCheck.HasRows)
{ // It's not there - insert it
executeSQL("...");
}
drCheck.Close();
drCheck.Dispose();
scCheck.Dispose();

If you moved (aka Refactored) this chunk of code into its own method,
and then did the same with all other occurrences of similar code,
you'd soon see that there are numerous small private methods doing the
same basic job, just with different data.

Your IDE will even do the extraction for you.... highlight some code
with the mouse and right click, somewhere there will be a 'refactor'
menu option with something like 'extract method...' on it.

Once we have these small methods , we can then remove all of them but
One, and Pass It The Data to use... we do this, one method at a
time.

Another useful technique is to change inline comments into the names
of private methods, and move the code there...

time for another example...

// Update DML records in cull
protected void updateCullRecord(String ucrLine)
{

// --- CHECK #1 --- record name //
checkRecordName(...)

// --- CHECK #2 --- set name //
checkSetName(...)

}

Once again, select the code blocks to extract( the code between each
comment) and use your IDEs refactoring tool to extract method...

Once you have broken your methods down into sizes of no more than 10
lines, you will probably have 10, 20 or more little methods working
together to do the job as it currently does.

Now, the next step is to find the groupings of these methods that
naturally fit into their own class.

So for example.....

All of the db related methods are retrieving/inserting data, but
actually don't need to be part of the CodeStatistics class. Indeed,
moving those db related methods into their own class called
StatisticsModel? StatisticsDataSource? StatisticsGateway? (Gateway
is a design pattern...) means the Responsibility of getting and
updating the db is now in its own class, and that new class has only
one Responsibility.

time for a break....(aka kids playing up... ;-) )

Andrew

LX-i

2007-02-01, 3:55 am

andrewmcdonagh wrote:
>
> (Take these comments as good will ones, not character destroying..etc)

of course... :)

> Ok so a quick scan through tells me that this class....
>
> Whilst 'doing' one job of analysing the code files, is actually
> several (currently) hidden classes working together. This is backed up
> by the large number of IFs Switch statements, loops and repeated code.

Heh - I really thought I had eliminated repeated code. (In seeing your
revisions, I realized I hadn't.)

> The style of coding is more procedural than OO. This is evident when
> we have large methods, containing multple 'if' blocks or/and 'Switch'
> statements.

I wasn't really pleased with the size of some of the methods, but it
seemed silly to have 30 methods to do the same thing. I guess it's not
after all.

> A general guideline to use for OO methods, is to keep their size to no
> more than 10 lines (including whitespace, empty lines etc).
>
> Lots of string literals embedded within the code, making the
> maintenance of the program difficult. - These strings (esp db ones)
> are better moved into a property file or constants.

Are you referring here to the SQL stuff, or the element subtypes? (The
subtypes come from a database, and a bunch of other stuff is tied to
that, so I can't change them.)

> So lets start with the large methods... within your class there are
> numerous occurrences of code duplication...
>
> for example, several places you create an SQLCommand, insert some data
> then close and dispose:
>
> SqlCommand scCheck = new SqlCommand("....
> ... dbConn);
>
> SqlDataReader drCheck = scCheck.ExecuteReader();
> if (!drCheck.HasRows)
> { // It's not there - insert it
> executeSQL("...");
> }
> drCheck.Close();
> drCheck.Dispose();
> scCheck.Dispose();
>
> If you moved (aka Refactored) this chunk of code into its own method,
> and then did the same with all other occurrences of similar code,
> you'd soon see that there are numerous small private methods doing the
> same basic job, just with different data.

I hadn't thought about that. The other guy in our office has made a
control that he uses on his ASP.NET web pages, and it has an executeSQL
method. :)

> Another useful technique is to change inline comments into the names
> of private methods, and move the code there...

Hmmm... An interesting thought. So, maybe I'm thinking OO to a point,
but not brining the thing full circle?

> Now, the next step is to find the groupings of these methods that
> naturally fit into their own class.
>
> So for example.....
>
> All of the db related methods are retrieving/inserting data, but
> actually don't need to be part of the CodeStatistics class. Indeed,
> moving those db related methods into their own class called
> StatisticsModel? StatisticsDataSource? StatisticsGateway? (Gateway
> is a design pattern...) means the Responsibility of getting and
> updating the db is now in its own class, and that new class has only
> one Responsibility.

How would I do that? I've heard that over and over - "separate data
access from business logic". I guess I'm asking how I would go about
it. Would there be a bunch of methods in there, hiding the datareaders
with some other iterator?

See if this would be more OO - this would be a second class, one for the
database stuff. According to VS, this is syntactically sound...

// This class is responsible for database stuff
public class DBstuff {
static public SqlConnection newConnection() {
SqlConnection conn = new SqlConnection();
conn.ConnectionString = "[whatever]";
conn.Open();
return conn;
}
static public SqlDataReader newReader(String sql, SqlConnection conn) {
SqlCommand sc = new SqlCommand(sql);
return sc.ExecuteReader();
}
static public void closeReader(SqlDataReader sdr) {
sdr.Close();
sdr.Dispose();
}
static public void closeConnection(SqlConnection conn) {
conn.Close();
conn.Dispose();
}
}
// This class is responsible for telling if an item exists
public class ItemExistance : DBstuff {

ItemExistance() {
}

static public bool ItemExists(String element) {
bool itExists;
String sql = "SELECT element_id "
+ "FROM active_elements "
+ "WHERE element_id = '" + element + "'";
SqlConnection myConn = newConnection();
SqlDataReader dr = newReader(sql, myConn);
if (dr.HasRows) {
itExists = true;
} else {
itExists = false;
}
closeReader(dr);
closeConnection(myConn);
return itExists;
}
}

I'm still not sure how to separate the data readers... Would the method
just return a data reader, and the other method could run it as it does
today?

More to come...

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~
~ / \ / ~ Live from Montgomery, AL! ~
~ / \/ o ~ ~
~ / /\ - | ~ daniel@thebelowdomain ~
~ _____ / \ | ~ http://www.djs-consulting.com ~
~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~
~ GEEKCODE 3.12 GCS/IT d s-:+ a C++ L++ E--- W++ N++ o? K- w$ ~
~ !O M-- V PS+ PE++ Y? !PGP t+ 5? X+ R* tv b+ DI++ D+ G- e ~
~ h---- r+++ z++++ ~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~

"Who is more irrational? A man who believes in a God he doesn't see, or
a man who's offended by a God he doesn't believe in?" - Brad Stine

Sponsored Links