Поругайте код (Rust Edition)

Оно собирается и работает? Я, вообще-то, склонен доверять Расту.

Даже тесты есть.

Из мелких придирок.

pub const KIND_BINARY: u32 = 1;
pub const KIND_LOCATOR: u32 = 2;
pub const KIND_TEXT: u32 = 0;

Это точно enum'ом заменить нельзя?

let mut k = try!(file.read_u8());
while k != 0 {
    item_key.push(k);
    k = try!(file.read_u8());
}

Почему.

https://github.com/Kilte/rust-ape/blob/master/src/meta.rs#L37

...

https://github.com/Kilte/rust-ape/blob/master/src/error.rs#L67

`Debug` для отладочного вывода же. Почему бы не использовать `derive`? И да, ты забыл про `#[derive(Clone, Copy, PartialEq)]`

И да, твой код излишне императивен. Используй итераторы! Их приятно читать, и компилятор всё равно превращает их в циклы.

Да, пожалуй с enum лучше будет.

Наверное, всё-таки действительно можно было лучше сделать. Вроде можно вычислить размер ключа, а потом воспользоваться методом take. Короче, надо будет подумать.

Что не так в meta?

По остальным пунктам вопросов нет. Благодарю.

if !found  {
    found = try!(probe_ape(reader, SeekFrom::Start(0)));
    if !found {
        // When located at the end of an MP3 file, an APE tag should be placed after
        // the the last frame, just before the ID3v1 tag (if any).
        if try!(probe_id3v1(reader)) {
            found = try!(probe_ape(reader, SeekFrom::End(ID3V1_OFFSET - APE_HEADER_SIZE)));
            if !found {
                // ID3v1 tag maybe preceded by Lyrics3v2: http://id3.org/Lyrics3v2
                let size = try!(probe_lyrics3v2(reader));
                if size != -1 {
                    found = try!(probe_ape(reader, SeekFrom::End(ID3V1_OFFSET - size - APE_HEADER_SIZE)));
                }
            }
        }
    }
}

Как-то слишком много вложенностей.

Да, лучше обернуть это в список функций и выполнять пока какая-нибудь не зафэйлит.

М-да, не в укор ТСу, похоже, авторы Rust таки решили использовать скрещение ML и C++ для лексики/синтаксиса. Ужаснейшая читабельность в итоге (для понимания придётся знакомиться как с ML, так и с C++). Это даже хуже, чем Scala.

Несколько замечаний, часть из них явная вкусовщина, так что с чистой совестью можешь игнорировать. Тем более, что глубоко не вникал.

• use std::result::Result as StdResult;
  

  Кажется излишним такое переименовывание, учитывая, что используется оно аж один раз - можно и полным именем воспользоваться.

• Error::Io(ref err) => StdError::description(err),
  Error::ByteOrder(ref err) => StdError::description(err),
  Error::ParseInt(ref err) => StdError::description(err),
  

  Можно вместо этого использовать:

  err.description()
  

  Заодно, опять же, можно не заморачиваться с явным указанием StdError.

• if DENIED_KEYS.iter().position(|&dk| dk == key).is_some() {
  

  Как по мне, any смотрелось бы лучше:

  if DENIED_KEYS.iter().any(|&dk| dk == key) {
  

• #![crate_name = "ape"]
  

  Просто любопытно - почему через атрибут, а не через Cargo.toml? Возможно, я не прав, но кажется более естественным, чтобы такие вещи управлялись на уровне конфига проекта, а не в сорцах задавались.
• Tag.item точно паниковать должен? Можно записать вот так:

      self.items.iter()
          .position(|item| item.key == key)
          .and_then(|idx| self.items.get(idx))
  

• pub fn vec_to_string(vec: &Vec<u8>) -> String {
      vec.iter().map(|&c| c as char).collect()
  }
  

  Есть специальная функция: from_utf8 (from_utf8_lossy).

  Ну и плюсую, что Meta.read стоит попытаться как-то покрасивее записать.

для понимания придётся знакомиться как с ML, так и с C++

Зачем? Tем более с обоими. Почему (если уж так хочется) не познакомиться именно с растом?

Код ТСа может стать намного читаемее, если его немного вычистить.

Тут, по-моему, не критиковать нужно, а радоваться, что на ржавчине пишут. Лучше, имхо, набирать команду на какой-то общий интересный проект. Например, замену сраной openssl.

На правах кулхацкинга: это можно записать как

let found =
    try!(probe_ape(reader, SeekFrom::End(-APE_HEADER_SIZE))) ||
    try!(probe_ape(reader, SeekFrom::Start(0))) ||
    try!(probe_id3v1(reader)) && {
        try!(probe_ape(reader, SeekFrom::End(ID3V1_OFFSET - APE_HEADER_SIZE))) || {
            let size = try!(probe_lyrics3v2(reader));
            size != -1 && try!(probe_ape(reader, SeekFrom::End(ID3V1_OFFSET - size - APE_HEADER_SIZE)))
        }
    };

if !found {
    return Err(Error::TagNotFound);
}

Если не критиковать, а радоваться, велик шанс получить еще один «сраный» ...ssl.

очередной ниосилятор в треде все втанк!

try!
try!
try!
try!
try!
try!

и эти люди ругают ексепшны

                }
            }
        }
    }
}

Приятно видеть, что раст не меняется

Чтобы критиковать, нужно самому хоть немного уметь. И нет, навязший в зубах «спердобейся» не катит.

Приятно видеть

...недостаток, на который указали в ревью.

Всем большое спасибо за замечания, на днях займусь как-нибудь.

Да ну, бред. Критиковать нужно всегда, иначе в итоге будут тонны говнокода, как в том же пыхе.

Вообще, я писал этот пост в предвкушении, что ты его потрешь :)

Я не собираюсь насаждать единодушие. Разве что вежливость...

Опа tailgunner в модераторах, надо почаще на лор заходить?

мля, то-то гляжу ни царя, ни анонiмуса не осталось :-/ кактеперьжитьто?

unwrap
unwrap
unwrap

https://www.rust-lang.org/faq.html#whats-the-deal-with-unwrap

https://www.rust-lang.org/faq.html#whats-the-deal-with-unwrap

Там же всего один unwrap вне тестов, зачем столько пафоса?

И правильно делают, ящитаю. Явное лучше неявного. Над синтаксическим шумом тоже работают.