Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote a simple generic intercalate function (that should be semantically equivalent to the Haskell intercalate).

template <typename Fwd_It, typename Out_It>
    auto intercalate(Fwd_It first, Fwd_It last, Out_It destFirst,
                     Fwd_It whatFirst, Fwd_It whatLast) {
        if (first == last) return destFirst;
        *destFirst = *first;
        ++first;
        ++destFirst;

        while (first != last) {
            std::copy(whatFirst, whatLast, destFirst);
            *destFirst = *first;
            ++first;
        }
        return destFirst;
    }

It appears to be working with what I've tested with (empty ranges, different iterator-supporting ranges, different types, etc.).

However, is this:

  • generic enough?
  • clean enough?
  • fast enough?
share|improve this question
    
Could you add an example od use and/or a link to the Haskell documentation of the function? It would be quite helpful :) –  Morwenn Jun 18 at 19:27

2 Answers 2

I would not cram in the if statement like that:

auto intercalate(Fwd_It first, Fwd_It last, Out_It destFirst,
                 Fwd_It whatFirst, Fwd_It whatLast) {
    if (first == last) return destFirst;
    *destFirst = *first;
    // ...

While this style is okay at times, it may not be too readable in all situations. Single-line statements can still use curly braces, especially if it may need to be maintained. You could also add a new line before the line that follows it.

auto intercalate(Fwd_It first, Fwd_It last, Out_It destFirst,
                 Fwd_It whatFirst, Fwd_It whatLast) {
    if (first == last) {
        return destFirst;
    }

    *destFirst = *first;
    // ...
share|improve this answer
  • it looks like you are missing ++destFirst in the loop body (or have a stray ++destFirst in the introductory part?).

  • if I understand Haskell semantics, *first is itself a list. *destFirst = *first; doesn't seem to be enough.

  • I'd try to implement intercalate in terms of concat and intersperse, which seem to be important enough to be implemented separately.

  • destFirst doesn't look right (First implies Last); just dest is a better choice.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.