Simple Imgur API

Question

I slowly made an Imgur client and everything works but I have few questions about how to make classes more OOP and cleaner.

1. Is that correct to make generateToken static when it is private?

2. Is calling the method from the constructor an anti-pattern?

   public class ImgurClient extends Imgur {
   
       public ImgurClient(String client_id, String client_secret) {
           super(client_id, client_secret);
           generateToken();
       }
   
        private static void generateToken(){
            System.out.println("Enter into link, accept application and paste here full redirect url to get credentials");
            System.out.println(ImgurURLs.getAuthorizeUrlWithClientID());
            ImgurAuthentication imgurAuthentication = new ImgurAuthentication(); //todo metody statyczne? moze na gore?
            imgurAuthentication.getURLFromUserAndGenerateTokenFromImgurOauth();
        }
   
        @Override
        public void getAccountBase() {
   
        }
   
        @Override
        void getAccountImages() {
   
        }
   }
   

This is the class where I parse redirect URL with tokens:

3. Should every methodbe static here?
4. I know calling two methods from one method is incorrect, but how should I make it better?

5. Extract methods are pretty similar. How should I make it better?

   public class ImgurAuthentication {
   
       private final Pattern accessTokenPattern = Pattern.compile("access_token=([^&]*)");
       private final Pattern refreshTokenPattern = Pattern.compile("refresh_token=([^&]*)");
       private final Pattern expiresInPattern = Pattern.compile("expires_in=(\\d+)");
       private final Pattern accountIdPattern = Pattern.compile("account_id=(\\d+)");
   
       private Matcher matcher;
       private String url = getUrlFromUser();
   
   
       public void getURLFromUserAndGenerateTokenFromImgurOauth(){
           getUrlFromUser();
           extractTokensValueFromURL();
       }
   
       private String getUrlFromUser(){
           Scanner scanner = new Scanner(System.in);
           return scanner.nextLine();
       }
   
       private void extractTokensValueFromURL(){
           try{
               extractAccessToken();
               extractRefreshToken();
               extractExpiresIn();
               extractAccountId();
           }catch (IllegalStateException ex){
               throw new IllegalArgumentException("Incorrect authorization url", ex);
           }
       }
   
       private void extractAccessToken(){
           matcher = accessTokenPattern.matcher(url);
           matcher.find();
           AccountCredentials.TOKEN=matcher.group(1);
       }
   
       private void extractRefreshToken(){
           matcher = refreshTokenPattern.matcher(url);
           matcher.find();
           AccountCredentials.REFRESH_TOKEN=matcher.group(1);
       }
   
       private void extractExpiresIn(){
           matcher = expiresInPattern.matcher(url);
       matcher.find();
           AccountCredentials.EXPIRES_IN = Long.parseLong(matcher.group(1));
       }
   
       private void extractAccountId(){
           matcher = accountIdPattern.matcher(url);
           matcher.find();
           AccountCredentials.ACCOUNT_ID = Long.parseLong(matcher.group(1));
       }
   }
   

This is my const class where I keep everything about the user:

6. I know keeping const in an interface is an anti-pattern. How about that way?

   public final class AccountCredentials {
   
       private AccountCredentials(){}
   
       public static String CLIENT_ID = "";
       public static String CLIENT_SECRET = "";
       public static String TOKEN = "";
       public static String REFRESH_TOKEN = "";
       public static long EXPIRES_IN = -1;
       public static long ACCOUNT_ID=-1;
   }
   

Answer 1

The AccountCredentials class should be treated as an object and not a set of constants:

public final class AccountCredentials {
    private final String id;
    private final String secret;
    private final String token;
    private final String refreshToken;
    private final long expiry;
    private final long accountID;

    public AccountCredentials(String id, String secret, String token, String refreshToken,
            long expiry, long accountID){
        this.id = id;
        this.secret = secret;
        this.token = token;
        this.refreshToken = refreshToken;
        this.expiry = expiry;
        this.accountID = accountID;
    }

    /* getters omitted for brevity */
}

This trickles down to the ImgurAuthentication class, which now has the "goal" of returning an AccountCredentials object. With a little refactoring:

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class ImgurAuthentication {

    private final Pattern accessTokenPattern = Pattern.compile("access_token=([^&]*)");
    private final Pattern refreshTokenPattern = Pattern.compile("refresh_token=([^&]*)");
    private final Pattern expiresInPattern = Pattern.compile("expires_in=(\\d+)");
    private final Pattern accountIdPattern = Pattern.compile("account_id=(\\d+)");

    private final String url;

    public ImgurAuthentication(String url){
        this.url = url;
    }

    public AccountCredentials generateCredentials(String id, String secret){
        try{
            return new AccountCredentials(id, secret, getAccessToken(),
                    getRefreshToken(), getExpiry(), getAccountID());
        }catch (IllegalStateException ex){
            throw new IllegalArgumentException("Incorrect authorization url", ex);
        }
    }

    private String getAccessToken(){
        Matcher matcher = accessTokenPattern.matcher(url);
        matcher.find();
        return matcher.group(1);
    }

    private String getRefreshToken(){
        Matcher matcher = refreshTokenPattern.matcher(url);
        matcher.find();
        return matcher.group(1);
    }

    private long getExpiry(){
        Matcher matcher = expiresInPattern.matcher(url);
        matcher.find();
        return Long.parseLong(matcher.group(1));
    }

    private long getAccountID(){
        Matcher matcher = accountIdPattern.matcher(url);
        matcher.find();
        return Long.parseLong(matcher.group(1));
    }
}

I would recommend not catching IllegalStateException, since that can be indicative of bigger things and should be passed up the chain. Instead, maybe check the result of matcher.find() and return null or throw new IllegalArgumentException(); if false?

That just leaves the ImgurClient class, which keeps an AccountCredentials field for itself:

import java.util.Scanner;

public class ImgurClient extends Imgur {

    private final AccountCredentials credentials;

    public ImgurClient(String client_id, String client_secret) {
        super(client_id, client_secret);
        credentials = generateToken();
    }

    private AccountCredentials generateToken(){
        System.out.println("Enter into link, accept application and paste here full redirect url to get credentials");
        System.out.println(ImgurURLs.getAuthorizeUrlWithClientID(this.getClientID()));
        ImgurAuthentication imgurAuthentication = new ImgurAuthentication(readURL());
        return imgurAuthentication.generateCredentials(this.getClientID(), this.getClientSecret());
    }

    @Override
    public void getAccountBase() {

    }

    @Override
    void getAccountImages() {

    }

    private static String readURL(){
        Scanner scanner = new Scanner(System.in);
        return scanner.nextLine();
    }
}

A quick note for future reference: favor shorter method names. getURLFromUserAndGenerateTokenFromImgurOauth() is a mouthful!