[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] 'kay, todo #1 is "done"
From:       tech () bishop ! dhs ! org
Date:       2002-03-06 5:06:54
[Download RAW message or body]

On Wed, Mar 06, 2002 at 03:55:35AM +0000, Rik Hemsley wrote:
> #if tech@bishop.dhs.org
> > I quoted the "done", as you guys are undoutably going to laugh at my
> > pitiful attempts at coding.  And then my feelers will get hurt, I'll
> > take my ball and go home, and cry for a little bit.  But don't worry,
> > I'll get over it, I'm tough like that.  Without further ado, here's the
> > code:
> > int saveAsXML(const QString &fileName,const char *memoData)
> > {
> > 	QFile f( fileName );
> > 	QTextStream stream( &f );
> > 	if ( f.open(IO_WRiteOnly) ) {
> > 		QString s = memoData;
> > 		stream << "<memo pilotid=\"" << "record->id" << "\">\n" << endl;
> > 		stream << "<title>" << "record->titlestring" << "</title>\n" << endl;
> > 		stream << s << endl;
> > 		stream << "</memo>\n" << endl;
> > 	}
> > 	f.close();
> > 	return 0;
> > }
> 
> Ok, hope you don't mind if I give some opinions on the code. I think it
> probably does what it's supposed to (apart from that it wouldn't
> compile, due to that type on the f.open line :) - but as you say you're
> learning coding, perhaps this might help...

Yes! This is *exactly* the sort of stuff I was looking for.

> 1. You are returning an int. Why ? If you can fail due to not being able
>    to open the file, and nothing else exciting can happen, return a
>    bool. And make sure to return false if you can't open the file.

I returned an int cuz that's what I typed :-P  I'll change that...

> 2. The less indentation, the easier to read. Suggestion:
> 
>    if (!f.open(IO_WriteOnly))
>      return false;
> 
>    instead of:
> 
>    if (f.open(IO_WriteOnly))
>    {
>      ...
>    }

A lot of this indentation is weird because I was 1/2 copy/pasting, and
1/2 typing, due to weird "tab all the way off the screen" stuff
happening between kdevelop and vim (mutt).  That also accounts for the
typo :-)

> 3. Don't use const char * unless you absolutely have to. Use QString
>    or QCString.

That was based soley off of what Adriaan wrote in the TODO file.  That's
what he said to make the subroutine take, so that's what it takes :-)
I'll leave it up to him to defend/change that.

> 4. Use QDomDocument to construct XML. More code, less chances for erro.r

This is probably a good idea.  However, it looked to take a deeper
understanding of Q* than I currently possess.  I'll look into it again.

> 5. \n == endl on UNIX, so sending \n then endl will give 2 newlines. See 4.

Sorry, remnant from a version where I did everything via printf's.  Will
take it out.

> 6. QFile is closed on destruction.

Does it hurt to close it explicitly?  I ask, as I copied that straight
from some tutorial/example code, and that's what they had B-)

> 7. You copy memoData into a temporary variable for no reason.

Well, as long as it's some char pointer, I don't really know exactly
what I can do with it.  This is soley because I'm a newbie programmer,
nothing explicitly wrong with using a char.  By converting it into a 
QString, it "fit" better with the other stuff I was doing, and I could
match it with the examples I was using from docs.trolltech.no. This would 
be clarified/dropped if Adriaan wants to switch to using a QString vs. 
the char *.  Or, if you convince me it isn't needed (wouldn't be that
hard: just repeat what you said last time :-).

> 8. I have no idea why record->id and record->titlestring are quoted - 
>    are they meant to be written out as strings like that ?

Yes, in order to make it compile in my test program :-)  This snippet is
meant to be incorporated into KPilot, which provides the kpilot object,
along with those accompanying methods.  However, I have no desire to
create an object and implement those methods simply to see if it will
write to a file.  So, I put in the proper code, but surrounded it by
quotes, so when it *is* put in KPilot proper, all you should have to do
is "unquote" it, and *bam*, everything works B-)  You'll notice I
seperated them off into their own << sections, so as to make it as
painless as possible.

Thank you very much for this!  I'm about to head to bed, but I'll make
those changes in the morning, and send an update to the list.  I don't
know if I've mentioned enough, but I'm really new to this cra^H^H^Hstuff
and have a tendency to do things in an "odd" manner.  But, as they say,
the only way to learn is be riducled by lots of people, so.... B)

Oh, and the subject of my mail was wrong, as I forgot step 2: implement
a KConfig thingy.  Maybe if this part works out, I'll dive into that.
W00t!

D.A.Bishop

[Attachment #3 (application/pgp-signature)]
_______________________________________________
kde-pim mailing list
kde-pim@mail.kde.org
http://mail.kde.org/mailman/listinfo/kde-pim
kde-pim home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread]