feedback-to-code/mozzarella huggingface dataset:
Repository: iluwatar/java-design-patterns
Issue: Add criticism to Singleton
Labels: type: enhancement
info: good first issue

http://stackoverflow.com/questions/137975/what-is-so-bad-about-singletons


Issue comments:
https://sites.google.com/site/steveyegge2/singleton-considered-stupid

http://www.yegor256.com/2016/06/27/singletons-must-die.html

http://misko.hevery.com/2008/08/17/singletons-are-pathological-liars/

HI!

I would like to pick this one and #436 as well.

@dmitraver you're good to go!


Pull request review comments:

Patch that resolved the issue:
diff --git a/service-locator/README.md b/service-locator/README.md
index 31d82b13f8bd..479c9ed0f942 100644
--- a/service-locator/README.md
+++ b/service-locator/README.md
@@ -33,6 +33,12 @@ improves the performance of application to great extent.
 * lookups of services are done quite frequently
 * large number of services are being used
 
+## Consequences
+
+* Violates Interface Segregation Principle (ISP) by providing pattern consumers with an access 
+to a number of services that they don't potentially need.
+* Creates hidden dependencies that can break the clients at runtime.
+
 ## Credits
 
 * [J2EE Design Patterns](http://www.amazon.com/J2EE-Design-Patterns-William-Crawford/dp/0596004273/ref=sr_1_2)
diff --git a/singleton/README.md b/singleton/README.md
index e1bb42a45eab..4032ffaed03b 100644
--- a/singleton/README.md
+++ b/singleton/README.md
@@ -36,6 +36,13 @@ Use the Singleton pattern when
 * [java.lang.System#getSecurityManager()](http://docs.oracle.com/javase/8/docs/api/java/lang/System.html#getSecurityManager--)
 
 
+## Consequences
+
+* Violates Single Responsibility Principle (SRP) by controlling their own creation and lifecycle.
+* Encourages using a global shared instance which prevents an object and resources used by this object from being deallocated.     
+* Creates tightly coupled code that is difficult to test.
+* Makes it almost impossible to subclass a Singleton.
+
 ## Credits
 
 * [Design Patterns: Elements of Reusable Object-Oriented Software](http://www.amazon.com/Design-Patterns-Elements-Reusable-Object-Oriented/dp/0201633612)


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Add criticism to Service Locator
Labels: type: enhancement
info: good first issue

More information could be added to `Consequences` section of the Service Locator pattern.

http://criticalsoftwareblog.com/index.php/2015/10/25/object-composability-another-reason-why-the-service-locator-is-an-anti-pattern/

See the pattern template here: https://github.com/iluwatar/java-design-patterns/wiki/02.-Pattern-template


Issue comments:
Assigned to @dmitraver


Pull request review comments:

Patch that resolved the issue:
diff --git a/service-locator/README.md b/service-locator/README.md
index 31d82b13f8bd..479c9ed0f942 100644
--- a/service-locator/README.md
+++ b/service-locator/README.md
@@ -33,6 +33,12 @@ improves the performance of application to great extent.
 * lookups of services are done quite frequently
 * large number of services are being used
 
+## Consequences
+
+* Violates Interface Segregation Principle (ISP) by providing pattern consumers with an access 
+to a number of services that they don't potentially need.
+* Creates hidden dependencies that can break the clients at runtime.
+
 ## Credits
 
 * [J2EE Design Patterns](http://www.amazon.com/J2EE-Design-Patterns-William-Crawford/dp/0596004273/ref=sr_1_2)
diff --git a/singleton/README.md b/singleton/README.md
index e1bb42a45eab..4032ffaed03b 100644
--- a/singleton/README.md
+++ b/singleton/README.md
@@ -36,6 +36,13 @@ Use the Singleton pattern when
 * [java.lang.System#getSecurityManager()](http://docs.oracle.com/javase/8/docs/api/java/lang/System.html#getSecurityManager--)
 
 
+## Consequences
+
+* Violates Single Responsibility Principle (SRP) by controlling their own creation and lifecycle.
+* Encourages using a global shared instance which prevents an object and resources used by this object from being deallocated.     
+* Creates tightly coupled code that is difficult to test.
+* Makes it almost impossible to subclass a Singleton.
+
 ## Credits
 
 * [Design Patterns: Elements of Reusable Object-Oriented Software](http://www.amazon.com/Design-Patterns-Elements-Reusable-Object-Oriented/dp/0201633612)


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Missing synchronized keyword
Labels: type: bug
info: help wanted
info: good first issue

Hi,

https://github.com/iluwatar/java-design-patterns/blob/master/object-pool/src/main/java/com/iluwatar/object/pool/ObjectPool.java#L58 - the method should also be `synchronized`.

Best regards,
Martin

Issue comments:
I'm happy to submit a PR for this

Pull request review comments:

Patch that resolved the issue:
diff --git a/object-pool/src/main/java/com/iluwatar/object/pool/ObjectPool.java b/object-pool/src/main/java/com/iluwatar/object/pool/ObjectPool.java
index df6e887b86f1..e76d003521a5 100644
--- a/object-pool/src/main/java/com/iluwatar/object/pool/ObjectPool.java
+++ b/object-pool/src/main/java/com/iluwatar/object/pool/ObjectPool.java
@@ -55,7 +55,7 @@ public synchronized void checkIn(T instance) {
   }
 
   @Override
-  public String toString() {
+  public synchronized String toString() {
     return String.format("Pool available=%d inUse=%d", available.size(), inUse.size());
   }
 }


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Is it threadsafe ? 
Labels: type: bug
info: help wanted

Seems like lastServedId field from LoadBlanacer.class is shared among all instances.
I think this is not threadsafe and will crash when mutiple threads will try to increment it.

https://github.com/iluwatar/java-design-patterns/blob/390c33e36e82b558e8cab6028d96e49c0a20da9f/monostate/src/main/java/com/iluwatar/monostate/LoadBalancer.java#L39

Issue comments:
@Krandelbord You are correct. It is not thread safe as field is being shared without synchronization. Can you raise a PR with the fix?
Perhaps change to atomicInteger?!
If I use AtomicInteger then I have to make reading, comparing, incrementing, and setting to zero as one atomic operation.
It will end up in something like that
```
private static AtomicInteger lastServedId = new AtomicInteger();
  public void serverRequest(Request request) {
    int serverIdToUse = lastServedId.getAndUpdate((lastServedId) -> lastServedId < servers.size() - 1 ? ++lastServedId : 0);
    Server server = servers.get(serverIdToUse);
    server.serve(request);
  }
```
Doesn't look good. So I decided to make method synchronized.
Is Volatile a Solution?!
No, ```volatile``` is not suitable for cases where you want to read-update-write as an atomic operation; access to a volatile variable never has the potential to block: you're only ever doing a simple read or write ([source](https://www.javamex.com/tutorials/synchronization_volatile.shtml)).

Since we're doing changes in this class, could you also fix the misleading variable name ```serverIdToUse```: the value has nothing to do with the actual identifier of the server, it is just the index of the next server to use from ```List<Server> servers```

Pull request review comments:

Patch that resolved the issue:
diff --git a/monostate/src/main/java/com/iluwatar/monostate/LoadBalancer.java b/monostate/src/main/java/com/iluwatar/monostate/LoadBalancer.java
index e440dfba22d6..b280de76702d 100644
--- a/monostate/src/main/java/com/iluwatar/monostate/LoadBalancer.java
+++ b/monostate/src/main/java/com/iluwatar/monostate/LoadBalancer.java
@@ -35,10 +35,10 @@
 
 public class LoadBalancer {
   private static List<Server> servers = new ArrayList<>();
-  private static int id;
   private static int lastServedId;
 
   static {
+    int id = 0;
     servers.add(new Server("localhost", 8081, ++id));
     servers.add(new Server("localhost", 8080, ++id));
     servers.add(new Server("localhost", 8082, ++id));
@@ -67,14 +67,12 @@ public static int getLastServedId() {
   /**
    * Handle request
    */
-  public void serverRequest(Request request) {
+  public synchronized void serverRequest(Request request) {
     if (lastServedId >= servers.size()) {
       lastServedId = 0;
     }
     Server server = servers.get(lastServedId++);
     server.serve(request);
   }
-
-
-
+  
 }


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Consider adding another badge
Labels: info: help wanted
type: enhancement

@iluwatar as this project is open-source, I was thinking why not add https://bestpractices.coreinfrastructure.org
as a badge. It will be like cool both standardization + learning for readers.
You have to create the project there and add the badge.

Others probably also can contribute whenever you create it.
> Just thought, please ignore if not relevant

Issue comments:
I have no previous experience in the "CII Best Practices Badge Program". I see they have some [criteria](https://github.com/coreinfrastructure/best-practices-badge/blob/master/doc/criteria.md) that has to be met for a project to "self-certify". I briefly looked at it and it seems that we are already passing all their tests. So if you @zafarella or somebody else want to work on this and get us the badge I'm ok with it.

Pull request review comments:

Patch that resolved the issue:
diff --git a/README.md b/README.md
index 6eed9561b551..d7f2f154c781 100644
--- a/README.md
+++ b/README.md
@@ -8,6 +8,7 @@
 [![License MIT](https://img.shields.io/badge/license-MIT-blue.svg)](https://raw.githubusercontent.com/iluwatar/java-design-patterns/master/LICENSE.md)
 [![Join the chat at https://gitter.im/iluwatar/java-design-patterns](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/iluwatar/java-design-patterns?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)
 [![Quality Gate](https://sonarqube.com/api/badges/gate?key=com.iluwatar%3Ajava-design-patterns)](https://sonarqube.com/dashboard/index/com.iluwatar%3Ajava-design-patterns)
+[![CII Best Practices](https://bestpractices.coreinfrastructure.org/projects/1503/badge)](https://bestpractices.coreinfrastructure.org/projects/1503)
 
 # Introduction
 


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Remove redundant explicit super() constructor calls
Labels: type: enhancement
info: good first issue

The super() calls (taking no arguments) are inserted during compile time, so just make the code more verbose than required.

One example is found in the [HeroStat ](https://github.com/iluwatar/java-design-patterns/blob/master/value-object/src/main/java/com/iluwatar/value/object/HeroStat.java#L40) class.

Issue comments:
@iluwatar I'll take this if no one else has
@james-a-roland there is a pull request about this, see https://github.com/iluwatar/java-design-patterns/pull/695. But I have a feeling it does not solve all redundant calls to `super()`. Either work with @jonatasemidio by reviewing the PR or create your own PR that resolves all the cases. I will assign the issue to you.
Hi @james-a-roland and @iluwatar,
Do you know what type of unit test can I include to check this other cases?  
I would probably just `grep` through all `super` occurrences
@iluwatar - Pull request can be found here https://github.com/iluwatar/java-design-patterns/pull/697
@iluwatar, another consideration... I know that grep can find all occurrences, but we might have some samples that super() calls some father's constructors.

Hi @james-a-roland, How did you checked all super() occurrences?
I used `grep 'super()'`
Same as @olcbean - any calls to super() called from a custom constructor were deleted. Since all objects are POJO's, I think this is a safe operation.
So... In this case, we can use #697 instead of #695  
 

Pull request review comments:

Patch that resolved the issue:
diff --git a/value-object/src/main/java/com/iluwatar/value/object/HeroStat.java b/value-object/src/main/java/com/iluwatar/value/object/HeroStat.java
index ff85e0e934dc..0274bc47f394 100644
--- a/value-object/src/main/java/com/iluwatar/value/object/HeroStat.java
+++ b/value-object/src/main/java/com/iluwatar/value/object/HeroStat.java
@@ -37,7 +37,6 @@ public class HeroStat {
 
   // All constructors must be private.
   private HeroStat(int strength, int intelligence, int luck) {
-    super();
     this.strength = strength;
     this.intelligence = intelligence;
     this.luck = luck;


Test patch:
Repository: iluwatar/java-design-patterns
Issue: SonarCloud badge URL changing
Labels: info: help wanted
epic: web site
info: good first issue

Hi,

About a month ago, we deployed new built-in badges on SonarCloud: 
News: https://about.sonarcloud.io/news/2018/02/05/new-project-badges.html
Tweet: https://twitter.com/sonarcloud/status/960790516936867840
As explained in the news, the URL for those new badges is different from the old one - which we will remove on Monday April, 2nd (so in a month from now).

I'm contacting you because we noticed that some of the projects you are admin of might still be referencing the deprecated badge URL ("/api/badges/..."). If this is not the case, please ignore this email - what we see in our logs might come from forks of your repositories. However, if some of your projects do still use those deprecated URL, you might want to update them in the upcoming month to keep on enjoying SonarCloud badges on your projects. 

Hope you enjoy the service!

Best regards,
Fabrice Bellingard | SonarSource
SonarCloud Product Manager
https://sonarsource.com

Issue comments:
Hi,
    Did they provide you with the new URL pattern? It looks like that even in the SonarSource repo they are still using the old URL (search for quality gate):

     https://raw.githubusercontent.com/SonarSource/sonar-java/master/README.md
The new URL's are provided on the sonarcloud.io project page. The options

https://sonarcloud.io/api/project_badges/measure?project=com.iluwatar%3Ajava-design-patterns&metric=alert_status
https://sonarcloud.io/api/project_badges/quality_gate?project=com.iluwatar%3Ajava-design-patterns
https://sonarcloud.io/images/project_badges/sonarcloud-white.svg

Pull request review comments:

Patch that resolved the issue:
diff --git a/README.md b/README.md
index 03aea2bf64e0..4ec0f33bd350 100644
--- a/README.md
+++ b/README.md
@@ -7,7 +7,7 @@
 [![Build status](https://travis-ci.org/iluwatar/java-design-patterns.svg?branch=master)](https://travis-ci.org/iluwatar/java-design-patterns)
 [![License MIT](https://img.shields.io/badge/license-MIT-blue.svg)](https://raw.githubusercontent.com/iluwatar/java-design-patterns/master/LICENSE.md)
 [![Join the chat at https://gitter.im/iluwatar/java-design-patterns](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/iluwatar/java-design-patterns?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)
-[![Quality Gate](https://sonarcloud.io/api/badges/gate?key=com.iluwatar%3Ajava-design-patterns)](https://sonarcloud.io/dashboard/index/com.iluwatar%3Ajava-design-patterns)
+[![Quality Gate](https://sonarcloud.io/api/project_badges/measure?project=com.iluwatar%3Ajava-design-patterns&metric=alert_status)](https://sonarcloud.io/dashboard/index/com.iluwatar%3Ajava-design-patterns)
 [![CII Best Practices](https://bestpractices.coreinfrastructure.org/projects/1503/badge)](https://bestpractices.coreinfrastructure.org/projects/1503)
 
 # Introduction


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Minor Checkstyle issues

com.iluwatar.acyclicvisitor.ZoomVisitor.java and com.iluwatar.retry.Retry.java have Checkstyle warnings. 

Issue comments:
PR #787 has been merged. Closing this issue now.

Pull request review comments:

Patch that resolved the issue:
diff --git a/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/ZoomVisitor.java b/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/ZoomVisitor.java
index b4c949ed7b71..6a47867fc81b 100644
--- a/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/ZoomVisitor.java
+++ b/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/ZoomVisitor.java
@@ -25,6 +25,6 @@
 /**
  * ZoomVisitor interface
  */
-public interface ZoomVisitor extends ModemVisitor{
+public interface ZoomVisitor extends ModemVisitor {
   void visit(Zoom zoom);
 }
diff --git a/retry/src/main/java/com/iluwatar/retry/Retry.java b/retry/src/main/java/com/iluwatar/retry/Retry.java
index 5ed60f98b537..da5c76d5d3ac 100644
--- a/retry/src/main/java/com/iluwatar/retry/Retry.java
+++ b/retry/src/main/java/com/iluwatar/retry/Retry.java
@@ -105,6 +105,7 @@ public T perform() throws BusinessException {
           //ignore
         }
       }
-    } while (true);
+    }
+    while (true);
   }
 }


Test patch:
Repository: iluwatar/java-design-patterns
Issue: static enum
Labels: info: help wanted
info: good first issue

https://github.com/iluwatar/java-design-patterns/blob/55c75799831716b6248ceefa1315881e7f90806b/facade/src/main/java/com/iluwatar/facade/DwarvenMineWorker.java#L89

Enums don't need to be defined as a static. Can change it to :
`
enum Action {
`

Issue comments:
It might be best just to put a comment saying so if needed. It doesn't really matter if it's there or not. It actually might be clearer to have it.
[Java Reference on Enums](https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.9)
Well spotted @lprone. I will gladly accept a pull request that removes the unnecessary `static`.

Pull request review comments:

Patch that resolved the issue:
diff --git a/facade/src/main/java/com/iluwatar/facade/DwarvenMineWorker.java b/facade/src/main/java/com/iluwatar/facade/DwarvenMineWorker.java
index bb80ae87409d..b313dd9890d9 100644
--- a/facade/src/main/java/com/iluwatar/facade/DwarvenMineWorker.java
+++ b/facade/src/main/java/com/iluwatar/facade/DwarvenMineWorker.java
@@ -86,7 +86,7 @@ public void action(Action... actions) {
 
   public abstract String name();
 
-  static enum Action {
+  enum Action {
     GO_TO_SLEEP, WAKE_UP, GO_HOME, GO_TO_MINE, WORK
   }
 }


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Unnecesary return
Labels: info: good first issue

https://github.com/iluwatar/java-design-patterns/blob/55c75799831716b6248ceefa1315881e7f90806b/spatial-partition/src/main/java/com/iluwatar/spatialpartition/QuadTree.java#L58

you can change this block
`
    if (!this.boundary.contains(p)) {
      return;
    } else {
`

to this other most clear:
`
    if (this.boundary.contains(p)) {
`



Issue comments:
I don't really think this is an issue at all. This is a code style suggestion, and in my opinion, both are equally clear.
Yes, just create a pull request and we can improve the style.
Hi @iluwatar  
I can take this up
Ok @kanwarpreet25 

Pull request review comments:

Patch that resolved the issue:
diff --git a/spatial-partition/src/main/java/com/iluwatar/spatialpartition/QuadTree.java b/spatial-partition/src/main/java/com/iluwatar/spatialpartition/QuadTree.java
index 072468ce80d4..815bf8acc74c 100644
--- a/spatial-partition/src/main/java/com/iluwatar/spatialpartition/QuadTree.java
+++ b/spatial-partition/src/main/java/com/iluwatar/spatialpartition/QuadTree.java
@@ -55,16 +55,13 @@ public class QuadTree {
   }
 
   void insert(Point p) {
-    if (!this.boundary.contains(p)) {
-      return;
-    } else {
+    if (this.boundary.contains(p)) {
       if (this.points.size() < this.capacity) {
         points.put(p.id, p);
       } else {
         if (!this.divided) {
           this.divide();
         }
-
         if (this.northwest.boundary.contains(p)) {
           this.northwest.insert(p);
         } else if (this.northeast.boundary.contains(p)) {


Test patch:
Repository: iluwatar/java-design-patterns
Issue: Dirty Flag pattern Issue
Labels: info: help wanted
type: enhancement
info: good first issue

https://github.com/iluwatar/java-design-patterns/blob/fedc2d9e47cb2b44aa4ea23aed9d117bbd4ed069/dirty-flag/src/main/java/com/iluwatar/dirtyflag/App.java#L60

Creating object of World class inside Thread's Run method doesn't make sense, as every time it is creating new object and reading the file. It should be outside this thread's run method.

for now it is like this
    executorService.scheduleAtFixedRate(new Runnable() {
      @Override
      public void run() {
      World world = new World();
        List<String> countries = world.fetch();

but it should be like this
    executorService.scheduleAtFixedRate(new Runnable() {
      World world = new World();
      @Override
      public void run() {
        List<String> countries = world.fetch();

Issue comments:
@iluwatar  Can i take this.  Thanks
Ok @ranjeet-floyd 
Is this fixed already?
Is this issue already resloved?

Pull request review comments:

Patch that resolved the issue:
diff --git a/dirty-flag/src/main/java/com/iluwatar/dirtyflag/App.java b/dirty-flag/src/main/java/com/iluwatar/dirtyflag/App.java
index 4d72b34da249..827028d09a06 100644
--- a/dirty-flag/src/main/java/com/iluwatar/dirtyflag/App.java
+++ b/dirty-flag/src/main/java/com/iluwatar/dirtyflag/App.java
@@ -55,9 +55,9 @@ public void run() {
 
     final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
     executorService.scheduleAtFixedRate(new Runnable() {
+      final World world = new World();
       @Override
       public void run() {
-        World world = new World();
         List<String> countries = world.fetch();
         System.out.println("Our world currently has the following countries:-");
         for (String country : countries) {


Test patch:
Repository: iluwatar/java-design-patterns
Issue: typo in FYI comment at "java-design-pattern/acyclic-visitor"
Labels: type: bug
info: good first issue

From here:
https://github.com/iluwatar/java-design-patterns/blob/fedc2d9e47cb2b44aa4ea23aed9d117bbd4ed069/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/App.java#L50

The comment should be:
```text
// Hayes modem with Dos configurator 
```

Issue comments:
I am working on this issue.
Ok @saksham93 
Pull Request raised

Pull request review comments:

Patch that resolved the issue:
diff --git a/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/App.java b/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/App.java
index 06e4bd2f255d..2a2f64591b6c 100644
--- a/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/App.java
+++ b/acyclic-visitor/src/main/java/com/iluwatar/acyclicvisitor/App.java
@@ -47,7 +47,7 @@ public static void main(String[] args) {
     Zoom zoom = new Zoom();
     Hayes hayes = new Hayes();
     
-    hayes.accept(conDos); // Hayes modem with Unix configurator
+    hayes.accept(conDos); // Hayes modem with Dos configurator
     zoom.accept(conDos); // Zoom modem with Dos configurator
     hayes.accept(conUnix); // Hayes modem with Unix configurator
     zoom.accept(conUnix); // Zoom modem with Unix configurator   


Test patch: