Bufor cykliczny - review

Odpowiedz Nowy wątek
2017-07-17 15:49
0

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.

#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
edytowany 1x, ostatnio: kq, 2017-07-18 18:12

Pozostało 580 znaków

2017-07-17 16:27
kq
1
 
#include <iostream>
template <typename T>
 
class RoundBuffer

bardzo dziwne bindowanie do include'a.

    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)

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

A wersja mutable gdzie?

    void pushObject(const T& object);

pop? Front/back?

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

Dlaczego szablon po Y jest friendem dla T?

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 ^

    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

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?

    using std::swap; //enable ADL

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

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.

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


Wow:D Sporo tego ;-) Dzięki wielkie za wypunktowanie mi wszystkiego - sporo jeszcze do zrobienia/zrozumienia. - pele2525 2017-07-17 16:44
Spoko, nie jest tak źle, nawet jeśli post tak zabrzmiał ;​) - kq 2017-07-17 16:46

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0