Макрос за полуавтоматично клониране

Програмиране с Rust

Курс във Факултета по Математика и Информатика към СУ

Решение на Logging от Александър Дойков

Обратно към всички решения

Към профила на Александър Дойков

Резултати

  • 16 точки от тестове
  • 0 бонус точки
  • 16 точки общо
  • 12 успешни тест(а)
  • 3 неуспешни тест(а)

Код

use std::cell::RefCell;
use std::io;
use std::io::Write;
use std::iter::Iterator;
use std::rc::Rc;
use std::time::Instant;
pub trait Logger {
fn push(&mut self, time: Instant, text: &str);
fn log(&mut self, text: &str);
fn try_flush(&mut self) -> io::Result<()>;
fn flush(&mut self);
}
pub struct BufferedLogger<W: Write> {
buffer: Rc<RefCell<Vec<(Instant, String)>>>,
output: Rc<RefCell<W>>,
size: usize,
}
impl<W: Write> BufferedLogger<W> {
pub fn new(out: W, buffer_size: usize) -> Self {
let buffer = Vec::with_capacity(buffer_size);
BufferedLogger {
buffer: Rc::new(RefCell::new(buffer)),
output: Rc::new(RefCell::new(out)),
size: buffer_size,
}
}
pub fn buffered_entries(&self) -> Vec<String> {
let mut result: Vec<String> = Vec::with_capacity(self.size);
for record in (*self.buffer).borrow().iter() {
result.push(record.1.clone());
}
result
}
}
impl<W: Write> Clone for BufferedLogger<W> {
fn clone(&self) -> Self {
let new_buf = Rc::clone(&self.buffer);
let new_output = Rc::clone(&self.output);
BufferedLogger {
buffer: new_buf,
output: new_output,
size: self.size,
}
}
}
impl<W: Write> Logger for BufferedLogger<W> {
fn push(&mut self, time: Instant, text: &str) {
let mut alias = (*(self.buffer)).borrow_mut();
let mut index: i32 = -1;
for record in alias.iter().enumerate() {
if (record.1).0 > time {
index = record.0 as i32;
}
}

Единия от проблемите, заради които се чупят 3 теста, е тук. Логиката ти в главата си я превеждам като new_time < record_time, и ако това е вярно, това е индекса, на който ще го сложим. Но ти тогава продължаваш да циклиш и всеки следващ елемент също ще има по-голямо време. Ако итерираш наобратно с alias.iter().enumerate().rev(), кода ти минава 2 от fail-натите тестове -- "последния" елемент, чийто индекс се запази, обикаляйки в обратната посока, има правилния индекс.

Тук можеше да сложиш един .sort() и всичко щеше да е наред. Добра идея е да мислиш за performance, но кода първо трябва да е правилен, а после да е бърз -- ако беше написал няколко теста с повече от два елемента (както ние сме направили), щеше лесно да намериш проблема.

Андрей
if index > -1 {
alias.insert(index as usize, (time, text.to_string()));
} else {
alias.push((time, text.to_string()));
}

Вместо да използваш "сервизната" стойност -1, защо не None? Ако индекса ти (който можеше да се казва target_index, found_index, много по-специфични неща) беше от тип Option(usize), нямаше да има нужда да правиш никакъв casting, и щеше if-клаузата ти да е if index.is_none(). В езици като C се използва -1, понеже няма друга опция. В Rust си има удобни типове за целта.

Андрей
if alias.len() == self.size {
for record in alias.iter() {
let mut result = record.1.clone();
result.push_str("\n");
(*self.output).borrow_mut().write(result.as_bytes());
}
alias.clear()
}
}

Доста от имената и структурите могат да се подобрят. "Alias" не означава нищо само по себе си -- "alias" на какво? Можеше да го кръстиш buffer_alias, макар че можеше и просто да го кръстиш buffer. Щеше да опрости четенето на кода доста.

record също е misleading. В единия случай, това е двойка от индекс и запис, в другия е само запис. По-добре щеше да е да направиш нещо такова:

for (index, record) in buffer.iter().enumerate() {

Така record винаги щеше да значи едно и също нещо, и нямаше да имаш изрази като (record.1).0, което изглежда трудно за ментално парсене. В тоя ред на мисли, можеше да имаш тип Record { time, text } и сравнението щеше да се чете като if record.time > time {, което е доста по-ясно.

Добър rule of thumb е, "ако наричаш нещо в кода си по някакъв начин, може би има смисъл това да е официалното име на това нещо". Не винаги, но си заслужава поне да обмисли човек дали е добра идея.

Андрей
fn log(&mut self, text: &str) {
let mut alias = (*self.buffer).borrow_mut();
alias.push((Instant::now(), text.to_string()));
if alias.len() == self.size {
for record in alias.iter() {
let mut result = record.1.clone();
result.push_str("\n");
(*self.output).borrow_mut().write(result.as_bytes());
}
alias.clear()
}

В този auto-flushing код е другата ти грешка. Какво става ако self.size е нула? Никога няма да се flush-не. В случая, едно >= вместо == щеше да свърши работа. И щеше да е чудесен case за простичък тест :).

Друг проблем тук е повторението. Защо имплементацията на log не е просто push(Instant::now(), text)? Защо flush-ването в тези методи не вика просто flush()? Щеше да е доста по-малко код за писане, и щеше да е по-лесно като намериш бъг, да го промениш само на едно място (примерно, >= проверката трябва да се промени на две места, и ако забравиш едното, продължаваш да имаш бъг).

Андрей
}
fn try_flush(&mut self) -> io::Result<()> {
for record in (*self.buffer).borrow_mut().iter() {
let mut result = record.1.clone();
result.push_str("\n");
(*self.output).borrow_mut().write(result.as_bytes())?;
}
(*self.buffer).borrow_mut().clear();
Ok(())
}
fn flush(&mut self) {
for record in (*self.buffer).borrow_mut().iter() {
let mut result = record.1.clone();
result.push_str("\n");
(*self.output).borrow_mut().write(result.as_bytes());
}
(*self.buffer).borrow_mut().clear();
}
}
pub struct MultiLogger {
loggers: Vec<Box<dyn Logger>>,
}
impl MultiLogger {
pub fn new() -> Self {
MultiLogger {
loggers: Vec::new(),
}
}
pub fn log_to<L: Logger + 'static>(&mut self, logger: L) {
self.loggers.push(Box::new(logger));
}
}
impl Logger for MultiLogger {
fn push(&mut self, time: Instant, text: &str) {
for mut logger in &mut self.loggers {
logger.push(time, text);
}
}
fn log(&mut self, text: &str) {
for mut logger in &mut self.loggers {
logger.log(text);
}
}
fn try_flush(&mut self) -> io::Result<()> {
for mut logger in &mut self.loggers {
logger.try_flush()?;
}
Ok(())
}
fn flush(&mut self) {
for mut logger in &mut self.loggers {
logger.flush();
}
}
}
pub struct ScopedLogger<L: Logger> {
logger: L,
label: String,
}
impl<L: Logger> ScopedLogger<L> {
pub fn new(tag: &str, base_logger: L) -> Self {
ScopedLogger {
logger: base_logger,
label: tag.to_string(),
}
}
}
fn put_label(text: &str, mut label: String) -> String {
label.insert_str(0, "[");
label.push_str("] ");
label.push_str(text);
label
}
impl<L: Logger> Logger for ScopedLogger<L> {
fn push(&mut self, time: Instant, text: &str) {
let labled_text = put_label(text, self.label.clone());
self.logger.push(time, &labled_text);
}
fn log(&mut self, text: &str) {
let labled_text = put_label(text, self.label.clone());
self.logger.log(&labled_text);
}
fn try_flush(&mut self) -> io::Result<()> {
self.logger.try_flush()
}
fn flush(&mut self) {
self.logger.flush();
}
}

Лог от изпълнението

Compiling solution v0.1.0 (/tmp/d20190123-22631-f1osel/solution)
warning: unused `std::result::Result` that must be used
  --> src/lib.rs:73:17
   |
73 |                 (*self.output).borrow_mut().write(result.as_bytes());
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_must_use)] on by default
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
  --> src/lib.rs:86:17
   |
86 |                 (*self.output).borrow_mut().write(result.as_bytes());
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
   --> src/lib.rs:106:13
    |
106 |             (*self.output).borrow_mut().write(result.as_bytes());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
  --> src/lib.rs:73:17
   |
73 |                 (*self.output).borrow_mut().write(result.as_bytes());
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_must_use)] on by default
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
  --> src/lib.rs:86:17
   |
86 |                 (*self.output).borrow_mut().write(result.as_bytes());
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
   --> src/lib.rs:106:13
    |
106 |             (*self.output).borrow_mut().write(result.as_bytes());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this `Result` may be an `Err` variant, which should be handled

    Finished dev [unoptimized + debuginfo] target(s) in 5.10s
     Running target/debug/deps/solution-2e785d603b538f71

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/debug/deps/solution_test-29808948fb50ed3a

running 15 tests
test solution_test::test_automatic_flushing_when_buffer_limit_is_reached ... ok
test solution_test::test_automatic_flushing_when_zero_buffer_limit ... FAILED
test solution_test::test_basic_log ... ok
test solution_test::test_basic_push ... ok
test solution_test::test_cloning_a_logger_shares_a_buffer ... ok
test solution_test::test_cloning_a_logger_shares_their_io ... ok
test solution_test::test_erroring_io ... ok
test solution_test::test_flushing_the_buffer ... ok
test solution_test::test_logger_combinations ... ok
test solution_test::test_multilogger_logs_and_flushes_when_needed ... ok
test solution_test::test_multilogger_logs_to_several_ios ... ok
test solution_test::test_reordering_logs_in_buffer ... FAILED
test solution_test::test_reordering_logs_in_io ... FAILED
test solution_test::test_scoped_logger ... ok
test solution_test::test_scoped_logger_with_a_string_tag ... ok

failures:

---- solution_test::test_automatic_flushing_when_zero_buffer_limit stdout ----
thread 'solution_test::test_automatic_flushing_when_zero_buffer_limit' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`', tests/solution_test.rs:210:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- solution_test::test_reordering_logs_in_buffer stdout ----
thread 'solution_test::test_reordering_logs_in_buffer' panicked at 'assertion failed: `(left == right)`
  left: `["Second", "Third", "First", "Fourth"]`,
 right: `["First", "Second", "Third", "Fourth"]`', tests/solution_test.rs:111:5

---- solution_test::test_reordering_logs_in_io stdout ----
thread 'solution_test::test_reordering_logs_in_io' panicked at 'assertion failed: `(left == right)`
  left: `"Second\nThird\nFirst\nFourth\n"`,
 right: `"First\nSecond\nThird\nFourth\n"`', tests/solution_test.rs:134:5


failures:
    solution_test::test_automatic_flushing_when_zero_buffer_limit
    solution_test::test_reordering_logs_in_buffer
    solution_test::test_reordering_logs_in_io

test result: FAILED. 12 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test solution_test'

История (1 версия и 5 коментара)

Александър качи първо решение на 14.12.2018 22:38 (преди почти 7 години)

Решението ти минава повечето тестове, но има какво да се желае откъм четимост и способност за модификация. Можеше да си спестиш доста код (и даже време в дебъгване и донагласяне, вероятно), ако просто беше преизползвал някои методи. Можеше и да си спестиш главоболия с конвертиране между типове, ако беше използвал средствата, които Rust ти дава (Option).

Добре е да пробваш да го поизчистиш през ваканцията. Не мисля, че ще е трудно, и може сам да останеш доволен от резултата. Писането на чист код, който е лесен за работа, е умение, изгражда се с практиката.

Also, и това ще го кажа на почти всички, пиши тестове :). Предизвиквай първото си решение с edge case-ове, за да намериш проблемите му и да го обработиш, докато можеш да си уверен, че работи правилно.