2006-09-01

Refactoring in action

Gosh, I'm all beat up.

I started to hack up a small piece of an editor:
BOOL RichEditDlg::FillTemplateFromDocum(const CDocInfo &doc)
{
FINDTEXTEX ft; CMUFString str;

ft.chrg.cpMin=0; ft.chrg.cpMax=-1;

for(;;)
{
ft.lpstrText="{TAG";

long iPos=m_RichCtrl.FindText(0,&ft);
if(-1==iPos) break;

ft.lpstrText="}"; ft.chrg.cpMin=ft.chrgText.cpMin;

long iPos2=m_RichCtrl.FindText(0,&ft);
if(-1==iPos2) continue;

ft.chrgText.cpMin=iPos;

m_RichCtrl.SetSel(ft.chrgText);

str=m_RichCtrl.GetSelText();
str.Replace("{TAG",""); str.Replace("}","");

int iFld=atoi(str), iVol=1;
if(0==iFld) continue;

int iPos3=str.FindOneOf(".,");
if(iPos3>0) iVol=atoi(str.Mid(iPos3+1));

CString sTmp;
iPos3=str.Find("_");

if(iPos3>0) sTmp=str.Mid(iPos3+1); else str.Empty();

CDocInfo::stFieldInfo fi; doc.GetFieldInfo(iFld,fi);
str=doc.GetValue(iFld,iVol);

if(sTmp=="PROP") // Value in lower case
{
if(0==fi.sFieldType.Left(5).CompareNoCase("NUMBER"))

str=str.FormatSum();
else if(0==fi.sFieldType.Left(4).CompareNoCase("DATE"))

str=str.DateToScript();
else if(0==fi.sFieldType.Left(4).CompareNoCase("CHAR"))

{
CString sTmp1=fi.sFieldName; sTmp.MakeLower();

if(sTmp1.Find("address"))
str=AddrToTxt(str);

}
}
m_RichCtrl.ReplaceSel(str);
}

}


Someone can argue, but IMHO the code terribly smells.
First of all, sort out nameless constants.
const CHARRANGE FULLTEXT = {0,-1};
ft.chrg = FULLTEXT;

Go further, get rid of the implicit modifying operations.
Starting from the passing and returning to the separate function the whole structure FULLTEXT retain only the necessary and get:
const CHARRANGE NOTFOUND = {-1,-1};

CHARRANGE lookup(CRichEditCtrl &richEdit, const CHARRANGE &searchRange, const char* lookupString) {

FINDTEXTEX findData;
findData.chrg = searchRange;
findData.lpstrText = const_cast<char*>(lookupString);

if(richEdit.FindText(FR_MATCHCASE,&findData))
return findData.chrgText;

return NOTFOUND;
}

bool operator==(const CHARRANGE &cr1, const CHARRANGE &cr2) {

return cr1.cpMin == cr2.cpMin && cr1.cpMax == cr2.cpMax;

}

Fix a small piece
/*
ft.lpstrText="{TAG";
long iPos=m_RichCtrl.FindText(0,&ft);
if(-1==iPos) break;*/

result = lookup(m_RichCtrl,FULLTEXT,"{TAG");

if(NOTFOUND == result)
break;
long iPos = result.cpMin;

The next piece
/*
ft.lpstrText="}"; ft.chrg.cpMin=ft.chrgText.cpMin;
long iPos2=m_RichCtrl.FindText(0,&ft);
if(-1==iPos2) continue;
ft.chrgText.cpMin=iPos;
m_RichCtrl.SetSel(ft.chrgText);*/

CHARRANGE lookform = {result.cpMin,-1};

result = lookup(m_RichCtrl,lookform,"}");
if(NOTFOUND == result)

continue;

CHARRANGE selectionRange = {iPos,result.cpMax};

m_RichCtrl.SetSel(selectionRange);

A little bit long and terrifying but now it is clear what is really happening here.

And one:
CHARRANGE startTag = lookup(m_RichCtrl,FULLTEXT,"{TAG");

if(NOTFOUND == startTag)
break;
long iPos = startTag.cpMin;


CHARRANGE lookform = {startTag.cpMin,-1};
CHARRANGE endTag = lookup(m_RichCtrl,lookform,"}");

if(NOTFOUND == endTag)
continue;

CHARRANGE selectionRange = {startTag.cpMin,endTag.cpMax};

m_RichCtrl.SetSel(selectionRange);


And two:
CHARRANGE lookupFrom(CRichEditCtrl &richEdit, int startPos, const char* lookupString);

CHARRANGE endTag = lookupFrom(m_RichCtrl,startTag.cpMin,"}");

if(NOTFOUND == endTag)
continue;

CHARRANGE selectionRange = {startTag.cpMin,endTag.cpMax};

m_RichCtrl.SetSel(selectionRange);


The next piece.
/*str=m_RichCtrl.GetSelText();
str.Replace("{TAG",""); str.Replace("}","");
int iFld=atoi(str), iVol=1;*/

CString getTagContent(CRichEditCtrl &richEdit,const CHARRANGE &startTag, const CHARRANGE &endTag) {

CHARRANGE selectionRange = {startTag.cpMin,endTag.cpMax};

richEdit.SetSel(selectionRange);

CString tagContent=richEdit.GetSelText();


tagContent.Replace("{TAG","");
tagContent.Replace("}","");

return tagContent;
}


str = getTagContent(m_RichCtrl,startTag,endTag);

int iFld=atoi(str);
int iVol=1;


I'm tired and doing careless work - commenting.
/*
int iPos3=str.FindOneOf(".,");
if(iPos3>0) iVol=atoi(str.Mid(iPos3+1));
CString sTmp;
iPos3=str.Find("_");
if(iPos3>0) sTmp=str.Mid(iPos3+1); else str.Empty();
*/

//Define additional code
int iPos3=tagContent.FindOneOf(".,");
if(iPos3>0) iVol=atoi(tagContent.Mid(iPos3+1));

//Define suffix
CString sTmp;
iPos3=tagContent.Find("_");

if(iPos3>0)
sTmp=tagContent.Mid(iPos3+1);

else
tagContent.Empty();


suffix proceeding goes immediately to the end
void replaceTag(
CRichEditCtrl &richEdit,
const CHARRANGE &startTag,
const CHARRANGE &endTag,

const char* newContent) {
CHARRANGE selectionRange = {startTag.cpMin,endTag.cpMax};

richEdit.SetSel(selectionRange);
richEdit.ReplaceSel(newContent);

}

replaceTag(startTag,endTag,str);



God damn my laziness, but to write tests for code processing 10 global structures is problematical, so I test it manually.

Before the very running I've noticed the error, fix it
CHARRANGE startTag,endTag;
for(int curPos = 0;;curPos=endTag.cpMax)


It works. Before further improvements can be done we should generalize work with RichEdit
class TextRange {
public:
TextRange()
:startPos(-1),endPos(-1) {};

TextRange(int start, int end)
:startPos(start),endPos(end){}

int startPos;
int endPos;

friend bool operator==(const TextRange&, const TextRange&);

static const TextRange NOTFOUND;
static const TextRange FULLTEXT;

};


class ISourceContainer
{
public:
ISourceContainer();

virtual ~ISourceContainer();

virtual TextRange lookupFrom(
int startPos,
const char* lookupString) = 0;

virtual std::string getTagContent(

const TextRange &startTag,
const TextRange &endTag) = 0;

virtual void replaceTag(

const TextRange &startTag,
const TextRange &endTag,
const char* newContent) = 0;

};

Inherit from it a class with the constructor
RichEditSourceContainder(CRichEditCtrl &richEdit)
and transfer there our functions realization.

Ensure that everything works do one more extract method and get:
RichEditSourceContainer sourceContainer(m_RichCtrl);

TextRange startTag,endTag;
for(int curPos = 0;;curPos=endTag.endPos)

{
startTag = sourceContainer.lookupFrom(curPos,"{MIF");

if(TextRange::NOTFOUND == startTag)
break;
endTag = sourceContainer.lookupFrom(startTag.startPos,"}");

if(TextRange::NOTFOUND == endTag)
break;

CString tagContent = sourceContainer.getTagContent(startTag,endTag).c_str();


//define field number
int iFld=atoi(tagContent);
if(0==iFld)
continue;

//define nesting
int iVol=1;
int iPos3=tagContent.FindOneOf(".,");

if(iPos3>0)
iVol=atoi(tagContent.Mid(iPos3+1));

//define suffix
CString sTmp;
iPos3=tagContent.Find("_");

if(iPos3>0)
sTmp=tagContent.Mid(iPos3+1);
else
tagContent.Empty();

CString fieldValue=doc.GetValue(iFld,iVol);
if(sTmp=="PROP") // Value in upper case

fieldValue = getFieldTextValue(doc,iFld,iVol);
//replace tag with field value

sourceContainer.replaceTag(startTag,endTag,fieldValue);
}



That's all, I'm tired, it's Friday.

On the next stage I'll move this piece out from the dialog to the model... Gosh!

No comments:

Post a Comment