ronantakizawa/codereview-bench huggingface dataset:
Repository: coqui-ai/TTS
File: TTS/api.py (Python)

Code before:
from pathlib import Path
from TTS.utils.synthesizer import Synthesizer
from TTS.utils.manage import ModelManager


class TTS():
    """TODO: Add voice conversion and Capacitron support.
    """
    def __init__(self, model_name:str=None, progress_bar:bool=True, gpu=False):
        """🐸TTS python interface that allows to load and use the released models.

        Args:
            model_name (str, optional): Model name to load. You can list models by ```tts.models```. Defaults to None.
            progress_bar (bool, optional): Whether to pring a progress bar while downloading a model. Defaults to True.
            gpu (bool, optional): Enable/disable GPU. Some models might be too slow on CPU. Defaults to False.
        """
        self.manager = ModelManager(models_file=self.get_models_file_path(), progress_bar=progress_bar, verbose=False)
        self.synthesizer = None
        if model_name:
            self.load_model_by_name(model_name, gpu)

    @property
    def models(self):
        return self.manager.list_tts_models()

    @property
    def is_multi_speaker(self):
        if hasattr(self.synthesizer.tts_model, "speaker_manager") and self.synthesizer.tts_model.speaker_manager:
            return self.synthesizer.tts_model.speaker_manager.num_speakers > 1
        return False


Review comment (suggestion):
I don't know If calling it TTS is the best thing because it conflicts with the name of the package so it might prevent this kind of import: `from TTS.api import TTS`

Code after:
from pathlib import Path

from TTS.utils.manage import ModelManager
from TTS.utils.synthesizer import Synthesizer


class TTS:
    """TODO: Add voice conversion and Capacitron support."""

    def __init__(self, model_name: str = None, progress_bar: bool = True, gpu=False):
        """🐸TTS python interface that allows to load and use the released models.

        Example with a multi-speaker model:
            >>> from TTS.api import TTS
            >>> tts = TTS(TTS.list_models()[0])
            >>> wav = tts.tts("This is a test! This is also a test!!", speaker=tts.speakers[0], language=tts.languages[0])
            >>> tts.tts_to_file(text="Hello world!", speaker=tts.speakers[0], language=tts.languages[0], file_path="output.wav")

        Example with a single-speaker model:
            >>> tts = TTS(model_name="tts_models/de/thorsten/tacotron2-DDC", progress_bar=False, gpu=False)
            >>> tts.tts_to_file(text="Ich bin eine Testnachricht.", file_path="output.wav")

        Args:
            model_name (str, optional): Model name to load. You can list models by ```tts.models```. Defaults to None.
            progress_bar (bool, optional): Whether to pring a progress bar while downloading a model. Defaults to True.
            gpu (bool, optional): Enable/disable GPU. Some models might be too slow on CPU. Defaults to False.
        """
        self.manager = ModelManager(models_file=self.get_models_file_path(), progress_bar=progress_bar, verbose=False)
        self.synthesizer = None
        if model_name:
            self.load_model_by_name(model_name, gpu)
Repository: corona-warn-app/cwa-server
File: services/download/src/main/java/app/coronawarn/server/services/download/BatchDownloadException.java (Java)

Code before:
package app.coronawarn.server.services.download;

import static java.time.format.DateTimeFormatter.ISO_LOCAL_DATE;

import java.time.LocalDate;

public class BatchDownloadException extends RuntimeException {

  private static final long serialVersionUID = 1L;

  public BatchDownloadException(LocalDate date, Throwable cause) {
    this(null, date, cause);
  }

  public BatchDownloadException(String batchTag, LocalDate date, Throwable cause) {
    super("Downloading batch " + (batchTag != null ? batchTag : "") + " for date " + date.format(ISO_LOCAL_DATE)
        + " failed. Reason: " + (cause != null ? cause.getMessage() : "none given"), cause);
  }
}


Review comment (suggestion):
Now seeing this: Maybe we should change the type to a checked exception? Because we want to make sure that we catch every instance.

Code after:
package app.coronawarn.server.services.download;

import static java.time.format.DateTimeFormatter.ISO_LOCAL_DATE;

import java.time.LocalDate;

public class BatchDownloadException extends Exception {

  private static final long serialVersionUID = 1L;

  public BatchDownloadException(LocalDate date, Throwable cause) {
    this(null, date, cause);
  }

  public BatchDownloadException(String batchTag, LocalDate date, Throwable cause) {
    super("Downloading batch " + (batchTag != null ? batchTag : "") + " for date " + date.format(ISO_LOCAL_DATE)
        + " failed. Reason: " + (cause != null ? cause.getMessage() : "none given"), cause);
  }
}
Repository: corona-warn-app/cwa-server
File: services/callback/src/main/java/app/coronawarn/server/services/callback/HashingUtils.java (Java)

Code before:

import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import org.bouncycastle.util.encoders.Hex;

public class HashingUtils {

  public static final String HASHING_ALGORITHM = "SHA-256";

  private HashingUtils() {
    throw new IllegalStateException("Utility class");
  }

  /**
   * Computes the SHA-256 hash of the provided string.
   *
   * @param subject the string to compute the hash
   * @return the SHA-256 hash of the string.
   */
  public static String computeSha256Hash(String subject) {
    MessageDigest digest = null;
    try {
      digest = MessageDigest.getInstance(HASHING_ALGORITHM);
    } catch (NoSuchAlgorithmException e) {
      throw new HashAlgorithmNotFoundException("Hashing algorithm '" + HASHING_ALGORITHM + "' not found.");
    }
    byte[] hash = digest.digest(
        subject.getBytes(StandardCharsets.UTF_8));
    return new String(Hex.encode(hash));
  }

}


Review comment (suggestion):
is there a reason, why we silently drop the complete stacktrace and ignore the root-cause?

```suggestion
      throw new HashAlgorithmNotFoundException("Hashing algorithm '" + HASHING_ALGORITHM + "' not found.", e);
```

Code after:

import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import org.bouncycastle.util.encoders.Hex;

public class HashingUtils {

  public static final String HASHING_ALGORITHM = "SHA-256";

  private HashingUtils() {
    throw new IllegalStateException("Utility class");
  }

  /**
   * Computes the SHA-256 hash of the provided string.
   *
   * @param subject the string to compute the hash
   * @return the SHA-256 hash of the string.
   */
  public static String computeSha256Hash(String subject) {
    MessageDigest digest = null;
    try {
      digest = MessageDigest.getInstance(HASHING_ALGORITHM);
    } catch (NoSuchAlgorithmException e) {
      throw new HashAlgorithmNotFoundException("Hashing algorithm '" + HASHING_ALGORITHM + "' not found.", e);
    }
    byte[] hash = digest.digest(
        subject.getBytes(StandardCharsets.UTF_8));
    return new String(Hex.encode(hash));
  }

}
Repository: corona-warn-app/cwa-server
File: services/callback/src/test/java/app/coronawarn/server/services/callback/controller/ApiExceptionHandlerTest.java (Java)

Code before:
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.test.web.servlet.MockMvc;

@ExtendWith({MockitoExtension.class})
class ApiExceptionHandlerTest {

  private static final String ENDPOINT = "/version/v1/callback";

  private MockMvc mockMvc;

  @Mock
  private FederationBatchInfoService serviceMock;
  @InjectMocks
  private CallbackController callbackController;

  @BeforeEach
  public void setup() {
    this.mockMvc = standaloneSetup(callbackController).setControllerAdvice(new ApiExceptionHandler()).build();
  }

  @Test
  void testUnexpectedExceptionsTriggersStatusCode500() throws Exception {
    mockMvc.perform(get(ENDPOINT).param("batchTag", "batchTag").param("date", "2020-05-05"))
        .andExpect(status().isInternalServerError());
  }

  @Test
  void testMissingBatchTagTriggersStatusCode400() throws Exception {
    mockMvc.perform(get(ENDPOINT)).andExpect(status().isBadRequest());
    mockMvc.perform(get(ENDPOINT).param("date", "2020-11-31")).andExpect(status().isBadRequest());
  }
}


Review comment (suggestion):
I think we should keep this logic. Right now the InternalServerError is thrown because config is null not because the mock throws an error

Code after:
import app.coronawarn.server.services.callback.config.CallbackServiceConfig;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.test.web.servlet.MockMvc;

@ExtendWith({MockitoExtension.class})
class ApiExceptionHandlerTest {

  private static final String ENDPOINT = "/version/v1/callback";

  private MockMvc mockMvc;

  @Mock
  private FederationBatchInfoService serviceMock;
  @InjectMocks
  private CallbackController callbackController;
  @Mock
  private CallbackServiceConfig callbackServiceConfig;

  @BeforeEach
  public void setup() {
    this.mockMvc = standaloneSetup(callbackController).setControllerAdvice(new ApiExceptionHandler()).build();
  }

  @Test
  void testUnexpectedExceptionsTriggersStatusCode500() throws Exception {
    when(callbackServiceConfig.getSourceSystem()).thenReturn(FederationBatchSourceSystem.EFGS);
    doThrow(RuntimeException.class).when(serviceMock).save(any());
    mockMvc.perform(get(ENDPOINT).param("batchTag", "batchTag").param("date", "2020-05-05"))
        .andExpect(status().isInternalServerError());
  }

  @Test
  void testMissingBatchTagTriggersStatusCode400() throws Exception {
    mockMvc.perform(get(ENDPOINT)).andExpect(status().isBadRequest());
    mockMvc.perform(get(ENDPOINT).param("date", "2020-11-31")).andExpect(status().isBadRequest());
  }
}
Repository: corona-warn-app/cwa-server
File: common/shared/src/test/java/app/coronawarn/server/common/shared/util/CwaStringTest.java (Java)

Code before:


package app.coronawarn.server.common.shared.util;

import static app.coronawarn.server.common.shared.util.CwaStringUtils.emptyCharrArrayIfNull;
import static java.time.ZoneOffset.UTC;
import static java.time.temporal.ChronoUnit.HOURS;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.time.Instant;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.util.Arrays;
import org.junit.jupiter.api.Test;

class CwaStringTest {
  private String STRING_TEST = "test me";
  private String EMPTY_STRING = "";

  @Test
  void shouldConvertStringToCharArray() {
    assertThat(String.valueOf(emptyCharrArrayIfNull(STRING_TEST))).isEqualTo(STRING_TEST);
    assertThat(emptyCharrArrayIfNull(EMPTY_STRING)).isEmpty();
  }

}


Review comment (suggestion):
```suggestion
    assertThat(emptyCharrArrayIfNull(EMPTY_STRING)).isEmpty();
    assertThat(emptyCharrArrayIfNull(null)).isEmpty();
```

Code after:
package app.coronawarn.server.common.shared.util;

import static app.coronawarn.server.common.shared.util.CwaStringUtils.emptyCharrArrayIfNull;
import static org.assertj.core.api.Assertions.assertThat;

import org.junit.jupiter.api.Test;

class CwaStringTest {
  private String STRING_TEST = "test me";
  private String EMPTY_STRING = "";

  @Test
  void shouldConvertStringToCharArray() {
    assertThat(String.valueOf(emptyCharrArrayIfNull(STRING_TEST))).isEqualTo(STRING_TEST);
    assertThat(emptyCharrArrayIfNull(EMPTY_STRING)).isEmpty();
    assertThat(emptyCharrArrayIfNull(null)).isEmpty();
  }

}
Repository: corona-warn-app/cwa-server
File: services/distribution/src/main/java/app/coronawarn/server/services/distribution/dgc/client/DigitalCovidCertificateFeignClient.java (Java)

Code before:
  ResponseEntity<List<BusinessRuleItem>> getRules();

  /**
   * HTTP GET to return all business rules.
   */
  @Timed
  @GetMapping(value = "${services.distribution.digital-green-certificate.client.bn-rules-path}")
  ResponseEntity<List<BusinessRuleItem>> getBoosterNotificationRules();

  /**
   * HTTP GET to return a specific business rule based on its country and hash.
   *
   * @param country - business rule country code.
   * @param hash - business rule hash
   */
  @Timed
  @GetMapping(value = "${services.distribution.digital-green-certificate.client.rules-path}/{country}/{hash}")
  ResponseEntity<BusinessRule> getCountryRule(@PathVariable String country, @PathVariable String hash);

  /**
   * HTTP GET to return a specific business rule based on its country and hash.
   * @param hash - business rule hash
   */
  @Timed
  @GetMapping(value = "${services.distribution.digital-green-certificate.client.bn-rules-path}/{hash}")
  ResponseEntity<BusinessRule> getBnRule(@PathVariable String hash);
}


Review comment (suggestion):
I would recommend not to abreviate this method names for consistency. No other method is abreviated in the Feign client. Also would rename all the other methods further down in the code which uses "Bn". It is just not clear enough.

Code after:
  ResponseEntity<List<BusinessRuleItem>> getRules();

  /**
   * HTTP GET to return all business rules.
   */
  @Timed
  @GetMapping(value = "${services.distribution.digital-green-certificate.client.bn-rules-path}")
  ResponseEntity<List<BusinessRuleItem>> getBoosterNotificationRules();

  /**
   * HTTP GET to return a specific business rule based on its country and hash.
   *
   * @param country - business rule country code.
   * @param hash - business rule hash
   */
  @Timed
  @GetMapping(value = "${services.distribution.digital-green-certificate.client.rules-path}/{country}/{hash}")
  ResponseEntity<BusinessRule> getCountryRule(@PathVariable String country, @PathVariable String hash);

  /**
   * HTTP GET to return a specific business rule based on its hash.
   * @param hash - business rule hash
   */
  @Timed
  @GetMapping(value = "${services.distribution.digital-green-certificate.client.bn-rules-path}/{hash}")
  ResponseEntity<BusinessRule> getBoosterNotificationRule(@PathVariable String hash);
}
Repository: corona-warn-app/cwa-server
File: services/distribution/src/main/java/app/coronawarn/server/services/distribution/statistics/file/MockStatisticJsonFileLoader.java (Java)

Code before:
  @Autowired
  DistributionServiceConfig serviceConfig;

  /**
   * Test method to get content from test file.
   *
   * @return String content of file
   */
  public JsonFile getFile(StatisticType statisticType) {
    String resourcePath;

    switch (statisticType) {
      case LOCAL:
        resourcePath = serviceConfig.getStatistics().getLocalStatisticPath();
        break;
      default:
        resourcePath = serviceConfig.getStatistics().getStatisticPath();
    }

    var resource = resourceLoader.getResource(String.format("classpath:%s", resourcePath));

    try {
      return new JsonFile(resource.getInputStream(), "local");
    } catch (IOException e) {
      throw new JsonFileLoaderException(String.format("Failed to load Local JSON from path %s",
          serviceConfig.getStatistics().getStatisticPath()), e);
    }
  }

  @Override
  public Optional<JsonFile> getFileIfUpdated(StatisticType statisticType, String etag) {
    return Optional.of(this.getFile(statisticType));
  }

}


Review comment (suggestion):
move the message into the custom exception
```suggestion
      throw new JsonFileLoaderException(serviceConfig.getStatistics().getStatisticPath(), e);
```

Code after:
  @Autowired
  DistributionServiceConfig serviceConfig;

  /**
   * Test method to get content from test file.
   *
   * @return String content of file
   */
  public JsonFile getFile(StatisticType statisticType) {
    String resourcePath;

    switch (statisticType) {
      case LOCAL:
        resourcePath = serviceConfig.getStatistics().getLocalStatisticPath();
        break;
      default:
        resourcePath = serviceConfig.getStatistics().getStatisticPath();
    }

    var resource = resourceLoader.getResource(String.format("classpath:%s", resourcePath));

    try {
      return new JsonFile(resource.getInputStream(), "local");
    } catch (IOException e) {
      throw new JsonFileLoaderException(serviceConfig.getStatistics().getStatisticPath(), e);
    }
  }

  @Override
  public Optional<JsonFile> getFileIfUpdated(StatisticType statisticType, String etag) {
    return Optional.of(this.getFile(statisticType));
  }

}
Repository: corona-warn-app/cwa-server
File: services/distribution/src/test/java/app/coronawarn/server/services/distribution/dgc/dsc/CloudDscFeignHttpClientProviderSmokeTest.java (Java)

Code before:
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.boot.test.context.ConfigDataApplicationContextInitializer;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;

@EnableConfigurationProperties(value = DistributionServiceConfig.class)
@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes = {CloudDscFeignHttpClientProvider.class},
    initializers = ConfigDataApplicationContextInitializer.class)
class CloudDscFeignHttpClientProviderSmokeTest {

  @Autowired
  DistributionServiceConfig distributionServiceConfig;

  @Test
  void testThrowException() {
    Ssl ssl = new Ssl();
    ssl.setTrustStore(new File("Incorrect"));
    ssl.setTrustStorePassword("Incorrect");
    distributionServiceConfig.setConnectionPoolSize(1);
    distributionServiceConfig.getDigitalGreenCertificate().getDscClient().setSsl(ssl);

    CloudDscFeignHttpClientProvider provider = new CloudDscFeignHttpClientProvider(distributionServiceConfig);
    assertThatExceptionOfType(CloudDscFeignHttpClientProviderException.class)
        .isThrownBy(() -> provider.createDscFeignClient());
  }

}



Review comment (suggestion):
```suggestion
    assertThatExceptionOfType(CloudFeignHttpClientProviderException .class)
```

Code after:
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.boot.test.context.ConfigDataApplicationContextInitializer;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;

@EnableConfigurationProperties(value = DistributionServiceConfig.class)
@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes = {CloudDscFeignHttpClientProvider.class},
    initializers = ConfigDataApplicationContextInitializer.class)
class CloudDscFeignHttpClientProviderSmokeTest {

  @Autowired
  DistributionServiceConfig distributionServiceConfig;

  @Test
  void testThrowException() {
    Ssl ssl = new Ssl();
    ssl.setTrustStore(new File("Incorrect"));
    ssl.setTrustStorePassword("Incorrect");
    distributionServiceConfig.setConnectionPoolSize(1);
    distributionServiceConfig.getDigitalGreenCertificate().getDscClient().setSsl(ssl);

    CloudDscFeignHttpClientProvider provider = new CloudDscFeignHttpClientProvider(distributionServiceConfig);
    assertThatExceptionOfType(CloudFeignHttpClientProviderException.class)
        .isThrownBy(() -> provider.createDscFeignClient());
  }

}
Repository: corona-warn-app/cwa-server
File: services/distribution/src/main/java/app/coronawarn/server/services/distribution/statistics/keyfigurecard/factory/InfectionsCardFactory.java (Java)

Code before:
    return keyFigureBuilder.addAllKeyFigures(List.of(
        getInfectionsReported(stats),
        getInfectionsAverage(stats),
        getReportsCumulated(stats))).build();
  }

  @Override
  protected List<Optional<Object>> getRequiredFieldValues(StatisticsJsonStringObject stats) {

    List<Optional<Object>> requiredFields = List.of(
        Optional.ofNullable(stats.getInfectionsReportedCumulated()),
        Optional.ofNullable(stats.getInfectionsReported7daysTrend5percent()),
        Optional.ofNullable(stats.getInfectionsReported7daysAvg()),
        Optional.ofNullable(stats.getInfectionsReportedDaily())
    );

    if (requiredFields.contains(Optional.empty())
        || stats.getInfectionsReportedCumulated() <= 0
        || stats.getInfectionsReported7daysAvg() <= 0
        || stats.getInfectionsReportedDaily() <= 0) {
      return List.of(Optional.empty());
    }

    if (stats.getInfectionsReportedDaily() < stats.getInfectionsReported7daysAvg() * 0.01) {
      logger.warn("skipping '{}' for '{}', because the reported infections is less than {}% of the last 7 days average", stats.getEffectiveDate(),
          INFECTIONS_CARD, 0.01);
      return List.of(Optional.empty());
    }
    return requiredFields;
  }
}


Review comment (suggestion):
```suggestion
      logger.warn("skipping '{}' for '{}', because the reported infections are less than {}% of the last 7 days average", stats.getEffectiveDate(),
          INFECTIONS_CARD, 0.01 * 100);
```

Code after:
        getInfectionsReported(stats),
        getInfectionsAverage(stats),
        getReportsCumulated(stats))).build();
  }

  @Override
  protected List<Optional<Object>> getRequiredFieldValues(StatisticsJsonStringObject stats) {

    List<Optional<Object>> requiredFields = List.of(
        Optional.ofNullable(stats.getInfectionsReportedCumulated()),
        Optional.ofNullable(stats.getInfectionsReported7daysTrend5percent()),
        Optional.ofNullable(stats.getInfectionsReported7daysAvg()),
        Optional.ofNullable(stats.getInfectionsReportedDaily())
    );

    if (requiredFields.contains(Optional.empty())
        || stats.getInfectionsReportedCumulated() <= 0
        || stats.getInfectionsReported7daysAvg() <= 0
        || stats.getInfectionsReportedDaily() <= 0) {
      return List.of(Optional.empty());
    }

    if (stats.getInfectionsReportedDaily() < stats.getInfectionsReported7daysAvg() * 0.01) {
      logger.warn(
          "skipping '{}' for '{}', because the reported infections are less than {}% of the last 7 days average",
          stats.getEffectiveDate(), INFECTIONS_CARD, 0.01 * 100);
      return List.of(Optional.empty());
    }

    return requiredFields;
  }
}