DRY, SoC, IoC, KISS... (NOT)
-
Do you have a point?
I thought his subject line made it quite plain what he regarded as wrong here: Don't Repeat Yourself - There's a lot of copy-and-paste style repetition all over that method. Separation of Concerns - This one method seems to cover a lot of ground. Inversion of Control - This looks like classical procedural code to me. Keep It Simple Stupid - 'nuff said. Generally, any method over 20-30 lines should often be regarded as suspect. This is a true horror.
-
I thought his subject line made it quite plain what he regarded as wrong here: Don't Repeat Yourself - There's a lot of copy-and-paste style repetition all over that method. Separation of Concerns - This one method seems to cover a lot of ground. Inversion of Control - This looks like classical procedural code to me. Keep It Simple Stupid - 'nuff said. Generally, any method over 20-30 lines should often be regarded as suspect. This is a true horror.
-
Do you have a point?
-
I thought his subject line made it quite plain what he regarded as wrong here: Don't Repeat Yourself - There's a lot of copy-and-paste style repetition all over that method. Separation of Concerns - This one method seems to cover a lot of ground. Inversion of Control - This looks like classical procedural code to me. Keep It Simple Stupid - 'nuff said. Generally, any method over 20-30 lines should often be regarded as suspect. This is a true horror.
No need to post the whole bloody thing.
-
No need to post the whole bloody thing.
You're right, PIEBALDconsult. Excuse me for this, okay? (Sorry, guys!) I wanted to give a dramatic effect and, as a big fan of Tarantino, exaggerate the amount of blood. :) I have this code posted on a blog on wordpress. You think I should edit my post and replace the entire code for a link pointing to the post in wordpress?
Marcelo Palladino Brasil
-
This code is based on true and real CRITICAL application. **In memory
private void ProcessaNota(TextReader txt, string cDestino)
{
string baseDir = InfoApp.PastaSchemas() + "\\nfe_v1.10.xsd";if (!File.Exists(baseDir)) { this.cMensagemErro += "Arquivo: " + baseDir + " não encontrado" + Environment.NewLine; return; } DataSet dsNfe = new DataSet(); dsNfe.ReadXmlSchema(baseDir); dsNfe.EnforceConstraints = false; DataRow dremit = dsNfe.Tables\["emit"\].NewRow(); DataRow drdest = dsNfe.Tables\["dest"\].NewRow(); DataRow drPISOutr = null; DataRow drPISST = null; DataRow drCOFINSOutr = null; DataRow drCOFINSST = null; DataRow drtransporta = null; DataRow drIPITrib = null; DataRow drVol = null; string idprod = ""; int iControle = 1; int nElementos; int iLeitura; string\[\] dados; Int64 iTmp = 0; bool vNovaNota = false; bool vTiraxFant = false; bool transpAdd = false; this.nNF = 0; int DIid = 0; int prodID = 0; int idcomb = 0; int volid = 0; int indadicid = 0; while (cLinhaTXT != null) { cLinhaTXT = this.ConvertToOEM(this.cLinhaTXT); //cLinhaTXT += "!@#$%^&\*()\_+"; bool reLe = false; for (int x = 0; x < cLinhaTXT.Length - 1; ++x) if (/\*char.IsSymbol(cLinhaTXT, x) ||\*/ char.IsControl(cLinhaTXT, x)) { this.cMensagemErro += "Linha \[" + this.iLinhaLida.ToString() + "\] coluna \[" + (x + 1).ToString() + "\] contem o caracter \[" + cLinhaTXT.Substring(x, 1) + "\] que não é permitido" + Environment.NewLine; //this.cMensagemErro += "\\t"+cLinhaTXT + Environment.NewLine; cLinhaTXT = txt.ReadLine(); iLinhaLida++; reLe = true; break; } if (reLe) continue; dados = cLinhaTXT.Split('|'); dados\[0\] = dados\[0\].ToUpper(); nElementos = dados.GetLength(0) - 1; for (int n = 0; n < nElementos; ++n) dados\[n\] = dados\[n\].Trim(); #region -- Segmentos switch (dados\[0\]) { case "NOTAFISCAL": case "NOTA FISCAL": break; case "A": #region -- A if (this.nNF > 0) {**Hey don't you have source control? No need to dump your code here ;P
Yusuf May I help you?
-
Hey don't you have source control? No need to dump your code here ;P
Yusuf May I help you?
Good idea! I hadn't thought of that... use CP as an offsite backup facility. Hmmm. I wonder if the hamsters will complain...
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
-
Do you have a point?
That looks like at least a few dozens of function points to me.
Agh! Reality! My Archnemesis![^]
| FoldWithUs! | sighist | WhoIncludes - Analyzing C++ include file hierarchy -
Hey don't you have source control? No need to dump your code here ;P
Yusuf May I help you?
-
This code is based on true and real CRITICAL application. **In memory
private void ProcessaNota(TextReader txt, string cDestino)
{
string baseDir = InfoApp.PastaSchemas() + "\\nfe_v1.10.xsd";if (!File.Exists(baseDir)) { this.cMensagemErro += "Arquivo: " + baseDir + " não encontrado" + Environment.NewLine; return; } DataSet dsNfe = new DataSet(); dsNfe.ReadXmlSchema(baseDir); dsNfe.EnforceConstraints = false; DataRow dremit = dsNfe.Tables\["emit"\].NewRow(); DataRow drdest = dsNfe.Tables\["dest"\].NewRow(); DataRow drPISOutr = null; DataRow drPISST = null; DataRow drCOFINSOutr = null; DataRow drCOFINSST = null; DataRow drtransporta = null; DataRow drIPITrib = null; DataRow drVol = null; string idprod = ""; int iControle = 1; int nElementos; int iLeitura; string\[\] dados; Int64 iTmp = 0; bool vNovaNota = false; bool vTiraxFant = false; bool transpAdd = false; this.nNF = 0; int DIid = 0; int prodID = 0; int idcomb = 0; int volid = 0; int indadicid = 0; while (cLinhaTXT != null) { cLinhaTXT = this.ConvertToOEM(this.cLinhaTXT); //cLinhaTXT += "!@#$%^&\*()\_+"; bool reLe = false; for (int x = 0; x < cLinhaTXT.Length - 1; ++x) if (/\*char.IsSymbol(cLinhaTXT, x) ||\*/ char.IsControl(cLinhaTXT, x)) { this.cMensagemErro += "Linha \[" + this.iLinhaLida.ToString() + "\] coluna \[" + (x + 1).ToString() + "\] contem o caracter \[" + cLinhaTXT.Substring(x, 1) + "\] que não é permitido" + Environment.NewLine; //this.cMensagemErro += "\\t"+cLinhaTXT + Environment.NewLine; cLinhaTXT = txt.ReadLine(); iLinhaLida++; reLe = true; break; } if (reLe) continue; dados = cLinhaTXT.Split('|'); dados\[0\] = dados\[0\].ToUpper(); nElementos = dados.GetLength(0) - 1; for (int n = 0; n < nElementos; ++n) dados\[n\] = dados\[n\].Trim(); #region -- Segmentos switch (dados\[0\]) { case "NOTAFISCAL": case "NOTA FISCAL": break; case "A": #region -- A if (this.nNF > 0) {**I think that this piece of code is adorable (in a coding horror kind of way). Reminds me of a scene in Tarantino's Kill Bill pt.1, the one where Uma Thurman was slaughtering bad guys in Japan ))) I've really enjoyed all these cases in the code, but it is the size of this method that makes me dizzy. I still wonder how the PM or co-workers haven't done something before this method grow that big.
-
I think that this piece of code is adorable (in a coding horror kind of way). Reminds me of a scene in Tarantino's Kill Bill pt.1, the one where Uma Thurman was slaughtering bad guys in Japan ))) I've really enjoyed all these cases in the code, but it is the size of this method that makes me dizzy. I still wonder how the PM or co-workers haven't done something before this method grow that big.
makumazan84, when PIEBALDconsult talked about blood, I thought that exact scene you described. Kill Bill is a masterpiece ... and the code, of a very particular way, it is also. As you get dizzy, I think in Visual Studio that could be solved with several sections #region. :cool:
Marcelo Palladino Brasil
-
:sigh: This sort of horror is quite normal, but seldom are we given an example that we DON'T have to read...and thank you, those who did so! :laugh:
-
makumazan84, when PIEBALDconsult talked about blood, I thought that exact scene you described. Kill Bill is a masterpiece ... and the code, of a very particular way, it is also. As you get dizzy, I think in Visual Studio that could be solved with several sections #region. :cool:
Marcelo Palladino Brasil
hmmm...in VS2003 and in VS2008SP1, you can't define #regions within a function body
-
hmmm...in VS2003 and in VS2008SP1, you can't define #regions within a function body
Dammit! :-)
Marcelo Palladino Brasil Twitter: @ProgrammerHead Blog (pt-BR): http://programmerhead.wordpress.com
-
Dammit! :-)
Marcelo Palladino Brasil Twitter: @ProgrammerHead Blog (pt-BR): http://programmerhead.wordpress.com
Palladino wrote:
Dammit!
Yeah, I had the same reaction the first time I wanted to use it, some time about two years ago. I dislike large function bodies, but realistically there are a number of situation-specific algorithms that depend on a long sequence of unrepeated operations. To reduce the size, one is left with the option of breaking them into smaller chunks as subroutines, then calling each subroutine in sequence. I've seen differences in how well that approach works when maintenance is being performed by novices to the software. I've seen genuinely intense frustration expressed by some developers when they have to nest down through two or more levels of subroutine that were simply put in to eliminate very lengthy code sequences, where the developer in question felt it would have been much easier to follow the sequence in its original lengthy form. Allowing collapsible regions within a subroutine brings the best of both techniques to bear on the problem.
