SetCaption function seems inconsistent

[cyberbliss]

Hi

Although the SetCaption function specifies the actual text as a variadic argument, only the first element is actually used/displayed due to the IF test in the actual function, and if there is more than one element the entire captionText arg is ignored. Is this deliberate?

func (t *Table) SetCaption(caption bool, captionText ...string) {
	t.caption = caption
	if len(captionText) == 1 {
		t.captionText = captionText[0]
	}
}

[hbobenicio]

I've asked myself the same question a few minutes ago, when I looked at this function signature.

after some thoughts, I think maybe the author of this function thought in allowing setting the caption visibility with the same function, like this:

table.SetCaption(true) // or table.SetCaption(false)

then it will just set t.caption which configures the caption visibility.

IMHO mixing two behaviours in just one API function is kinda awkward. An API design smell.

Here is what I think that would be a better design:

1. Rename Table.caption to Table.showCaption (better semantics about its use purpose)
2. Refactor Table.SetCaption into 2 functions:

func (t *Table) SetCaption(text string) {
    t.captionText = text
}

// or maybe `SetCaptionVisibility`, if the `Set` prefix is really a must.
func (t *Table) ShowCaption(show bool) {
    t.showCaption = show
}

then you would have

// if you want to set the caption text
table.SetCaption("My Caption") // no awkward bool and variadic text

// if you want to set the caption visibility
table.ShowCaption(false)

Another possibility is to encapsulate all fields related to the caption in its own type

type Caption struct {
    text string
    show bool
}

type Table struct {
    // other fields...
    caption Caption
    // other fields...
}

// how about that?
table.Caption().SetText("My Caption Text")
table.Caption().Show(true)

Does it make sense?

Anyway... both suggestions would break API. Is it a problem or could it be a plan for the next release?