Bufor cykliczny - review :: 4programmers.net

pele2525

2017-07-17T15:49:37+02:00

Rejestracja: dni
Ostatnio: dni
Postów: 19

Witam!
Chciałbym by ktoś z bardziej doświadczonych kolegów zrobił mi review mojego kodu. Jest to implementacja bufora cyklicznego, który ma mi posłużyć do zbierania logów.

Kopiuj

#ifndef ROUNDBUFFER_H
#define ROUNDBUFFER_H

#include <iostream>
template <typename T>

class RoundBuffer
{
public:
    RoundBuffer() = delete;
    explicit RoundBuffer(std::size_t size);
    RoundBuffer(const RoundBuffer<T>& rhs);
    RoundBuffer& operator=(RoundBuffer<T> copy);
    RoundBuffer& operator=(RoundBuffer<T>&& copy);
    RoundBuffer(RoundBuffer<T> && rhs);
    ~RoundBuffer();
    T const& operator[](std::size_t index) const;
    void pushObject(const T& object);
    void resize(std::size_t newSize);
    std::size_t getSize() const;

    template <typename Y>
    friend void swap(RoundBuffer<Y>& lhs, RoundBuffer<Y>& rhs) noexcept;

    template <typename Y>
    friend std::ostream& operator<<(std::ostream& stream, const RoundBuffer<Y>& object);

private:
    std::size_t index;
    std::size_t size;
    T* buffer;
    void writeToBuffer(std::size_t& index, std::size_t& size, T* buffer, const T& data);
};

template <typename T>
RoundBuffer<T>::RoundBuffer(std::size_t size) : index(0), size(size), buffer(size ? new T[size] : nullptr) {}

template <typename T>
RoundBuffer<T>::RoundBuffer(const RoundBuffer<T> &rhs) : index(rhs.index), size(rhs.size), buffer(size ? new T[size] : nullptr)
{
    std::copy(rhs.buffer, rhs.buffer + size, buffer);
}

template <typename T>
RoundBuffer<T>& RoundBuffer<T>::operator =(RoundBuffer<T> copy)
{
    swap(*this, copy);
    return *this;
}

template <typename T>
RoundBuffer<T>& RoundBuffer<T>::operator =(RoundBuffer<T>&& copy)
{
    swap(*this, copy);
    return *this;
}

template <typename T>
RoundBuffer<T>::RoundBuffer(RoundBuffer<T> && rhs)
{
    swap(*this,rhs);
    rhs.buffer = nullptr;
}

template <typename T>
RoundBuffer<T>::~RoundBuffer()
{
    if(buffer) delete[] buffer;
}

template <typename T>
T const& RoundBuffer<T>::operator [](std::size_t index) const
{
    if(index > this->index)
    {
        throw std::out_of_range("Index is out of possible range");
    }
    else
    {
        return buffer[index];
    }

}

template <typename T>
std::size_t RoundBuffer<T>::getSize() const
{
    return size;
}

template <typename T>
void RoundBuffer<T>::writeToBuffer(std::size_t& index, std::size_t& size, T *buffer, const T& data)
{
    if (index != size)
    {
        buffer[index] = data;
        index++;
    }
    else
    {
        index = 0;
        buffer[index] = data;
        index++;
    }
}
template <typename T>
void RoundBuffer<T>::pushObject(const T &object)
{
    writeToBuffer(index, size, buffer, object);
}

template <typename T>
void RoundBuffer<T>::resize(std::size_t newSize)
{
    if(newSize == size) return;
    T* newBuffer = newSize ? new T[newSize] : nullptr;
    std::copy(buffer,buffer+size,newBuffer);
    T* oldBuffer = this->buffer;
    this->buffer = newBuffer;
    this->size = newSize;
    delete[] oldBuffer;
}
template <typename T>
std::ostream& operator<<(std::ostream& stream, const RoundBuffer<T>& object)
{
    {
        for (std::size_t i = 0; i < object.size; ++i)
        {
            stream << "buff[" << i << "] = " << object.buffer[i] << std::endl;
        }
        return stream;
    }
}

template <typename T>
void swap(RoundBuffer<T>& lhs, RoundBuffer<T>& rhs) noexcept
{
    using std::swap; //enable ADL
    swap(lhs.buffer, rhs.buffer);
    swap(lhs.index, rhs.index);
    swap(lhs.size, rhs.size);
}


#endif // ROUNDBUFFER_H

kq

2017-07-17T16:27:55+02:00

Rejestracja: dni
Ostatnio: dni
Lokalizacja: Szczecin

Kopiuj


#include <iostream>
template <typename T>
 
class RoundBuffer

bardzo dziwne bindowanie do include'a.

Kopiuj

    RoundBuffer(const RoundBuffer<T>& rhs);
    RoundBuffer& operator=(RoundBuffer<T> copy);
    RoundBuffer& operator=(RoundBuffer<T>&& copy);
    RoundBuffer(RoundBuffer<T> && rhs);

Tutaj aż się prosi o użycie wektora i rule of zero (alternatywnie o wydzielenie osobnej klasy do trzymania tablicy, jeśli wektor za duży)

Kopiuj

    T const& operator[](std::size_t index) const;

A wersja mutable gdzie?

Kopiuj

    void pushObject(const T& object);

pop? Front/back?

Kopiuj

    template <typename Y>
    friend void swap(RoundBuffer<Y>& lhs, RoundBuffer<Y>& rhs) noexcept;

Dlaczego szablon po Y jest friendem dla T?

Kopiuj

template <typename T>
class RoundBuffer;

template <typename T>
void swap(RoundBuffer<T>& lhs, RoundBuffer<T>& rhs) noexcept;

template <typename T>
class RoundBuffer
{
    // ...
    friend void swap<>(RoundBuffer& lhs, RoundBuffer& rhs) noexcept;
    // ...
};

Zrobiłbym tak ^

Kopiuj

    template <typename Y>
    friend std::ostream& operator<<(std::ostream& stream, const RoundBuffer<Y>& object);

Raczej zbyteczne, lepiej wystaw iteratory i pozwól użytkownikowi samemu się bawić. + to samo odnośnie frienda dla szablonu po Y zamiast T

Kopiuj

template <typename T>
void RoundBuffer<T>::writeToBuffer(std::size_t& index, std::size_t& size, T *buffer, const T& data)
{
    if (index != size)
    {
        buffer[index] = data;
        index++;
    }
    else
    {
        index = 0;
        buffer[index] = data;
        index++;
    }
}

O co tu chodzi? Czemu parametry tej funkcji są non-const referencjami?

Kopiuj

    using std::swap; //enable ADL

A po co? :) Typy użyte w swapie są stałe: wskaźnik i dwa inty.

Kopiuj

template <typename T>
T const& RoundBuffer<T>::operator [](std::size_t index) const
{
    if(index > this->index)
    {
        throw std::out_of_range("Index is out of possible range");
    }
    else
    {
        return buffer[index];
    }
 
}

#include <stdexcept>. Niezbyt rozumiem, co jeśli dodasz 15 elementów do 12-elementowego bufora cyklicznego? [0] powinno zwracać pierwszy element.

Kopiuj

template <typename T>
RoundBuffer<T>& RoundBuffer<T>::operator =(RoundBuffer<T>&& copy)
{
    swap(*this, copy);
    return *this;
}
 
template <typename T>
RoundBuffer<T>::RoundBuffer(RoundBuffer<T> && rhs)
{
    swap(*this,rhs);
    rhs.buffer = nullptr;
}

Brak konsekwencji, raz zerujesz bufor-dawcę, a raz nie.

Ogółem, to wygląda jak część implementacji bufora cyklicznego, w konwencji niezgodnej z biblioteką standardową, trochę zmieszana z god objectem (wypisywanie do streamów) i zawierająca nadmiarowy kod. Ponadto jest trochę niekonsekwencji w nazewnictwie parametrów (rhs vs copy) Poza wypisanymi problemami jest jeszcze rażące przesłanianie nazw. index czy buffer są podawane jako parametry funkcji, pomimo, że są też elementami klasy. Tyle na szybko