On 19 Sep 2000, Lars Gullik Bjønnes wrote:
> Marko Vendelin <[EMAIL PROTECTED]> writes:
>
> | Hi!
>
> I don't like this code:
> + // setting sizes of the widgets
> + string path;
> + string w, h;
> + path = PACKAGE "/" + LOCAL_CONFIGURE_PREFIX;
> +
> + w = path + "/" + CONF_PANE_INFO + CONF_PANE_INFO_DEFAULT;
>
> it should be writen as:
>
> // setting sizes of the widgets
> string path = PACKAGE "/" + LOCAL_CONFIGURE_PREFIX;
> string w = pat + "/" + CONF_PANE_INFO + CONF_PANE_INFO_DEFAULT;
>
> Where is h used?
fixed. h was used in former FormCitation implementation.
> + int i, sz;
> + for (i = 0, sz = clist_bib_->columns().size(); i < sz; ++i)
> + {
> + w = path + "/" + CONF_COLUMN + "_" + tostr(i) + CONF_COLUMN_DEFAULT;
> + clist_bib_->column(i).set_width( gnome_config_get_int(w.c_str()) );
> + }
> +
>
> int const sz = clist_bib_->columns().size();
> for (int i = 0; i < sz; ++i) {
> ...
> }
>
> Would also suit me a lot better.
>
>
> + for ( i = 0, sz = blist.size(); i < sz; ++i )
> + {
> + bibkeys.push_back(blist[i].first);
> + bibkeysInfo.push_back(blist[i].second);
> + }
> +
>
> sz = blist.size(); // remove const from the prev sz
> for (int i = 0; i < sz; ++i) {
> ...
> }
>
>
> The same "bugs" are done in other blaces as well.
I'll look for them in my code.
> The comments can be looked at as nit-picking, but is more (IMO) a case
> of good style and clearity.
>
> And remember: const const const! (and that goes for all of you)
>
> Found a strange one:
>
> + contents += clist_bib_->selection().operator[](i).operator[](1).get_text();
> + }
>
> Why can't that be written as "clist_bib->selection[i][1].get_text();" ?
>
>
> + lv_->getLyXFunc()->Dispatch( LFUN_CITATION_INSERT,
> + params.getAsString().c_str() );
>
> Get rid of the c_str() to Dispatch.
>
> + string tmp, stext( search_string_ );
>
> Please one variable one line!
>
> string tmp;
> string stext(searc_string_);
>
>
> lv_->getLyXFunc()->Dispatch( LFUN_INDEX_INSERT,
> params.getAsString().c_str() );
> }
>
> beware of c_str to Dispatch.
>
> + // update lists
> + if (toc_.size() > 0)
> + {
>
> Use
> // update lists
> if (!toc_.empty()) {
>
> instead.
>
> +void Menubar::Pimpl::updateList(vector<Buffer::TocItem> * toclist,
>vector<ListsHolder> * pgui)
> +{
>
> Pass by reference instead. If const is possible use const.
>
This will go as bind<> argument later. As far as I know we cannot use
references as slot or bind arguments yet.
> + vector<pair<string,string> > names =
> + viewable
> + ? Exporter::GetViewableFormats(owner_->buffer())
> + : Exporter::GetExportableFormats(owner_->buffer());
> +
> + for (vector<pair<string,string> >::const_iterator cit = names.begin();
> + cit != names.end()
>
> some typedef for vector<pair<string, string> > would be handy.
Lars, thank you very much for looking into the code! I'll scan the rest of
my code for these "bugs"
Marko
