Обработка исключений и др. - Как сделать этот веб-загрузчик не «бедным»?

Я не делал Java-кодирование в течение многих лет, но я думал, что дам ему шанс на собеседование. У меня есть несколько вопросов:

  1. Почему эта обработка ошибок считается бедной? Как я должен это делать?

  2. Какую ошибку следует искать в предложении finally?

  3. Как я маскирую исключения?

package jGet;

/**
 * Problem
 * 
 * Create a very simple Java class that will retrieve the resource of any URL (using the HTTP protocol)
 * and save the contents as seen by the browser, to a file.
 * 
 * Restrictions
 * 
 * You are free to use any library/technique, except for java.net.Url, java.net.URI or java.net.UrlConnection.
 * Solutions using these classes will not be accepted.
 * You are free to change the class signature for better error handling and readability.
 * 
 * Initial Class outline
 * 
 * public JGet extends Object {
 *     public JGet( String urlToPage, String saveToFilename ) {
 *     }
 *     public Object getContents() {
 *     }
 * }
 * 
 * Sample Test cases
 * 
 * The class should be able to download the following sample URL's to a file:
 *     http://www.bing.com/
 *     http://www.aw20.co.uk/images/logo.png
 * 
 * Time Allowance
 * 
 * This took me a long time to complete (longer than 30 minutes).
 * I returned the completed .java file to the person who invited me to take this, and
 * this is the feedback I got back:
 *     1) Poor exception handling
 *     2) No finally clause in case of errors
 *     3) getContents() returns a void
 *     4) You should never throw a NullPointerException
 *     5) Exception masking
 *     6) Constructor calls the function getContents() no matter what
 *     7) Poor layout
 *
 */

import java.io.FileOutputStream;
import java.io.IOException;

import org.apache.commons.io.IOUtils;
import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.DefaultHttpClient;

public class JGet {
    private final String urlToPage;
    private final String saveToFileName;

    /**
     * @param urlToPage
     * @param saveToFilename
     */
    public JGet(String urlToPage, String saveToFilename) {
        if (urlToPage == null) {
            throw new IllegalArgumentException("The URL must not be null");
        }
        if (saveToFilename == null) {
            throw new IllegalArgumentException(
                    "The name of the destination file must not be null");
        }
        if (urlToPage.length() == 0) {
            throw new IllegalArgumentException("The URL must not be blank");
        }
        if (saveToFilename.length() == 0) {
            throw new IllegalArgumentException(
                    "The name of the destination file must not be blank");
        }
        this.saveToFileName = saveToFilename;
        this.urlToPage = urlToPage;
        try {
            getContents();
        } catch (IOException e) {
            throw new IllegalArgumentException("The ability to save to the destination file must not be blocked");
        } catch (IllegalArgumentException e) {
            throw new IllegalArgumentException("The URL must not be malformed");
        }
    }

    /**
     * @throws IOException
     * @throws IllegalArgumentException
     */
    public void getContents() throws IOException, IllegalArgumentException {
        HttpClient httpClient = new DefaultHttpClient();
        HttpGet httpGet = new HttpGet(this.urlToPage);
        HttpResponse response = httpClient.execute(httpGet);
        HttpEntity entity = response.getEntity();
        IOUtils.copy(entity.getContent(), new FileOutputStream(this.saveToFileName));
    }

    /**
     * @param args
     */
    public static void main(String[] args) {
        new JGet(null, "bing.html");
        new JGet("http://www.bing.com/", null);
        new JGet("", "bing.html");
        new JGet("http://www.bing.com/", "");
        new JGet("http://www.bing.com/", "readonly.html");
        new JGet("Malformed URL", "bing.html");
        new JGet("http://www.bing.com/", "bing.html");
        new JGet("http://www.aw20.co.uk/a/img/logo.png", "logo.png");
    }

}
11 голосов | спросил Dave Babbitt 8 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowSat, 08 Sep 2012 00:01:25 +0400 2012, 00:01:25

3 ответа


11

Позвольте мне начать с того, чтобы сказать, насколько смешны ограничения:

Вы можете использовать любую библиотеку /технику, за исключением java.net.Url, java.net.URI
или java.net.UrlConnection.
Решения, использующие эти классы, не принимаются.

Дело в том, что библиотеки apache, которые вы используете, используют java.net.Url и java.net.URI.


Есть несколько вещей, которые я хотел бы указать на ваш код:

  1. Существует некоторая избыточность, которую можно избежать при проверке аргумента:

    if (urlToPage == null) {
        throw new IllegalArgumentException("The URL must not be null");
    }
    if (saveToFilename == null) {
        throw new IllegalArgumentException(
                "The name of the destination file must not be null");
    }
    if (urlToPage.length() == 0) {
        throw new IllegalArgumentException("The URL must not be blank");
    }
    if (saveToFilename.length() == 0) {
        throw new IllegalArgumentException(
                "The name of the destination file must not be blank");
    }
    

    Почему бы нам не выразить эту повторяющуюся логику в методе проверки:

    private static void validate(Object argument, Object illegal, String message){
        // reference == to avoid NullPointerException
        if (argument == illegal || argument.equals(illegal))
            throw new IllegalArgumentException(message);
    }
    

    Это значительно облегчает эту часть конструктора:

    validate(urlToPage, null, "The URL must not be null");
    validate(saveToFileName, null, "The name of the destination file must not be null");
    validate(urlToPage.length(), 0, "The URL must not be blank");
    validate(saveToFileName.length(), 0, "The destination file name must not be blank");
    
  2. Критика Constructor calls the function getContents() no matter what оправдана в том смысле, что ваш конструктор не должен выполнять какую-либо сложную работу (например, загрузку файла ) вообще. Поэтому просто установите два поля и сделайте это. Загрузка файла должна быть явно запущена после создания экземпляра.

  3. Трюк в конце вашего конструктора делает скрывает IOException как ---- +: = 7 = + ----. Избегайте когда-либо бросать IllegalArgumentException, если это не так в начале вашего метода, или вы будете путать своих абонентов. Удалите все это, так как вы все равно не должны выполнять загрузку в конструкторе.

  4. Вы правы: IllegalArgumentException не является адекватным типом возврата для Object. В этой ситуации было бы важно потребовать объяснения относительно того, какой возврат ожидается!
    Однако: если вы измените метод для возврата getContents, вам также необходимо изменить имя как void больше не является точным глаголом. Я бы просто пошел с get.

  5. Это может быть немного субъективно, но я не буду указывать Исключения в предложении download, если они не будут отмечены исключениями и, следовательно, имеют быть включенным. Поэтому я не буду оставлять throws. Критика throws IllegalArgumentException, по-видимому, имела в виду незакрытый выходной поток:

    finally

В целом, я не считаю это упражнение эффективным в показе, если кто-то хороший программист или нет. Ориентация объекта надуманна; статический класс полезности будет намного более подходящим для задачи. Кто хочет создать экземпляр нового объекта для каждой загрузки, особенно если none особенностей объектной ориентации (наследования, полиморфизма, ...) используются благотворно?

ответил Adam 8 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowSat, 08 Sep 2012 02:19:01 +0400 2012, 02:19:01
10

Есть несколько вещей, которые я вижу, что мне захочется реорганизовать, если бы я работал над этим

  1. Метод getContents() генерирует исключения, которые просто захватываются и повторно забрасываются в JGet, который модифицирует сообщение об ошибке, но не включает причину (исходное исключение). Это может затруднить поиск и устранение неисправностей кода, поскольку вы скрываете информацию о том, где возникла проблема. При вызове нового исключения вы можете передать исключение, которое вызвало его как дополнительный параметр.
  2. Возникает вопрос, действительно ли необходимо перехватывать и перебрасывать исключение (исключения) из getContents(). Может быть полезно, если измененное сообщение об ошибке поможет программисту узнать контекст, почему это незаконный аргумент, но, возможно, нет. В этом случае вы перехватываете IOException и повторно бросаете его как IllegalArugumentException, предполагая, что единственное, что может вызвать IOException в первую очередь, что целевой файл недоступен. Это то, о чем я говорю, когда говорю, что вы скрываете основную причину. Поймав IllegalArgumentException, а затем повторно перебросив его с помощью "The URL must not be malformed" также может затенять что-то и в этом случае и выглядит полностью избыточным, поскольку вы не меняете тип исключения или добавляете какую-либо полезную информацию. Просто позвольте исключению свернуть цепочку, как это было бы естественно.
  3. Первая половина (или около того) конструктора JGet - это функциональность, которая должна быть обернута в свой метод для удобства чтения (вы может позже изменить то, что определяет «незаконный» аргумент, но вы захотите сохранить это определение, содержащееся в методе, который выполняет эту работу по определению, является ли это незаконным).

Итак, я бы реорганизовал это как-то вроде:

/** Does what a JGet does (this is a joke comment)
  * 
  * @param uriToPage The URL to the page you want
  * @param saveToFilename The full or relative path to the file you want to save to
  *
  * @throws IllegalArgumentException if the either the urlToPage or saveToFilename
  *         parameters are null or blank.
  * @throws IOException if an I/O problem occurs either retrieving the results from the URL or saving the results to the file.
  */
public JGet(String urlToPage, String saveToFilename) throws IllegalAgumentException, IOException {
  this.saveToFileName = saveToFilename;
  this.urlToPage = urlToPage;

  validate();
  getContents();
}

/** Some useful documentation...
  */
private void validate() throws IllegalArgumentException {
  // your code for checking for invalid arguments
}


...the rest of it...

Я думаю, что ограничения довольно смешны, но вы можете повторно использовать инструмент ОС, предполагая, что он доступен, и вызывать curl -L [URL] или что-то в этом роде.

ответил jefflunt 8 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowSat, 08 Sep 2012 02:33:47 +0400 2012, 02:33:47
7

Вот несколько заметок, которые ранее не упоминались. Во-первых, некоторые о спецификации:

  1. Имя пакета jGet не соответствует обычным соглашениям:

  2. Здесь extends Object не требуется:

    public JGet extends Object
    

    Это значение по умолчанию.

  3. JGet не является хорошим именем класса. Из Очистить код , стр. 25:

      

    Классы и объекты должны иметь имена существительных или именных имен, например Customer, WikiPage,   Account и AddressParser. [...] Имя класса не должно быть глаголом.

Другие уже упомянули сомнительный Object тип возврата getContents. Они могут ожидать, что кандидаты начнут спорить о спецификации, хотя их ответы не предлагают этого.

Некоторые примечания о коде:

  1. Комментарии, подобные этому, не нужны:

    /**
     * @param urlToPage
     * @param saveToFilename
     */
    

    Это говорит не что иное, как код уже делает, это довольно шум. ( Чистый код Роберт С. Мартин : Глава 4: Комментарии, комментарии к шуму )

  2. Я бы использовал существующие библиотеки для проверок ввода.

    checkArgument(StringUtils.isNotBlank(urlToPage), "urlToPage cannot be blank");
    checkArgument(StringUtils.isNotBlank(saveToFilename), "saveToFilename cannot be blank");
    

    Он использует Preconditions из Guava и StringUtils из Apache Commons Lang .

    См. также: Эффективное Java, второе издание , Пункт 47: Знать и использовать библиотеки

  3. Я не согласен с тем, что вы никогда не должны бросать NullPointerException . Если конструктор получает ссылку null, это обычно ошибка в клиентском коде. NPE прекрасно подходят для этих ситуаций. (См. Также: Эффективная Java, 2-е издание , Пункт 38: Проверка параметров для достоверности и Пункт 60: Использование стандартных исключений )

  4. Заметка о значении length():

    if (urlToPage.length() == 0) { ... }
    

    Ниже показано то же самое и легче читать:

    if (urlToPage.isEmpty()) { ... }
    
  5. Вызов переопределяемых методов (например, getContents) от конструкторов обычно не является хорошей практикой. См. Что случилось с переопределяемыми вызовами метода в конструкторах ? и Эффективное Java 2nd Edition, пункт 17: дизайн и документ для наследования, а также запретить его

  6. Обратите внимание, что HttpEntity имеет writeTo . Вы можете использовать это вместо IOUtils.copy. В любом случае, если вы не используете writeTo, вы должны close InputStream, который был возвращен из entity.getContent() (как EntityUtils.toString() метод , дляпример).

    InputStream contentStream = null;
    FileOutputStream fileOutputStream = null;
    try {
        contentStream = entity.getContent();
        fileOutputStream = new FileOutputStream(saveToFileName);
        IOUtils.copy(contentStream, fileOutputStream);
    } finally {
        IOUtils.closeQuietly(contentStream);
        fileOutputStream.close(); // do NOT ignore output errors
    }
    

    Это было бы проще с помощью метода writeTo:

    final FileOutputStream fileOutputStream = new FileOutputStream(saveToFileName);
    try {
        entity.writeTo(fileOutputStream);
    } finally {
        fileOutputStream.close();
    }
    
  7. В getContents вы можете использовать urlToPage вместо this.urlToPage и saveToFileName вместо this.saveToFileName. Современные IDE используют подсветку для выделения локальных переменных из переменных экземпляра.

ответил palacsint 8 thEurope/Moscowp30Europe/Moscow09bEurope/MoscowSat, 08 Sep 2012 23:40:56 +0400 2012, 23:40:56

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132