6
\$\begingroup\$

I am building a login using an API call to a URL that returns JSON data. The login works and is functional but in terms of "correctness" and "professionalism", I am looking to get input from some of the devs here. Please give me an tips that could possibly aide in making the most functional program here possible. (Please note I am using MATISSE to create the GUI)

This section is the Login GUI. Left out Matisse generate text:

public class LogIn extends javax.swing.JFrame {
   ImageIcon imageIcon;
   /**
   * Creates new form LogIn
   */
   public LogIn() 
   {
      imageIcon = new ImageIcon(LogIn.this.getClass().getResource("/Res/loading.gif"));
      initComponents();
   }

   /*
   This actually completes the login and displays the LOADING dialog box. 
   */
    class loginAction extends AbstractAction {
    LoginHandle lh = new LoginHandle();
    String devid = "&t2mdeveloperid=#####-####-####-####-#######";
    String account, username, password;
    String[] loginDataArr;

   @Override
   public void actionPerformed(ActionEvent evt) 
   {
   SwingWorker<Void, Void> mySwingWorker = new SwingWorker<Void, Void>()
   {
      @Override
      protected Void doInBackground() throws Exception 
      {
         account = "&t2maccount=" + txtAccount.getText();
         username = "&t2musername=" + txtUsername.getText();
         password = "&t2mpassword=" + txtPassword.getText();
         loginDataArr = lh.loginData(account, username, password, devid);
         return null;
      }
    };

    Window win = SwingUtilities.getWindowAncestor((AbstractButton)evt.getSource());
    mySwingWorker.addPropertyChangeListener(new PropertyChangeListener() 
    {      
        @Override
        public void propertyChange(PropertyChangeEvent evt) 
        {

            if (evt.getPropertyName().equals("state")) 
            {
                if (evt.getNewValue() == SwingWorker.StateValue.DONE) 
                {
                   loadingDialog.dispose();
                }
            }
        }
    });
    mySwingWorker.execute();
    loadingDialog.setLocationRelativeTo(win);
    loadingDialog.setVisible(true);
  }
}

This class actually does the URL request and parsing of the return data:

 public class LoginHandle 
 {
    public String[] loginData(String account, String username, String password, String devId)
    {
        String[] data = new String[2];
        JsonParser parser = new JsonParser();
        String receipt = "";

        /*
        Creating our trust manager. The program isn't accepting the SSL from the server for some reason. 
        */
        TrustManager[] trustAllCerts = new TrustManager[]
        {
            new X509TrustManager()
            {
                public java.security.cert.X509Certificate[] getAcceptedIssuers()
                {
                    return null;                    
                }
                public void checkClientTrusted(
                  java.security.cert.X509Certificate[] certs, String authType) {
                }
                public void checkServerTrusted(
                  java.security.cert.X509Certificate[] certs, String authType) {
                }
            }
        };

        /*
        Setting trust
        */
        try
        {
            SSLContext sc = SSLContext.getInstance("SSL");
            sc.init(null, trustAllCerts, new java.security.SecureRandom());
            HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
        }
        catch(Exception e)
        {
            System.err.println("Unable to set secure.");
        }

        /*
        Trying to establish the url connection and getting the data back.
        */
        try
        {
            URL url = new URL("https://m2web.talk2m.com/t2mapi/login?" + devId + account + username + password);
            System.out.println(url);
            URLConnection yc = url.openConnection();
            BufferedReader br = new BufferedReader(new InputStreamReader(yc.getInputStream()));
            String strTemp = "";
            while(null != (strTemp = br.readLine()))
            {
                receipt = strTemp;
            }
        }
        catch(Exception e)
        {
            System.err.println("Error getting data.");
        }

        /*
        Parsing the return JSON
        */
        try
        {
            Object obj = parser.parse(receipt);
            JsonObject jsonObj = (JsonObject) obj;
            JsonElement loginSuccess = jsonObj.get("success");
            JsonElement loginSession = jsonObj.get("t2msession");
            data[0] = loginSuccess.toString();
            data[1] = loginSession.toString();
        }
        catch(Exception e)
        {
            System.err.println("Error parsing json");
            e.printStackTrace();
        }

        return data;
    }

}
\$\endgroup\$
6
\$\begingroup\$
  1. lh isn't a clear variable name (unless it's used in immediate context only, but it isn't here). I'd rather call it loginHandle.

  2. devid misses camel case. I'd name it developerId.

  3. String[] loginDataArr;: It's obvious that this is an array. I'd not state it extra.

  4. Why is your URLConnection called yc?

  5. brreminds me of <br /> every time I look at it. And it refers to a technical aspect. I rather like to see where I get data from or where I put them to. I prefer to call my streams/readers/writers inSomething/outSomething. So it'd be inUrlConnection (or inUC if you like it short) in your case. But that might be personal taste.

  6. I'd rename strTemp to line since this is what it is supposed to contain.

\$\endgroup\$
  • \$\begingroup\$ Ah you are very correct in my initial errors. My naming conventions definitely could use some work. \$\endgroup\$ – basic Jun 9 '15 at 15:04
3
\$\begingroup\$

URL Structure

The URL structure is hardcoded in the account/username/etc fields, which doesn't seem like a good idea. It is confusing, and it means that LogIn and LoginHandle are responsible for how the actual remote call looks, instead of having it all in one place.

This means that in case the call ever changes, you have to change your code in two completely different classes.

So what I would do is move the &t2mFIELDNAME= strings into the LoginHandle class.

You don't specify if you have control over how the API works, but if you do, change it. Sensitive data should not be submitted via GET, as URLs are frequently logged (client- as well as server-side), while POST data is mostly not logged.

Exception Handling

Your exception handling could be improved. Just catching and printing is rarely the best choice. It's not user friendly, it's not development friendly (the stacktrace is swallowed), and it's not good for reusability.

It also takes away any control the calling class would have if you just threw the exceptions. What if it doesn't want to print? And what if it doesn't want to continue? Eg Unable to set secure seems like a good reason not to continue, except if specifically order to do so (you could add a fallbackToInsecure flag to loginData for this).

Misc

  • Your indentation is off, and the position of your curly brackets is not what Java programmers are used to.
  • Don't unnecessarily shorten variable names. win can be window, lh can be loginHandle, etc.
  • declare variables as late as possible in as small a scope as possible (see eg parser, data, etc.
\$\endgroup\$
  • \$\begingroup\$ unfortunately I have not control on the API. The first suggestion on changing the placing is awesome. I didn't even think of that when I was initially going through this but it makes total sense. The exception handling makes sense as well. Those were going to be adjusted just my own dumb little bookmarks. MISC- When you say my curly braces what exactly do you mean? Don't start on newline? And I don't fully understand when you say to declare them as late as possible. \$\endgroup\$ – basic Jun 9 '15 at 15:02
  • \$\begingroup\$ @xXspynXx yes, exactly, the should be on the same line (I know, it's nitpicking^^). Any IDE primarily designed for Java will format it that way by default (eg Netbeans or Eclipse). And I meant that eg you have JsonParser parser = new JsonParser(); at the very top of your method, but it's used only at the very bottom, inside a try block. So it would be better to just declare it there (so the reader doesn't have to keep that variable in mind until it actually becomes relevant). \$\endgroup\$ – tim Jun 9 '15 at 15:06
  • \$\begingroup\$ ah well that makes sense. The goal is to become a professional so if that's the more accepted why then I will definitely swap my {} placements. :) Secondly, moving those fields definitely makes sense. Keeping them where they are relevant instead of randomly in places. Great, great tips. Thank you! \$\endgroup\$ – basic Jun 9 '15 at 15:11
1
\$\begingroup\$

This code code not properly escape data for use in a URL:

account = "&t2maccount=" + txtAccount.getText();
username = "&t2musername=" + txtUsername.getText();
password = "&t2mpassword=" + txtPassword.getText();

You must escape data used in the context of a URL to remove ambiguity. What if someone had an ampersand in their password? With this method, the remainder of the password would be cut off.

In any case, URL building does not belong here. You should move the building of the URL into the loginData function where you build the rest of the URL. I'm no Java programmer, but I assume there is a simple method for taking a hash and converting it into a query string. Use it.

\$\endgroup\$
  • \$\begingroup\$ Already addressed based on info from others. Thank you for the tip though. \$\endgroup\$ – basic Jun 9 '15 at 21:14
  • \$\begingroup\$ @xXspynXx Maybe I missed it, but I read them and didn't see this point which is why I posted. Make very sure you are escaping data for use in the query string or POST data if you go that route. \$\endgroup\$ – Brad Jun 9 '15 at 21:15
  • \$\begingroup\$ thank you for the tip like I said anything that would help haha :)) \$\endgroup\$ – basic Jun 9 '15 at 21:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.