Sort a given string in ascending order This code sort the string below in ascending order.
I'd prefer it split-up in two or three smaller, simpler methods. I'm also wondering whether my algorithm has a decent time complexity.

Given string
[H, B, D, G, F, E, A, C]
Output
[A, B, C, D, E, F, G, H]

public class sortArray {
    public static void sort (String[] str)
    {
        int lastPos = str.length - 1;
        int minPos = 0;
        String s = "";
        for (int i = 0; i < lastPos; i++)
        {
            minPos = i;
            for (int j = i + 1; j <= lastPos; j++)
                if (str[j].compareTo (str[minPos]) < 0)
                    minPos = j;
            if (minPos != i)
            {
                s = str[i];
                str[i] = str[minPos];
                str[minPos] = s;
            }
        }
    }

    public static void main(String[] args){
        String[] str = {"H", "B", "D", "G","F", "E", "A", "C"};
        sort(str);
        System.out.println(Arrays.toString(str));
    }
}

Your algorithm is selection sort.

Selection sort has a time complexity of \$O(n^2)\$. For sorting algorithms, that's pretty slow.

Consider looking up a sorting algorithm such as quicksort, which, whilst it does have a worst case performance of \$O(n^2)\$, on average, it tends to take \$O(n log n)\$.

Or, as suggested in the comments, make use of the built-in libraries. Use Arrays.sort. Use of library code, especially when part of the language that you're programming in, can save significantly on the work you need to do.

Splitting up your code into several methods can be done.

Take this section, for instance.

        for (int j = i + 1; j <= lastPos; j++)
            if (str[j].compareTo (str[minPos]) < 0)
                minPos = j;

What it does is it looks for the smallest string in the array. So you could turn this into a function, with arguments for the array and the starting position (array.length will get you the end).

The swap,

        if (minPos != i)
        {
            s = str[i];
            str[i] = str[minPos];
            str[minPos] = s;
        }

Could also be a separate method.

If you extract those two sections of code to separate functions, the main sorting function will look like this:

public static void sort (String[] str)
{
    int lastPos = str.length - 1;
    for (int i = 0; i < lastPos; i++)
    {
        swap(str, i, findSmallest(str, i));
    }
}


Simple C Port Scanner I have been writing a Simple Port scanner in C, once I got it to work I decided I wanted to make it faster because it takes a very long time for it to get done with scanning all of the ports.Is there anything I can improve on in my code to make it faster? Here is my Code :

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <time.h>
#define MAX 65536

int main(int argc, char ** argv)
{
    time_t begin, end;
    time(&begin);

    printf("Scanning ports 0 - 65535 for IP address : %s\n", argv[1]);

    FILE * file = fopen("/home/ether/Desktop/Projects/Ports_log.txt", "w");//open log file

    for (unsigned short port = 0; port < MAX; port++)//loop through and scan all ports
    {
        int Connection = socket(AF_INET,SOCK_STREAM, 0);
        sockaddr_in temp = { 0 };

        //fill the struct with information on the computer you wish to scan
        temp.sin_addr.s_addr = inet_addr(argv[1]);
        temp.sin_family = AF_INET;
        temp.sin_port = htons(port);

        int error = connect(Connection, (sockaddr*)&temp, sizeof temp);
        if(error != -1)
        {
            //if the port is open then get information about that port and print it to the log file
            char host[128];
            char service[128];
            getnameinfo((sockaddr*)&temp, sizeof temp, host, (sizeof host), service, sizeof service, 0);
            printf("Port : %d, Service : %s, Open\n", port, service);
            fprintf(file, "Port : %d, Service : %s, Open\n", port, service);
        }
        close(Connection); //Destroy socket
    }
    fclose(file);//Close the file

    //calculate the time that elapsed since the program was started and rint it
    time(&end);
    double elapsed_secs = difftime(end, begin);
    printf("Elapsed Seconds : %lf", elapsed_secs);
    return 0;
}

After creating a socket make it non-blocking. Then call connect(). If connect returns 0 then the connection has been established and you can use it already. If it retuned -1 and errno is equal to EINPROGRESS then you need to wait until it becomes write-ready using select() or poll(). When it becomes write-ready use getsockopt(sock, SOL_SOCKET, SO_ERROR, ...) to tell if the connection has failed or succeeded. The example code for using non-blocking connect is here:

http://developerweb.net/viewtopic.php?id=3196

But this sample shows waiting on single socket only. You need to wait on a bunch of them at the same time. So have a table of socket connections with some fixed size, e.g. 32:

#define MAX_CONNECT 32

struct socket_info {
    int sock;    /* the socket */
    int ready;   /* initialize to 0 when filling the table, set to 1 when the connection has established or failed */
    int error;   /* set to errno got with getsockopt() for failed connections, set to 0 if connection succeeded */
    sockaddr_in addr;
};

struct socket_info pending_connect[MAX_CONNECT];

First fill this table in a loop calling socket/connect. Then wait in another loop with select until all of the sockets become ready. Set appropriate flags in the table as the sockets become ready.

After the select loop you have info about 32 ports. Print it. Close the sockets. Then proceed with a next bunch of 32 ports.

This approach could be further optimised. You could close a socket as soon as you get info about it, print that info, and immediately open another socket in its place in the table. This will be faster but perhaps this also requires more careful coding.


Binary tree isSame() Please help me review this code. I want to make it both accurate and more Go-styled.

Go Playground code

package main

import(
    "fmt"
)

type TreeNode struct {
    val int
    left *TreeNode
    right *TreeNode

}

func isSameTree(p *TreeNode , q *TreeNode ) (bool){
    if p == nil && q == nil {
        return true
    }
    if p != nil && q == nil{
        return false;
    }
    if p ==nil && q != nil {
        return false;
    }
    if (p.val == q.val) && (isSameTree(p.left,q.left)) && (isSameTree(p.right ,q.right)){
        return true;
    } else {
        return false;
    }
}
func main(){
    p := &TreeNode{val: 1}
    p.left = &TreeNode{val: 2}
    p.right = &TreeNode{val: 3}

    q := &TreeNode{val: 1}
    q.left = &TreeNode{val: 2}
    q.right = &TreeNode{val: 3}

    isSame := isSameTree(p,q)
    fmt.Println("is same?: ", isSame)
}

A quick way to make your code more Go-styled is to have it automatically formatted.  Specifically, spaces are a bit off.  For example, in the isSameTree() signature, you have a space before a comma, a space before the closing parenthesis, and no space before the opening brace.  The parentheses around (bool) are unnecessary.  You're also writing semicolons inconsistently.

The isSameTree() implementation is a bit confusing ("A but not B and not C.  If you made it this far, check D and E and F?").  It would be clearer as one long affirmative expression.

func isSameTree(p *TreeNode, q *TreeNode) bool {
        return (p == nil && q == nil) ||
               (p != nil && q != nil &&
                p.val == q.val &&
                isSameTree(p.left, q.left) && isSameTree(p.right, q.right))
}


HexToString function returning maximum of 16kb The HexToString function returning maximum of 16kb even for sources greater than 16KB

string HexStringToString(string HexString)
        {
            string stringValue = "";
            for (int i = 2; i < HexString.Length / 2; i++) // Starting from position too just to avoid the 0x Hex Code that messes stuff up
            {
                try
                {
                    string hexChar = HexString.Substring(i * 2, 2);
                    int hexValue = Convert.ToInt32(hexChar, 16);
                    stringValue += Char.ConvertFromUtf32(hexValue);
                }
                catch (Exception ExCon)
                {
                    //continue; //Forcing application to ignore anomaly in the file being processed
                    MessageBox.Show(ExCon.ToString());
                }
            }
            return stringValue;
        }

Just a few comments.

First, I'd change the loop a bit. Instead of:

for (int i = 2; i < HexString.Length / 2; i++)
// ...
     HexString.Substring(i * 2, 2);

I'd use something like:

for (int i=2; i<HexString.Length-1; i += 2)
// ...
    HexString.Substring(i, 2);

Note the Length - 1 though--if we happen to be passed a string with odd length, we don't want to index beyond its end.

Secondly, at least for general use, you almost certainly do not want this to catch the exception and display a MessageBox. The basic reason for using exception handling to start with is that a low-level function like this doesn't know (and shouldn't care) how such an exception will be handled/displayed, whether to continue execution after it happens, etc.


Javascript Performance Optimization I am using the code below that loops around thousands of time, I need this to be as fast as possible, however I am not an expert in Javascript and all I know is that this code is faster than doing it with the innerHTML method.

Can it be optimized? If so, how?

    var a,b,c,d,e,
    trEl, tdEl, imgEl

    for (i = 0; i < docs.length; i++) {
      trEl = document.createElement('tr');
      tdEl = document.createElement('td');
      td1El = document.createElement('td');
      td2El = document.createElement('td');
      td3El = document.createElement('td');
      imgEl = document.createElement('img');

      var str
      a = docs[i]['a']
      b = docs[i]['b']
      c = docs[i]['c']
      d = docs[i]['date'] + " - " + docs[i]['timestamp']
      e = docs[i]['_e']

      trEl.setAttribute('id', e);
      trEl.setAttribute('ondblclick', 'scan(' + '"' + e + '"' + ')');
      imgEl.setAttribute('src', 'css/bullet.png');
      tdEl.appendChild(imgEl);

      tdEl.appendChild(document.createTextNode(a));
      td1El.appendChild(document.createTextNode(d));
      td2El.appendChild(document.createTextNode(b));
      td3El.appendChild(document.createTextNode(c));

      trEl.appendChild(tdEl);
      trEl.appendChild(td1El);
      trEl.appendChild(td2El);
      trEl.appendChild(td3El);

      document.getElementById("search-results").appendChild(trEl)
      }

Don't evaluate docs.length in every iteration - it's constant so it only needs to be evaluated once:

for (var i = 0, n = docs.length; i < n; ++i)

Deference docs[i] into an aliased variable so that you're not repeatedly doing an array lookup into the same element, and don't use strings for keys unless the key is variable:

var doc = docs[i];
var a = doc.a;
...

If the above local variables a through e are only used once, don't bother storing them at all, just put them in as trivial parameters when needed (see below)

Use DOM properties instead of setAttribute function calls when you can:

trEl.id = doc._e;                // instead of 'e'
imgEl.src = 'css/bullet.png';

Don't use inline event handlers:

trEl.addEventListener('dblclick', scan, false);

There's no need to pass the variable e to scan since it's just the clicked element's ID and therefore trivially extracted within the event handler:

function scan(event) {
    var e = this.id;
    ...
}

Use a DocumentFragment to store all of the new elements, and then add the entire fragment into the document in one go, allowing the browser to handle all of the visible DOM changes in one hit:

var rows = document.createDocumentFragment();

for (...) {
    ...
    rows.appendChild(trEl);
}

document.getElementById("search-results").appendChild(rows);


Display a List in Console I was searching for a way to display a List<T> with n elements in a user friendly and easy to handle way within the console application. So I wrote this console-list-pagiation method.

Probably there are ways to improve the usability of my approach.

public static void ConsolePaging<T>(this IEnumerable<T> Values)
{
    int Pagesize = Console.WindowHeight - 2;
    int PageWidth = Console.WindowWidth - 1;
    int CurrentPage = 1;
    int PageCount = (Values.Count() + Pagesize - 1) / Pagesize;
    ConsoleKeyInfo keyInfo = new ConsoleKeyInfo();
    do
    {
        if (keyInfo.Key == ConsoleKey.RightArrow || keyInfo.Key == (ConsoleKey)0 || keyInfo.Key == ConsoleKey.LeftArrow)
        {
            if (keyInfo.Key == ConsoleKey.RightArrow && CurrentPage < PageCount)
            {
                CurrentPage++;
            }
            else if (keyInfo.Key == ConsoleKey.LeftArrow && CurrentPage > 1)
            {
                CurrentPage--;
            }
            Console.Clear();
            Console.BackgroundColor = ConsoleColor.White;
            Console.ForegroundColor = ConsoleColor.DarkRed;
            string HeaderText = string.Format("Page:[{0}]/[{1}] │ Records:[{2}] ║ [◄] Previous │ [►] Next │ [Esc] Escape", CurrentPage, PageCount, Values.Count());
            Console.WriteLine(HeaderText.PadRight(Console.BufferWidth - 1, ' '));
            Console.ResetColor();
            foreach (T item in Values.Skip(Pagesize * (CurrentPage - 1)).Take(Pagesize))
            {
                string row = item.ToString();
                if (row.Length > PageWidth)
                {
                    row = row.Substring(0, PageWidth);
                }
                Console.WriteLine(row);
            }
        }
        keyInfo = Console.ReadKey(true);
    } while (keyInfo.Key != ConsoleKey.Escape);
}

usage:

Enumerable.Range(1, 1000).ToList().ConsolePaging();

To my taste it's doing way to much. You'd be better off if you implemented the paging logic for a collection without mixing it with console interaction.

Write another method that expects a collection, page, page length etc. and do the navigation in another method.

If you do this you can test the paging alogorithm without the console and without key input. You'll be also able to test the navigation separately.

Even better, your paging method will be reusable.

To speed up the paging you could group the items by page. If you then ToList() it you can access each page directly without reiterating the collection each time:

public static IEnumerable<IGrouping<int, T>> GroupByPage<T>(this IEnumerable<T> values, int pageSize)
{
    var currentPage = 0;
    var page = new Func<int, int>(index => index > 0 && index % pageSize == 0 ? ++currentPage : currentPage);
    return values
        .Select((v, i) => new { v, i })
        .GroupBy(x => page(x.i), x => x.v)
        .ToList();
}

Here's another example with a decorator:

First we create an interface:

public interface IPager<out T>
{
    int PageIndex { get; }
    int PageSize { get; }
    int PageCount { get; }
    IGrouping<int, T> CurrentPage { get; }
    IGrouping<int, T> this[int page] { get; }
    void Next();
    void Prev();
}

Then we create a general pager:

public class Pager<T> : IPager<T>
{
    private int _pageIndex;

    private readonly IEnumerable<IGrouping<int, T>> _pages;

    public Pager(IEnumerable<T> values, int pageSize)
    {
        PageSize = pageSize;
        var currentPage = 0;
        var page = new Func<int, int>(index => index > 0 && index % pageSize == 0 ? ++currentPage : currentPage);
        _pages = values
            .Select((v, i) => new { v, i })
            .GroupBy(x => page(x.i), x => x.v);
    }

    public int PageIndex => _pageIndex;

    public int PageSize { get; }

    public int PageCount => _pages.Count();

    public IGrouping<int, T> this[int page] => _pages.ElementAtOrDefault(page);

    public IGrouping<int, T> CurrentPage => this[_pageIndex];

    public void Next() => _pageIndex++;

    public void Prev() => _pageIndex--;

}

Now we create another pager but this time it can display the item in the console:

public class ConsolePager<T> : IPager<T>
{
    private readonly IPager<T> _pager;

    public ConsolePager(IPager<T> pager)
    {
        _pager = pager;
    }

    public int PageIndex => _pager.PageIndex;

    public int PageSize => _pager.PageSize;

    public int PageCount => _pager.PageCount;

    public IGrouping<int, T> CurrentPage => _pager.CurrentPage;

    public void Next() => _pager.Next();

    public void Prev() => _pager.Prev();

    public IGrouping<int, T> this[int page] => _pager[page];

    public void Run()
    {
        var pageWidth = Console.WindowWidth - 1;
        var keyInfo = new ConsoleKeyInfo();

        do
        {
            if (keyInfo.Key == ConsoleKey.RightArrow || keyInfo.Key == (ConsoleKey)0 || keyInfo.Key == ConsoleKey.LeftArrow)
            {
                if (keyInfo.Key == ConsoleKey.RightArrow && _pager.PageIndex < _pager.PageCount - 1)
                {
                    _pager.Next();
                }
                else if (keyInfo.Key == ConsoleKey.LeftArrow && _pager.PageIndex > 1)
                {
                    _pager.Prev();
                }
                Console.Clear();
                Console.BackgroundColor = ConsoleColor.White;
                Console.ForegroundColor = ConsoleColor.DarkRed;
                var headerText = string.Format("Page:[{0}]/[{1}] │ Records:[{2}] ║ [◄] Previous │ [►] Next │ [Esc] Escape", _pager.PageIndex + 1, _pager.PageCount, _pager.CurrentPage.Count());
                Console.WriteLine(headerText.PadRight(Console.BufferWidth - 1, ' '));
                Console.ResetColor();

                foreach (var item in _pager.CurrentPage)
                {
                    var row = item.ToString();
                    if (row.Length > pageWidth)
                    {
                        row = row.Substring(0, pageWidth);
                    }
                    Console.WriteLine(row);
                }
            }
            keyInfo = Console.ReadKey(true);
        } while (keyInfo.Key != ConsoleKey.Escape);
    }
}

Usage:

new ConsolePager<int>(
    new Pager<int>(Enumerable.Range(1, 1000),
    Console.WindowHeight - 2)
).Run();


Is this routine for determining the epsilon of a Double 100% valid? This algorithm is intended to determine the epsilon of a given double precision number. As numbers increase in value, their accuracy decreases. This algorithm will return the smallest increment / decrement possible at any given value.

Given the way floating point numbers are structured, is there logically any situation where this algorithm won't work? It has worked fine for whatever numbers I've thrown at it.

public enum EZeroDirection { TowardsZero, AwayFromZero };

[StructLayout(LayoutKind.Explicit)]
private struct DL
{
    [FieldOffset(0)]
    internal double D;

    [FieldOffset(0)]
    internal long L;
}

/// <summary>
/// Returns the dynamic epsilon of this Double. If the value is NaN or ±Inf, or the initial result overflows, the result will be null.
/// </summary>
/// <remarks>See (http://bit.ly/10KS2wo)</remarks>
/// <param name="dir">Determines if the epsilon is calculated from the delta of its next value towards zero or away from zero. Choose the direction
/// the value moves in. Note: EZeroDirection.TowardsZero allows proper processing of Double.MaxValue and Double.MinValue.</param>
/// <returns>Double?</returns>
public static Double? DynamicEpsilon(this Double a, EZeroDirection dir = EZeroDirection.TowardsZero)
{
    if (!double.IsNaN(a) && a != double.NegativeInfinity && a != double.PositiveInfinity)
    {
        DL dl = new DL();
        dl.D = a;
        dl.L -= ((dir == EZeroDirection.TowardsZero) ? 1 : -1);

        if (!double.IsNaN(dl.D))
        {
            return Math.Abs(a - dl.D);
        }
    }

    return null;
}

a != double.NegativeInfinity && a != double.PositiveInfinity

This could be simplified to just double.IsInfinity(a). Though as far as I know, it is correct. I was worried that there may be more values representing infinity, like there are for NaN, but that doesn't seem to be the case.

Otherwise, your method seems to work to me in all edge cases, including +/-0, double.MaxValue and double.Epsilon (though you should write unit tests for these just in case). In the case of +/-0 and TowardsZero, D will become NaN, so your method returns null, which I think is correct behavior.


Using inheritance and polymorphism in Java I'm trying to use inheritance and polymorphism to make it easy for the program to be enhance to many more rules. ClassificationRule is the base class and RuleFirstOccrnc and RuleOccrncCount and derived classes and by using the same non-static apply() method ClassificationRule should invoke all other non-static apply() method. I'm just starting on Java and I'm not sure if I'm doing it right. Can anyone tell me if I'm using inheritance or just calling the apply() method?

//PrioritRule Class
public class PrioritRuls {
     final static ArrayList<ClassificationRule> rulesA
            = new ArrayList<ClassificationRule>();

     static{
            rulesA.add( new RuleFirstOccrnc() );
            rulesA.add( new RuleOccrncCount() );
     }
     public static void prioritize( final String aUserInput ){
            for (ClassificationRule rule:rulesA)
                    rule.apply(aUserInput);
    }
}

// ClassificationRule Class - Based Class
public class ClassificationRule {

     public void apply (final String aUserInput) {
           apply( aUserInput );
     }
}

//RuleFirstOccrnc - Derived Class
public class RuleFirstOccrnc extends ClassificationRule {
     public void apply ( final String aUserInput ) {
          for( TweetCat t: TwtClassif.tCat )
               applyFirstOccurrenceRuleTo( t, aUserInput );
}
   * * *

//RuleOccrncCount - Derived Class
public class RuleOccrncCount extends ClassificationRule {
     public void apply ( final String aUserInput ) {
          for( TweetCat t: TwtClassif.tCat )
               applyOccurrenceCountRuleTo( t, aUserInput );
}
  * * *

In your code, two classes differently extend a base class ClassificationRule.
Objects of these classes are added to a list, cast as the base class.
They are retrieved from the list and a method, apply(), which is present in the base class but overridden in the child classes.
As you intend, the apply() method as present in the child class will run, not the apply() method from the base class.

In the case of your code, the child class does not actually gain functionality from the base class. You are inheriting solely in order manipulate objects of different types as if they were of one homogenous type, because you know they provide certain functionality.

In software engineering terms, you would say 'the classes implement an interface'.
An interface looks very much like an empty base class, like the one you wrote, and it is a Java language feature different from a base class. Implementing an interface (i.e. providing some agreed functionality) is different from inheriting functionality.

And the code for it is different, you would write public interface ClassificationRule {, and public class RuleFirstOccrnc implements ClassificationRule {.

In Java, you can only inherit from one class (thought that class may itself have a parent). But you can implement as many interfaces as you like. A typical java object will implement interfaces that let it be sorted (comparable), serialized to transmissable data (serializable), etc, but can only ever inherit from a single base class.
C++ lets you inherit multiple base classes. Things go horribly wrong when you do. Or at least when I do.


"Guess the Number" in C This is my first program in C. Do you have any improvements to this program? The program allows you to guess a number and also returns the attempts.
#include <stdio.h>
#include <stdlib.h>
#include <conio.h>
#include <time.h>
/*
 * Sophie
 * 21.03.2021
 */
void main() {
     int secret;
     int guess;
     int try = 1;

    time_t t;
    srand((unsigned ) time(&t));
    secret = rand() % 100+1;

    printf("Please Enter value:\n");
    scanf("%i", &guess);

    while (guess != secret){
        if (guess > secret)
            printf("Guess too high. \n");
        else
            printf("Guess too low. \n");

        printf("Please enter a new value:\n");
        scanf("%i", &guess);
        try++;
    }
    printf("You gussed the number! \n");
    printf("You needed %i Tries.", try);

    getch();
}

void main is a really bad habit. It must be int main.

Always check what scanf returns. For example, try to enter a non-numeric input, and see your program entering the infinite loop.

Avoid conio.h (and hence getch). It is very non-portable.


Usage of static block vs. direct static variable initialization Suppose, I have a data access layer class, where I want to specify database connection parameters. Usually, I do it as static variables and initialize it right after definition, e.g. see DB_NAME variable case.

Performing code review, I paid attention that sometimes such logic is implemented with a static block, e.g. see DB_HOST variable case.

public class DAL {

    private static final String DB_NAME = "MY_DB";
    private static final String DB_USER = "DB_ADMIN";
    private static final String DB_HOST;

    static {
        DB_HOST = "127.0.0.1";
    }
}

The questions:

Is there any benefit from using a static block in such specific scenario?
When should I prefer to use a static block over a regular static variable initialization?

P.S. I found a similar question on SO: Why use static blocks over initializing instance variables directly?, but if you have something to add, you're welcome.

In such scenario it only obstructs code readability. No benefits.
You should use static block only if static field initialization is not enough (e.g. you should call several methods, or this methods throw checked exceptions).

Just created 2 sample classes:

public class Static {
    public static String s = "qwe";
}

public class StaticBlock {
    public static String s;
    static {
        s = "qwe";
    }
}

And using javap -c we can see bytecodes of this classes:

Compiled from "Static.java"
public class Static {
  public static java.lang.String s;

  public Static();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return

  static {};
    Code:
       0: ldc           #2                  // String qwe
       2: putstatic     #3                  // Field s:Ljava/lang/String;
       5: return
}

Compiled from "StaticBlock.java"
public class StaticBlock {
  public static java.lang.String s;

  public StaticBlock();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return

  static {};
    Code:
       0: ldc           #2                  // String qwe
       2: putstatic     #3                  // Field s:Ljava/lang/String;
       5: return
}

As you can see, bytecodes are identical :)

UPD: Its getting more interesting! If we add final modifier to our static field, bytecodes will be different.

public class Static {
    public static final String s = "qwe";
}

public class StaticBlock {
    public static final String s;
    static {
        s = "qwe";
    }
}

We will have:

Compiled from "Static.java"
public class Static {
  public static final java.lang.String s;

  public Static();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return
}

Compiled from "StaticBlock.java"
public class StaticBlock {
  public static final java.lang.String s;

  public StaticBlock();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return

  static {};
    Code:
       0: ldc           #2                  // String qwe
       2: putstatic     #3                  // Field s:Ljava/lang/String;
       5: return
}

I found a little bit of explanation in this question.


Beginning Python guessing game I am new to Python (and coding in general) and after about a week of reading "Thinking Like a Computer Scientist: Learning with Python" I decided to try and build a version the classic "guessing game". I added some extra features such as counting the number of guesses the user takes, and playing against a simulated "computer" player to make the program slightly more interesting. Also, the number of guesses the computer takes is based on the mean number of guesses needed to guess a number in a given range (which is logarithmic of base 2 for range n) and varies according to standard deviation.

Any feedback on the structure of my code or the way I generate the number of guesses the computer takes would be much appreciated!

# Number guessing game in Python
# Taylor Wright
# July 27 2016

import random

def get_number(level):                  #selects a random number in range depending on difficulty selected
    if level == "e":
        number = random.randint(1,20)
    if level == "m":
        number = random.randint(1,100)
    if level == "h":
        number = random.randint(1,1000)
    elif level != "e" and level != "m" and level != "h":
        print ("Invalid input!")
        get_number()
    return number

def select_level():    #prompts the user to select a difficulty to play on
    while True:
        level = str(input("Would you like to play on easy, medium, or hard? \n"
                      "Type 'e' for easy, 'm' for medium, or 'h' for hard!\n"))
        if level != "e" and level != "m" and level != "h":
            print("Invalid input!\n")
        if level == "e" or level == "m" or level == "h":
            break
    return level

def guess_number(level):        #function that prompts the user to guess within range depending on chosen difficulty
    if level == "e":
        guess = int(input("Guess a number between 1 and 20:\n"))
    if level == "m":
        guess = int(input("Guess a number between 1 and 100:\n"))
    if level == "h":
        guess = int(input("Guess a number between 1 and 1000:\n"))
    return guess

def check_guess(guess,number):         #processes the guess and tells the user if it is too high, too low, or bang on
    if guess > number:
        print ("your guess is too high! Try again! \n")
    if guess < number:
        print ("your guess is too low! Try again! \n")
    if guess == number:
        print("\n{0} was the number!".format(number))

def com_num_guesses(level):          #function to get the number of guesses taken by the computer
    if level == "e":
        com_guesses = round(random.normalvariate(3.7,1.1))
    if level == "m":
        com_guesses = round(random.normalvariate(5.8,1.319))
    if level == "h":
        com_guesses = round(random.normalvariate(8.99,1.37474))
    print("The computer guessed the number in {0} guesses! Can you beat that?".format(com_guesses))
    return com_guesses

def mainloop():
    level = select_level()
    number = get_number(level)
    com_guesses = com_num_guesses(level)
    num_guesses = 0
    while True:                 #tells program how to handle guesses after the first guess
        guess = guess_number(level)
        check_guess(guess,number)
        num_guesses += 1
        if guess == number:
            print( "You got it in {0} guesses.".format(num_guesses))
            if num_guesses == com_guesses:
                print("It took the computer {0} guesses too!\nIt's a tie!\n".format(com_guesses))
            if num_guesses > com_guesses:
                print("It took the computer {0} guesses.\nThe computer wins!\n".format((com_guesses)))
            if num_guesses < com_guesses:
                print("It took the computer {0} guesses.\nYou win!\n".format(com_guesses))
            play_again = str(input("To play again type 'yes'. To exit type 'no'. \n"))
            if play_again == "yes":
                mainloop()
            if play_again == "no":
                raise SystemExit(0)
            break

mainloop()

Without changing the global structure of your code, here are a few things that could make it better.

NB: I will try to cover as much as I can, skipping objects and if-reduction, as it has already been covered in another answer

Docstrings

You use regular comments at the beginning of your functions. Their purpose is to indicate what the function does, therefore they should be docstrings. The difference is that it is stored in the __doc__ special variable.

def get_number(level):
    """selects a random number in range depending on difficulty selected"""
    if level == "e":
        # rest of the function

About get_number

You could use a dictionary of levels to make it shorter and easier to understand. Also more flexible when you want to change levels later, or add new levels.

levels = {'e': 20, 'm': 100, 'h': 1000}

The content of the function is then something like:

if level != "e" and level != "m" and level != "h":
    print ("Invalid input!")
    get_number()
else:
    number = random.randint(1, levels[level])
return number

Or even better, try and catch a KeyError that is raised when the key does not exist. Final get_number function:

levels = {'e': 20, 'm': 100, 'h': 1000}

def get_number(level):
    """selects a random number in range depending on difficulty selected"""
    try:
        number = random.randint(1, levels[level])
    except KeyError:
        print ("Invalid input!")
        get_number()
    return number

About guess_number

Golden rule : never rely on a user input (you can read about it here for example). As we've seen before, the user does not always enter what you asked for, and this can cause your game to crash. Fortunately, using try/except can solve this problem. I've kept the use of the levels dictionary here.

def guess_number(level):
    """function that prompts the user to guess within range
    depending on chosen difficulty"""
    try:
        guess = int(input("Guess a number between 1 and "+str(levels[level])+":\n"))
    except KeyError:
        print('This should never have happened. "level" is not part of the levels')
    except (ValueError, NameError, SyntaxError):
        print('Invalid input\n')
        guess = guess_number(level)
    return guess

Naming arguments

Sometimes functions can be obscure, it can be helpful for others (and future you) to name arguments, as Python allows nicely.

if level == "e":
    com_guesses = round(random.normalvariate(mu=3.7, sigma=1.1))

This makes it way easier to understand.

Magic numbers

Speaking of the line of code just above, 3.7 and 1.1 seem to come out of nowhere. Two solutions to get this straight:

Create constants (not my favorite)

For instance,

EASY_MU = 3.7
EASY_SIGMA = 1.1
MEDIUM_MU = 5.8
# ...

and the line becomes much more understandable

if level == "e":
    com_guesses = round(random.normalvariate(mu=EASY_MU, sigma=EASY_SIGMA))

Use dictionaries (again)

We could improve the levels dictionary to make it contain what we want for later.

levels = {'e': {'size': 20, 'mu': 3.7, 'sigma': 1.1},
          'm': {'size': 100, 'mu': 5.8, 'sigma': 1.319},
          'h': {'size': 1000, 'mu': 8.99, 'sigma': 1.37474}
          }

Then the whole method becomes way more readable

def com_num_guesses(level):
    """function to get the number of guesses taken by the computer"""
    try:
        com_guesses = round(random.normalvariate(mu=levels[level]['mu'], sigma=levels[level]['sigma']))
    except KeyError:
        print('This should never have happened. "level" is not part of the levels')
    print("The computer guessed the number in {0} guesses! Can you beat that?".format(com_guesses))
    return com_guesses

Also note that in get_number and guess_number, we will now have calls to levels[level]['size'] since the dictionary has changed.

Lauching

It is recommended to encapsulate your mainloop function within an if __name__ == '__main__' condition. That way, it will not get run if this module is imported by someone else. More information on that here.

if __name__ == '__main__':
    mainloop()

Global remarks

You used a lot of recursive calls. This is OK, since the number of calls remains small (once per user mistake). But I personally have a preference for while True loops that break as soon as the user input is correct. It uses less memory.

This answer is already quite long, and I've given you pieces of information to improve your code, not revolutionized the whole structure, that's why I didn't copy the whole code at the end.


Remove item after a specified item from a linked list I have written a function that deletes the item that is next to the one being passed as a parameter. So far this function deletes but I feel like there is a better way to implement this.

public class MyLinked{
private class Node{
    Node next;
    double item;
    public Node(double item, Node next) {
        this.item = item;
        this.next = next;
    }
    public Node() {

    }
}
Node first;

Here is the function:

public void removeAfter(double x) {
    if(first == null) { return;}
    else {
        Node c = first;
        while(c!=null){
            if(c.item == x && c.next!=null) {
                c.next = c.next.next;
            }
            c = c.next;
        }
    }
}

if(first == null) { return;}
    else {
        Node c = first;
        while(c!=null){
            if(c.item == x && c.next!=null) {
                c.next = c.next.next;
            }
            c = c.next;
        }
    }

This could be written more briefly as

    for (Node c = first; c != null; c = c.next) {
        if (c.item == x && c.next != null) {
            c.next = c.next.next;
            return;
        }
    }

You don't need the first if, as the code has the same effect without it.

A for loop better captures the iterative properties here.

If we return immediately after doing our task, we save some iterations.

Another alternative would be

    if (first == null) {
        return;
    }

    for (Node current = first; current.next != null; current = current.next) {
        if (current.item == needle) {
            current.next = current.next.next;
            return;
        }
    }

I would prefer not to put everything on the same line.  This would be even more true if there was an else.

Since we return, we don't need an else.  This has the same effect without it, and we save the extra indentation.

I find current more descriptive than c.  Not a huge deal for a looping variable.

Since we know that first is not null, we can go ahead and check the next.  This saves us an iteration if we're looking for the last item.  This also saves having to check next later.

I find needle (meaning something for which we're searching) more descriptive than x.  That's a common idiom, but you can say target, etc. if that doesn't work for you.

You might consider returning the value that you remove.

            double value = current.next.item;
            current.next = current.next.next;
            return value;

This is common with remove operations.  You have the value right there at the time.  And this saves having to traverse the list twice to get the item and then remove it.  Of course, perhaps your usage patterns never have you doing that.


Finding the lowest perfect square that can be added to a number to make another perfect square The code below returns the smallest perfect square that can be added to a number n to result in a perfect square. It works perfectly fine but I need it to run faster.
def solve n
  (1..n).each do |i|
    i = i**2 + n
    return i -= n if (Math.sqrt(i) % 1).zero?
  end
  -1
end

p solve(13) #36
 # because 36 is the smallest perfect square that can be added to 13 to form a perfect square => 13 + 36 = 49

p solve(3) #= 1 # 3 + 1 = 4, a perfect square
p solve(12) #= 4 # 12 + 4 = 16, a perfect square
p solve(9) #= 16
p solve(4) #= -1`

I can see a few changes you can do in your code.
First one, save the calculated square and use it in the next iteration:
def solve2(n)
  # Start with an initial zero value
  pow = 0
  (1..n).each do |i|
    # This is the same as i**2, but using the square previously calculated.
    # Google squares and pascal triangle to more insight about this, but
    # in terms of cpu usage, this is faster.
    pow = pow + (i - 1) * 2 + 1
    r = pow + n
    return r -= n if (Math.sqrt(r) % 1).zero?
  end
  -1
end

And yes, it's not a big improvement, but it works. Trying in a worst case scenario, where no value is found so it must check every different iteration:
irb(main):250:0> require 'benchmark'
=> false
irb(main):251:0>
irb(main):252:0> Benchmark.bm do |x|
irb(main):253:1*   x.report { solve 50_000_002 }
irb(main):254:1>   x.report { solve2 50_000_002 }
irb(main):255:1> end
       user     system      total        real
   9.072362   0.029596   9.101958 (  9.783397)
   8.491063   0.030566   8.521629 (  9.159671) # ~600ms faster than the previous one, yay!

Next thing, instead of iterating between 1 and n, you need to iterate only integer values between Math.sqrt(1**2 + n) and Math.sqrt(n**2 + n). For example, using the current code with the same 50_000_002 value of the benchmark:
irb(main):358:0> Math.sqrt(1**2 + 50_000_002)
=> 7071.068023997506 # 1st iteration, for sure this will fail with ( % 1).zero?
irb(main):359:0> Math.sqrt(2**2 + 50_000_002)
=> 7071.068236129531 # 2nd one still with decimals
irb(main):360:0> Math.sqrt(3**2 + 50_000_002)
=> 7071.068589682892 # and so on...
irb(main):361:0> Math.sqrt(4**2 + 50_000_002)
=> 7071.069084657567
irb(main):362:0> Math.sqrt(5**2 + 50_000_002)
=> 7071.069721053526

as you can see, for the first values of the iteration, you're getting pretty much the same integer value with only decimals of difference. Eventually you'll be close to get an integer value much further in the 114th iteration:
irb(main):392:0> Math.sqrt(114**2 + 50_000_002)
=> 7071.986849535285
irb(main):393:0> Math.sqrt(115**2 + 50_000_002)
=> 7072.003040157718 # that was close, but not enough to get a integer. Keep trying...

So checking all those 114 values were useless, because their results weren't even integers. After a while, reaching the last values, you still don't get a single integer value:
irb(main):394:0> Math.sqrt(50_000_000**2 + 50_000_002)
=> 50000000.500000015
irb(main):395:0> Math.sqrt(50_000_001**2 + 50_000_002)
=> 50000001.50000001
irb(main):396:0> Math.sqrt(50_000_002**2 + 50_000_002)
=> 50000002.5

Then, the problem was to check float values when we need to check only when values have no decimals. In other words, instead of iterate between 1 and 50_000_002, you must iterate between 7072 (ceiling value for result of Math.sqrt(1**2 + 50_000_002), taken from your first previous iteration) and 50_000_002 (floor result of Math.sqrt(50_000_002**2 + 50_000_002), your last iteration, which at the end is the same value as n).
Why using this new perspective? mostly because Math.sqrt is an expensive operation compared with +, - or * (or even ** if the solve2 improvement hasn't been applied). I'll try to explain as much as I can in the code:
def solve3(n)
  # Lowest square root result. This will be the starting point
  lowest_sqrt = Math.sqrt(1 + n).ceil
  # then getting first i value to compare. This is the last time using
  # Math.sqrt
  i = Math.sqrt(lowest_sqrt**2 - n).to_i
  # Keeping a flag, which will be the value to return later
  flag = i
  # Initial values for perfect square, where will be used the base value from
  # previous iteration (that's why i - 1) to make the power replacement as in
  # solve2
  pow = (i - 1)**2
  # and result after adding the n value.
  res = sq(pow, i) + n
  loop do
    pow = sq(pow, i)
    # When square and result are the same, that's our lowest perfect square
    return flag**2 if res == pow

    # In case result is lower, result must be recalculated with new square
    # value.
    if res < pow
      flag = i
      res = pow + n
    end

    i += 1
    break if i > n
  end
  -1
end

# pascal triangle trick
def sq(pow, i)
  pow + (i - 1) * 2 + 1
end

This is a significant improvement compared with previous implementations:
irb(main):908:0> Benchmark.bm do |x|
irb(main):909:1*   x.report { solve 50_000_002 }
irb(main):910:1>   x.report { solve2 50_000_002 }
irb(main):911:1>   x.report { solve3 50_000_002 }
irb(main):912:1> end
       user     system      total        real
   9.077371   0.036185   9.113556 ( 10.015861)
   8.486830   0.026608   8.513438 (  9.090664)
   4.709684   0.012402   4.722086 (  4.925373)

And I think there's still space to improve the code. I have the feeling there are some useless comparisons in the loop that can be avoided, but I've spent a while explaining all this, so I'll leave that to you :)


Copying a large number of DOM elements The aim is to duplicate and append each .grid__item into the .gallery element. The following code was my first instinct, and works fine, but can't help wonder if there is a more efficient way to write this. My concern is when the element count gets up to a high amount, that there will be some delay.

Generate Dummy Elements (PHP)

    <?php for ($i = 0; $i < 100; $i++) :
        $randH = rand(500, 1010);
        $randW = rand(500, 1010);
        $imgUrl = 'http://placehold.it/' . $randH . 'x' . $randW;
    ?>

            <img src="<?php echo $imgUrl; ?>">

<?php echo $imgUrl; ?>

    <?php endfor;?>

Copy Elements (JS)

let $gridItems = document.querySelectorAll('.grid__item'),
    $gallery = document.querySelector('.gallery');

let timeBefore = new Date().getTime();

for (let i = 0; i < $gridItems.length; i++) {
    $gallery.innerHTML += '
' + $gridItems[i].innerHTML + '
';
}

let timeAfter = new Date().getTime();

console.log(timeAfter - timeBefore)

As a followup to your "selfie" answer :D

new Date().getTime()

// to

Date.now()

The effect is the same, but the difference is that you avoid generating a new Date object for the operation. Date.now() just returns the current timestamp.

$galleryItem.innerHTML = $gridItems[i].innerHTML

One problem with innerHTML is that it carries the state of whatever it was you fetched it from. This means if something happens to add the attribute data-foo="bar" into the element or its descendants, this gets copied over. You don't know what data-foo="bar" entails, and there might be side-effects.

An actual case is jQuery using an internal cache mapping of element references. It uses key of that mapping as a custom attribute on the element referenced. Using innerHTML would carry over this attribute to the new stuff, when it should not.


StringBuilder in C v2: Not a charged dance A follow-up on this. To quote by copy/paste:

  Lots of languages have something that lets you build up a dynamically-sized string with minimal overhead. C doesn't, and I found myself using code that did that manually in a couple of places, so I packaged it into a class. Note that I've only implemented functionality I'm using.

stringbuilder.h

It's the same as last time.

#ifndef CONCATEN_STRINGBUILDER_H
#define CONCATEN_STRINGBUILDER_H
#include <stddef.h>
#include <stdbool.h>

struct StringBuilder;
typedef struct StringBuilder *StringBuilder;
StringBuilder sb_new(size_t);
bool sb_append(StringBuilder, char);
char *sb_as_string(StringBuilder);
char *sb_free_copy(StringBuilder);
size_t sb_size(StringBuilder);
void sb_free(StringBuilder);

#endif //CONCATEN_STRINGBUILDER_H

stringbuilder.c

#include <stdlib.h>
#include <string.h>
#include "stringbuilder.h"

struct StringBuilder {
    char *mem;
    size_t count;
    size_t cap;
};
StringBuilder sb_new(size_t init_cap) {
    if (init_cap == 0) {
        return NULL;
    }
    StringBuilder ret = malloc(sizeof(struct StringBuilder));
    if (!ret) return NULL;
    ret->mem = calloc(init_cap, sizeof(char));
    if (!ret->mem) {
        free(ret);
        return NULL;
    }
    ret->cap = init_cap;
    ret->count = 0;
    return ret;
}
#define LOAD_FACTOR 2
bool sb_append(StringBuilder to, char c) {
    if (to->count + 1 == to->cap) {
        char *new_mem = realloc(to->mem, to->cap * LOAD_FACTOR);
        if (!new_mem) {
            return false;
        }
        memset(new_mem + to->cap, 0, to->cap);
        to->mem = new_mem;
        to->cap *= LOAD_FACTOR;
    }
    ++to->count;
    to->mem[to->count - 1] = c;
    return true;
}
char *sb_as_string(StringBuilder sb) {
    return sb->mem;
}
char *sb_free_copy(StringBuilder sb) {
    char *ret = malloc((sb->count + 1) * sizeof(char));
    if (!ret) {
        return NULL;
    }
    strcpy(ret, sb->mem);
    ret[sb->count] = 0;
    sb_free(sb);
    return ret;
}
size_t sb_size(StringBuilder sb) {
    return sb->count;
}
void sb_free(StringBuilder sb) {
    if (sb) {
        if (sb->mem) free(sb->mem);
        free(sb);
    }
}

This time around, I'm concerned most about... exactly the same things:

  Performance. This code gets called a lot. I want it to be as fast as possible.
  Memory safety. While I'm fairly sure that this doesn't leak memory (assuming it's used properly), I'm not confident, and I'm not
  sure how to check.
  Edge cases. It works, as far as I can tell, but that doesn't mean that it's bug-free.

Also,

Portability. Will this code be compileable on any (C11-conforming and/or major) compiler? I want the code that uses this to be as simple to deploy on as many platforms as possible.

Note that I specifically am not asking for advice on functions that would be nice to have. I'm only spending the time to implementing things I'm going to use; this isn't part of a general-purpose utility library, so I really don't care about supporting all the features when I use a tiny fraction of them.

The main reason why C don't have a system like that is because that produce some overhead. But I suppose you know this and want the less overhead possible and this system.

The two following lines are not C idiomatic:

struct StringBuilder;
typedef struct StringBuilder *StringBuilder;

Of course, they are two side about this, I will give you my vision. You should not hide the implementation of structure in C. Why? Because C is design to trust the user, if your user want to "play" with his data, you can't prevent it. C allow user to hack easily and this could improve the use of your library. For example, if your library don't have a code functionality the user could code it.

The second problem is that you hide the pointer in the typedef. You should not hide pointer, this made the code hard to read. And the user could think that this is not a pointer and try to write StringBuilder *foo;. A answer about this is available on stackoverflow.

For the following, I will assume that the user know that you hide the pointer and I will use explicit struct StringBuilder in the code.

Next I will talk about your function sb_new(), first I think you should not allocate the structure StringBuilder, this made a lot of overhead. You should separate your function into two sb_new() and sb_init(), one if the user want allocate the structure and one if the user take care of it.

Plus, you use calloc(), this add a lot of useless overhead. You should use malloc(). If the user want to affect the string by zero you should implement an other function for it, like sb_unused_zero().

And, if the user want a zero size string you send NULL, I don't see the point. Maybe you should use this value for tell to the function to use a default value like 42.

A detail about your call of malloc(), you could write struct StringBuilder *ret = malloc(sizeof *ret);, this is more readable and you can read this answer about it. You don't need to use sizeof char because by definition the is equal to 1.

Example:

struct StringBuilder *sb_new(size_t init_cap) {
    struct StringBuilder *sb = malloc(sizeof *sb);
    if (!sb) return NULL;
    if (!sb_init(sb, init_cap)) {
        free(sb);
        return NULL;
    }
    return sb;
}

bool sb_init(struct StringBuilder *sb, size_t init_cap) {
    init_cap = init_cap == 0 ? 42 : init_cap;
    sb->mem = malloc(init_cap);
    if (!sb->mem) return false;
    sb->count = 0;
    sb->cap = init_cap;
    return true;
}

void sb_unused(struct StringBuilder *sb, char c) {
    memset(sb->mem + to->count, c, sb->cap - sb->count);
}

This allow the user to do the following:

struct foo {
    struct StringBuilder sb;
};

int main(void) {
    struct foo foo;
    sb_init(&foo.sb, 0);
}

Let talks about sb_append(), you don't check the potential overflow when you multiply by your LOAD_FACTOR, so you have an undefined behavior.

In this function, you affect new allocate byte to zero, this is again a big overhead that is useless in most of use cast. Again, the user should use the sb_unused() function that affect unused byte in your string. Plus, you suppose that your LOAD_FACTOR is equal to 2 in your use of memset().

bool sb_append(StringBuilder sb, char c) {
    if (sb->count + 1 == sb->cap) {
        if (SIZE_MAX / LOAD_FACTOR < sb->cap) return false; // this could be improve
        size_t new_cap = sb->cap * LOAD_FACTOR;
        char *new_mem = realloc(sb->mem, new_cap);
        if (!new_mem) return false;
        sb->mem = new_mem;
        sb->cap = new_cap;
    }
    sb->mem[to->count++] = c;
    return true;
}

Your next function sb_as_string() suppose that mem is already a valid c-string, maybe you should verify that your string contain a nil terminate byte.

I almost finish, the function sb_free_copy() has a strange name, maybe rename it to something like sb_into_string(). And your implementation is strange you add yourself the nil terminate byte contrary to the function sb_as_string(), more strcpy() already do it. Whatever, you should use memcpy() because you already know the good size to copy this would be faster.

Just a detail in sb_free() you verify that sb->mem is not NULL but free() handle this, so this is useless and add overhead.

Finally, maybe your structure should be rename String because I think the user could use it to keep dynamic string and not to transform it into standard c-string?

The last thing is about your use about bool, this limit the detail about the error. Maybe you could replace it by an enum of just an int that will give you more flexibility about error report.


Determine if a binary tree is balanced I'm currently running through some Cracking the Coding Interview questions and my solution to 4-4 is quite different from any of the given solutions.

The question asks to determine if a given binary tree is balanced; more specifically, if the difference between the maximum and minimum depths within the tree is less than or equal to 1.

public static bool IsBalanced(Node node)
{
    var mm = new DepthMinMax();
    mm.Min = int.MaxValue;
    mm.Max = int.MinValue;

    FindMinMax(mm, node, 0);

    return (mm.Max - mm.Min <= 1) ? true : false;
}

private static void FindMinMax(DepthMinMax mm, Node node, int depth)
{
    if (node == null) return;

    FindMinMax(mm, node.L, depth + 1);
    FindMinMax(mm, node.R, depth + 1);

    // At a terminating node
    if (node.L == null || node.R == null)
    {
        if (mm.Min > depth) mm.Min = depth;
        if (mm.Max < depth) mm.Max = depth;
    }
}

public class Node
{
    public Node L { get; set; }
    public Node R { get; set; }
}

public class DepthMinMax
{
    public int Min { get; set; }
    public int Max { get; set; }
}

This works for my test cases, but they were far from exhaustive and I can't really conclude if my solution works for all cases / is efficient. My solution feels far too simple, which leads me to believe I've missed some kind of gotcha.

Came across this post as I was answering the same question in this book. My approach was also similar in solving this. I will post what I found
as an answer since I cannot comment.

First, I think the or condition for checking the terminating
node is incorrect. It should be a and condition to my understanding. I think we need to identify whether the current node is a leaf node before
updating depths.
For the following tree this would give a max depth of 3 and min depth of 1, which is incorrect. I did this in C++ as shown below.

      5
     /  \
    4    7
   /    / \
  1    6   8
 /
0

 void FindMinMax(int &min,int &max,Node *np,int height)
 {
     if(np==NULL) return;
     FindMinMax(min,max,np->left,height+1);
     FindMinMax(min,max,np->right,height+1);

     if(np->left==NULL || np->right==NULL)
     {
         if(min>height) min = height;
         if(max<height) max = height;
     }
 }

Second, I am not sure whether there are differences in the versions.
In my book the question 4.1 is about checking whether a binary tree is balanced. This is defined as

  heights of the two subtrees of any node never differ by more than one

If this is the requirement I think this algorithm won't work.
Check the first two answers of this question. I think this algorithms checks whether the difference between farthest and closest leaves from root differ by more than one.

Edit: Here I assumed min max corresponds to maximum and minimum depths to leaf nodes.


Dynamically allocated C strings in C++, concatenation, pointers, etc I'm writing a small Win32 console app that manipulates the contents of binary files. It accepts the filename as a command line parameter and writes to a file named filename.scramble

This is the first time I'm dynamically allocating memory for C strings (which I'm using since the fstream functions use const char*).

I've added the comments to help you understand what I'm trying to do. Please tell me whether I'm doing anything unsafe with pointers or whether I'm allocating memory wrong or any unsafe practice you see regarding dynamically allocating and concatenating C strings in C++. My question is specifically about the strings and the memory management.

#include<iostream>
#include<fstream>
using namespace std;

int main(int argc, char *argv[])
{
    //Program needs at least one extra argument
    //If less, print 'usage'
    if (argc < 2)
        cout<<"usage: "<< argv[0] <<" <filename>\n";

    else
    {
        //Assuming argv[1] is a file name
        fstream fin (argv[1],ios::binary|ios::in);

        if (!fin.is_open())
            cout<<"Could not open file\n";
        else
        {
            //Calculate size of file
            fin.seekg(0,ios::end);
            unsigned long long size = fin.tellg();
            //Find out how many left over characters
            //if using blocks of 16 characters
            int lastset=size%16;
            //Change size to number of blocks
            size /= 16;

            //File name must be <original filename>.scramble
            //Allocate space
            char* name = new char[sizeof(argv[1])+8];

            //Concatenate <original filename>
            strcat_s(name,sizeof(argv[1]),argv[1]);
            //Concatenate ".scramble"
            strcat_s(name,sizeof(argv[1])+8,".scramble");

            fstream fout (name,ios::binary|ios::out);
            //Buffer
            char memblock[16];
            fin.seekg(0,ios::beg);

            //Read from fin and write to fout in blocks of 16 characters
            for(unsigned long long i=0;i<size;++i)
            {
                fin.read(memblock,16);
                /*
                *
                * Manipulation on memblock to be done here
                *
                */
                fout.write(memblock,16);
            }

            //No manipulation for last n bytes, n<16
            fin.read(memblock,lastset);
            fout.write(memblock,lastset);

            fin.close();
            fout.close();

            delete[] name;
        }
    }
    return 0;
}

There is no need for manual dynamic memory allocation.

Don't use this:

using namespace std;

It causes lots of problems in anything but a small application. So get into the habbit of not using it. If you muse be lazy specifically import the things you need. Notice in the code below I always use the prefix std:: five letters is not a big overhead for being exact.

using std::cout;
using std::ifstream;

For a good discussion see https://stackoverflow.com/q/1452721/14065 my favorite answer to this question is https://stackoverflow.com/a/1453605/14065

Don't write comments where you explain what the code is doing. The code is already verbose enough to explain what. If you are going to comment explain "WHY" you are doing this. Over commenting is a real problem and you should use comments judiciously and explain why. https://softwareengineering.stackexchange.com/a/13/12917

    //Program needs at least one extra argument
    //If less, print 'usage'

If your condition exits the program. Return an error code immediately. This makes the alignment and reading of the rest of your code easier:

    if (argc < 2)
        cout<<"usage: "<< argv[0] <<" <filename>\n";

    else
    {

Easier to write as:

    if (argc < 2)
    {
        cout<<"usage: "<< argv[0] <<" <filename>\n";
        return 1;
    }
    <rest of code>

Same comment as above.

        if (!fin.is_open())
            cout<<"Could not open file\n";

The best way to comment this. Is to put this code in a function with an appropriate name. Then don't write the comments (the code is self explanatory). You may write a comment about "why" you are using 16 as this will help a maintainer to modify dependant code.

            //Calculate size of file
            fin.seekg(0,ios::end);
            unsigned long long size = fin.tellg();
            //Find out how many left over characters
            //if using blocks of 16 characters
            int lastset=size%16;
            //Change size to number of blocks
            size /= 16;

Easier to read as:

            int size = getNumberOf16ByteBlocksFromStream(fin);

It is very rare that you need to use new/delete. It is even rarer that you would use delete as any object created with new should be put in an object that manages its lifespan correctly. If you don't do this writing exception safe code is exceedingly hard.

In this case you can achieve the same affect by using a std::vector

            //File name must be <original filename>.scramble
            //Allocate space
            char* name = new char[sizeof(argv[1])+8];

Use std::vector for simple buffers. Its exception safe.

            std::vector<char>  name(strlen(argv[1]) + 8);

            // To get the address of the first byte use &name[0];

Note that sizeof() gives you the number of bytes in the variable. This does not give you the length of the string. the size of the variable in this case is the size of the pointer that is pointing at the string (so 4 or 8 depending on the system). Use strlen() to get the size of a string.

Also the +8 is wrong. As .scramble is 9 characters and you need to add a byte for the terminating \0 character. So in reality you should use std::string to build the filename. Again the comments are worse the useless (not because you wrote bad comments, but because the comments can go out of sync with the code. Then a maintainer will wonder why the code does not match the comments. Also this code is so trivial that you should be able to see what it is trying to do).

            //Concatenate <original filename>
            strcat_s(name,sizeof(argv[1]),argv[1]);
            //Concatenate ".scramble"
            strcat_s(name,sizeof(argv[1])+8,".scramble");

            // PS you got the usage all long.
            // I am very surprised if this actually runs correctly.
            // Normal operation.
            //   1) string-copy the first part
            //   2) string-cat all subsequent parts.
            //   3) The size (Second field) in <X>_s() functions is the
            //           TOTAL SIZE OF THE BUFFER
            //      not how much you want to copy. This should ALWAYS
            //      match the value you used in allocation.

Easier to write as:

            std::string filename = argv[1];
            filename += ".scramble"

That's is a very small buffer:

            char memblock[16];

You should probably have re-set the stream up where you were messing around with it finding the size.

            fin.seekg(0,ios::beg);

No need to manually close files. The destructor will do that. https://codereview.stackexchange.com/a/544/507

        fin.close();
        fout.close();

This delete is not exception safe.
Not a problem in this code. But in general you should try and make them exception safe so you don't leak. This means using RAII.

            delete[] name;


Script that helps people make better choices This is a "Help me make a choice" script where the user can input two options and then, based on the (3) reasons why they are good options, and the reasons weight the script will tell you which option you should choose.

This is my first Python script based on an idea I had and I'd like to know how I could make it better. For example, should I store the reasons (ie. option1a) in a list, dictionary or keep them in variables? Should I do the same to the weight (ie. option1aw) of each option?

The code works and does what I wanted it to do, but I don't know if I did in the best way possible.

import random

print("First, tell me what your options are.")

option1 = input("Option 1: ")
option2 = input("Ok. Now tell me the other option: ")

option1a = input("\nGood. Now tell me a reason why \'{}\' is a good choice: ".format(option1))

option1b = input("Ok. Now tell me another reason why \'{}\' is a good choice: ".format(option1))

option1c = input("Ok. Now tell me another reason why \'{}\' is a good choice: ".format(option1))

option2a = input("\nThats all for \'{}\'. Now tell me why \'{}\' is a good choice: ".format(option1, option2))

option2b = input("Now tell me another reason why \'{}\' is a good choice: ".format(option2))

option2c = input("Now tell me another reason why \'{}\' is a good choice: ".format(option2))

option1aw = int(input("\nNow let's evaluate the options. Regarding \'{}\', "
                      "from 1 to 5, tell me how important is \'{}\'? ".format(option1, option1a)))

option1bw = int(input("Still regarding \'{}\', from 1 to 5, tell me how important is \'{}\'? ".format(option1, option1b)))

option1cw = int(input("How about \'{}\', from 1 to 5, tell me how important it is: ".format(option1c)))

option2aw = int(input("\nNow let's evaluate the rest of the options. Regarding \'{}\',"
                      " from 1 to 5, tell me how important is \'{}\'? ".format(option2, option2a)))

option2bw = int(input("Still regarding \'{}\', from 1 to 5, tell me how important is \'{}\'? ".format(option2, option2b)))

option2cw = int(input("How about \'{}\', from 1 to 5, tell me how important it is: ".format(option2c)))

prompt = input("\nWe'll calculate now. Click enter when you are ready to see the results. > ")

option1result = option1aw + option1bw + option1cw

option2result = option2aw + option2bw + option2cw

options = [option1, option2]

def coinflip():
    print("Your best choice is:")
    print(options[random.randint(0, len(options)-1)])

if option1result > option2result:
    print("\n\'{}\' is your best choice based on the reasons you gave me. ".format(option1))

elif option1result == option2result:
    print("\nHonestly, both are good options. Do you want to flip a coin? Press enter. ")
    input()
    coinflip()

else:
    print("\n\'{}\' is your best choice based on the reasons you gave me. ".format(option2))

quit()

I think your program would be easier to use and create if you rearrange when you ask your questions. (This is mostly to show that I have consciously changed how your program works)
You are correct it would be easier to use your data if you enter it as a dictionary and some lists. I personally would use the following layout:

option_1 = {
    'option': option1,
    'reasons': [option1a, option1b, option1c],
    'weights': [option1aw, option1bw, option1cw],
}

This allows getting the relevant by indexing the objects.
For example to get the entered option you can do:

option_1['option']

To get the first reason you can do:

option_1['option'][0]

It should be noted that lists in Python, and most programming languages, are indexed starting at 0, which is why to get the first value we see the 0 above.
Reduce your workload by using functions, these allow you to define a set of instructions to run which you can then reuse by calling the function.

Take the following function to get an option:
Note: I have changed the questions in this code snippet.

def get_option():
    return {
        'option': input('Enter an option: '),
        'reasons': [
            input(f'Option 1: Why is this a good option? '),
            input(f'Option 2: Why is this a good option? '),
            input(f'Option 3: Why is this a good option? '),
        ],
        'weights': [
            int(input(f'How important is Option 1? (from 1-5) ')),
            int(input(f'How important is Option 2? (from 1-5) ')),
            int(input(f'How important is Option 3? (from 1-5) ')),
        ]
    }

Allowing a user to enter two options is now simple. You make a list with both of them.

options = [
    get_option(),
    get_option(),
]

Before we go any further I'd like to show you my favorite feature of Python - list comprehensions. These allow you to perform a task on a list in a single line. Take the above code snippet to generate two options, we can rewrite that using standard list generation methods exposed in Python and other languages, which would look like:

options = []  # Build an empty list
for _ in range(2):  # Loop twice
    options.append(get_option())  # Add an option to options on each loop

However this pattern is rather messy and it would be more Pythonic to use a comprehension here.

options = [
    get_option()
    for _ in range(2)
]

You should be able to notice we can also use this to simplify our get_option code.

def get_option():
    return {
        'option': input('Enter an option: '),
        'reasons': [
            input(f'Reason {i+1}: Why is this a good option? ')
            for i in range(3)
        ],
        'weights': [
            int(input(f'How important is Reason {i+1}? (from 1-5) '))
            for i in range(3)
        ]
    }

From here we can change your option1result and option2result to:

results = [sum(option['weights']) for option in options]

You can use random.choice to chose from a list.
It is best practice to use an if __name__ == '__main__': guard to prevent your code from running unless it's the main program.
Don't use quit, if you remove it the program will exit successfully.

import random

def get_option():
    return {
        'option': input('Enter an option: '),
        'reasons': [
            input(f'Reason {i+1}: Why is this a good option? ')
            for i in range(3)
        ],
        'weights': [
            int(input(f'How important is Reason {i+1}? (from 1-5) '))
            for i in range(3)
        ]
    }

if __name__ == '__main__':
    print('First, tell me what your options are.')
    options = [
        get_option()
        for _ in range(2)
    ]
    prompt = input('\nWe will calculate now.\n')

    results = [sum(option['weights']) for option in options]
    if results[0] == results[1]:
        input('There are multiple best options.\nThe best will be determined by coinflip.\n')
        best_option = random.choice(options)
    else:
        best_option = options[0] if results[0] > results[1] else results[1]

    print('Your best choice is:')
    print(options[0]['option'])

Additional improvements:

Your code only currently works for two options. To get the top options for any amount of options is fairly simple.

You want a dictionary that holds all the options with a certain total weight. After this you just want to take the max weight, which will give you all the options with that weight.

Once you have have the best options the code is pretty much the same, if there are multiple options then you just use random.choice on them to narrow them down to one.
You should allow your user to enter how many options and reasons they want. This now is just a simple question you can ask before entering either loop.
The way you get user input is error prone, if I enter a as my weight then your code blows up.
You should also take into account that you've told your user that only 1-5 are valid entries, but happily allow -1, and 6.
Not displaying all the best options seems like a poor oversight. Since we know all the best options we can just display them by looping.

Ignoring 3 this can get:

import random

def get_option():
    option = input('Enter an option: ')
    reasons = int(input('How many reasons do you have for this option? '))
    return {
        'option': option,
        'reasons': [
            input(f'Reason {i+1}: Why is this a good option? ')
            for i in range(reasons)
        ],
        'weights': [
            int(input(f'How important is Reason {i+1}? (from 1-5) '))
            for i in range(reasons)
        ]
    }

def get_top_options(options):
    rankings = {}
    for option in options:
        rankings.setdefault(sum(option['weights']), []).append(option)
    return rankings[max(rankings)]

if __name__ == '__main__':
    amount_options = int(input('How many options are there? '))
    options = [
        get_option()
        for _ in range(amount_options)
    ]
    prompt = input('\nWe will calculate now.\n')

    best_options = get_top_options(options)
    if len(best_options) == 1:
        print('Your best choice is:')
    else:
        print('Your best choices are:')
    for option in best_options:
        print(option['option'])


Angularjs alertservice and alertscontroller I'm a .net programmer with littler experience in AngularJs and Javascript.

I have implemented an AlertService, AlertsController and Alerts view for alerts/notifications in my system. PaymentService is a service that uses the alert service to notify user of things.

Could you please give me any comments about my code?
Am I following best practises in AngularJs and Javascript?
Should I handle errors in my services?

Here is my code:

alertService.js

'use strict';

app.service('AlertService', function () {

    var handlers = [];

    this.subscribe = function (handler) {
        handlers.push(handler);
    }

    this.success = function (message) {
        pushAlert({ type: 'success', message: message });
    };

    this.info = function (message) {
        pushAlert({ type: 'info', message: message });
    };

    this.warning = function (message) {
        pushAlert({ type: 'warning', message: message });
    };

    this.danger = function (message) {
        pushAlert({ type: 'danger', message: message });
    };

    function pushAlert(alert) {
        handlers.forEach(function (handler) {
            handler(alert);
        });
    }
});

alertsController.js

'use strict';

app.controller('AlertsController', ['$scope', 'AlertService', function ($scope, alertService) {

    $scope.alerts = [];

    alertService.subscribe(function (alert) {
        $scope.alerts.push(alert);
    });

    $scope.closeAlert = function(index) {
         $scope.alerts.splice(index, 1);
    };

}]);

alerts.html

<div ng-controller="AlertsController">
    <uib-alert ng-repeat="alert in alerts" type="{{alert.type}}" close="closeAlert($index)">{{alert.message}}</uib-alert>
</div>

paymentService.js

'use strict';

app.service('PaymentService', ['$http', 'AlertService',
    function ($http, alertService) {
        this.pay = function (id, paymentMethod, numberOfInstallments, creditCard, pin) {
            return $http.post('/api/payment',
                {
                    id: id,
                    paymentMethod: paymentMethod,
                    numberofInstallments: numberOfInstallments,
                    creditCard: creditCard,
                    pin: pin
                })
                .then(success, error);

            function success() {
                alertService.success('Payment confirmed.');
            }

            function error(response) {
                alertService.success(response.data);
            }
        };
    }
]);

So far, your code looks good. But here's some suggestions.

It's usually overkill to pull out success and error callbacks. You can simply inline them. Also, if you happen to have ES6 available via transpiler or if you just don't care about older browsers, by all means use it. It makes the code a lot shorter.

return $http.post('/api/payment', {
  id,
  paymentMethod,
  numberOfInstallments,
  creditCard,
  pin
}).then( _ => {
  alertService.success('Payment confirmed.');
}, _ => {
  alertService.success(response.data);
});

Angular also allows for implicit dependencies. Instead of an array of dependency names and a function at the end of the array, you can simply provide a function whose arguments are the name of the dependencies. Saves you keystrokes.

app.service('PaymentService', ['$http', 'AlertService', function ($http, alertService) {...}]);

app.service('PaymentService', function($http, AlertService){...});


Mini Test testing save on one and many trips I know there is a balance to be struck between writing dry code and damp code but I have the feeling that this is a bit too repeative. Is there a terse yet self explainatory way to rewrite these tests?

class TripTest < ActiveSupport::TestCase
  def setup
    @trip = trips(:one)
    @invoice = invoices(:one)
  end

  test 'trip can save' do
    trip = Trip.new
    trip.departure_date = "2040-03-24"
    trip.depart_airport = "DHW"
    trip.arrive_airport = "ABC"
    trip.invoice_id = 123534

    assert trip.save, "Error message: #{trip.errors.full_messages}"
  end

  test "one invoice can have multiple trips" do
    trip = Trip.new
    trip.departure_date = "2040-03-24"
    trip.depart_airport = "DHW"
    trip.arrive_airport = "ABC"
    trip.invoice_id = 123534
    @invoice.trips.push(trip)
    @invoice.save

    trip = Trip.new
    trip.departure_date = "2040-03-24"
    trip.depart_airport = "ICN"
    trip.arrive_airport = "XYZ"
    trip.invoice_id = 123534
    @invoice.trips.push(trip)
    @invoice.save

    assert @invoice.trips.size > 1
  end

If you're writing verbose tests, your test suite will skyrocket in length.

There are worse problems to have but your best asset to address this problem is a comprehensive test_helper class.

It doesn't matter what you call the file. Just load it up into your main test file with a require_relative.

In your example, it's fairly obvious which code is being repeated between methods. So you could refactor your code as follows:

# in test_helpers.rb
module TestHelpers
  def initialize_sample_trip(options={})
    Trip.new(
      departure_date: options[:departure_date]   || "2040-03-24"
      default_airport: options[:default_airport] || "DHW"
      arrive_airport: options[:arrive_airport]   || "ABC"
      invoice_id: options[:invoice_id]           || 123534
    )
  end
end

...

# in your main test file
require_relative './test_helpers.rb'
class TripTest < ActiveSupport::TestCase
  include TestHelpers
  def setup
    @trip = trips(:one)
    @invoice = invoices(:one)
  end

  test 'trip can save' do
    trip = initialize_sample_trip
    assert trip.save, "Error message: #{trip.errors.full_messages}"
  end

  test "one invoice can have multiple trips" do
    trip1 = initialize_sample_trip
    @invoice.trips.push(trip1)
    @invoice.save
    trip2 = initialize_sample_trip(
      depart_airport: ICN, arrive_airport: "XYZ"
    )
    @invoice.trips.push(trip)
    @invoice.save
    assert @invoice.trips.size > 1
  end

This can be considered a "factory" design pattern. There are also various gems which can assist with this such as factory_girl or faker.


"How can I make this SEDE query better?" (bad title finder) It was suggested on Meta for July 2015 Community Challenge to make a bot that can find questions with bad titles. I wrote this query to help others find examples to help them make write their code.

I made it a point to document as I wrote it so that someone who is a beginner with SQL could still make changes to it easily. Is there room for improvements?

Here is the query on SEDE

DECLARE @Question INT = (SELECT Id FROM PostTypes WHERE Name = 'Question');
DECLARE @Answer INT = (SELECT Id FROM PostTypes WHERE Name = 'Answer');

SELECT
/* Uncomment below if you want to limit the results */
/* TOP 100 */
    Posts.Id AS [Post Link]
  /* only for sanity check in case the filter is removed from WHERE clause */
  , CASE
      WHEN Posts.PostTypeId = @Question THEN 'Question'
      WHEN Posts.PostTypeId = @Answer THEN 'Answer'
      ELSE 'Other' END
      AS [Post Type]
  , Posts.Score
  , Posts.CreationDate
  , Users.Id AS [User Link]
  , Users.Reputation
FROM Posts
INNER JOIN Users
  ON Posts.OwnerUserId = Users.Id
WHERE
  /* Filter out non-question posts */
  Posts.PostTypeId = @Question

  /* Generic title */
  AND (
    Posts.Title LIKE '%please%' OR
    Posts.Title LIKE '%review%' OR
    Posts.Title LIKE '%improve%' OR
    Posts.Title LIKE '%better%' OR
    Posts.Title LIKE '%how to%'OR
    Posts.Title LIKE '%how can i%' OR
    Posts.Title LIKE '%how do i%'
  )

  /* Post not closed yet */
  AND Posts.ClosedDate IS NULL

  /* Question doesn't have positive score */
  /* AND Posts.Score <= 0 */

  /* User has low reputation */
  AND Users.Reputation <= 500

ORDER BY

  /* Lowest scores first - Change to DESC if you want Highest first */
  Posts.Score ASC

  /* Newest first - Change to ASC if you want oldest first */
  , Posts.CreationDate DESC

Your query is well structured, and consistent, but there is one issue you have failed to incorporate.... not all bad questions have a user. When questions are migrated, or there's a user deleted, the question may have no link back to the Users table.

This requires an Outer Join to Users.

Additionally, I have found that CTE expressions are fantastic for aliasing column names. For example, both Posts and Users tables have an ID column, and you need to have Posts.ID and Users.ID in different places. Both also have a CreatedDate. When these aliases become long, and complicated, it bogs down the readability of the query.

Oh, about the readability, the case statement is dead code. Don't use code that serves no purpose, or, alternatively, "debug code" should be removed before deployment. If you cannot trust yourself to have the right conditions on the query, then you cannot trust yourself to have the right debug code either.

So, if you alias these values in a CTE, and use the CTE to make the Outer join neat, you can reduce your query to:

SELECT PostID as [Post Link],
       Score,
       PostDate,
       UserID as [User Link],
       Reputation
FROM BadTitles
LEFT OUTER JOIN Users on UserId = Id
ORDER BY Score ASC, PostDate DESC

OK, that's a cheat, because BadTitles is a CTE, but, the full query, using your expanded comments, is:

DECLARE @Question INT = (SELECT Id FROM PostTypes WHERE Name = 'Question');
DECLARE @Answer INT = (SELECT Id FROM PostTypes WHERE Name = 'Answer');

with BadTitles as (
    SELECT Id as PostId,
           OwnerUserId as UserId,
           Score,
           CreationDate as PostDate
    FROM Posts
    WHERE
      /* Filter out non-question posts */
          Posts.PostTypeId = @Question

      /* Post not closed yet */
      AND Posts.ClosedDate IS NULL

      /* Generic title */
      AND (
            Posts.Title LIKE '%please%'
        OR  Posts.Title LIKE '%review%'
        OR  Posts.Title LIKE '%improve%'
        OR  Posts.Title LIKE '%better%'
        OR  Posts.Title LIKE '%how to%'
        OR  Posts.Title LIKE '%how can i%'
        OR  Posts.Title LIKE '%how do i%'
        )
)
SELECT PostID as [Post Link],
       Score,
       PostDate,
       UserID as [User Link],
       Reputation
FROM BadTitles
LEFT OUTER JOIN Users on UserId = Id
WHERE (Reputation IS NULL OR Reputation <= 500)
ORDER BY

  /* Lowest scores first - Change to DESC if you want Highest first */
  Score ASC,

  /* Newest first - Change to ASC if you want oldest first */
  PostDate DESC

Note that this produces an addition hundred or so questions from migrations, etc.


Spring boot loadbyusername I have the following classes. I wonder if this structure makes sense in any terms especially the usage of the UserDetails class from Spring. Should I use loadbyusername method to create and remove methods in account service or is it okay? Then where should we use loadbyusername method?

Base class to cover fundamental properties like id, createdAt, etc.

@Accessors(chain = true)
@Data
@MappedSuperclass
public class Base {
@Id
private String id = UUID.randomUUID().toString();

@CreatedDate
private Date createdAt;

@LastModifiedDate
private Date updatedAt;
}

Account class

@Entity
@JsonIgnoreProperties(value = {"createdAt", "updatedAt"})
@Data
public class Account extends Base {
@NotNull
private String username;

@NotNull
private String password;

@NotNull
@Email
private String email;

@NotNull
private Role role;

}

Account details class implementing userdetails

public class AccountDetails implements UserDetails {

private Account account;

public AccountDetails(Account account) {
    this.account = account;
}

@Override
public Collection<? extends GrantedAuthority> getAuthorities() {
    return null;
}

@Override
public String getPassword() {
    return account.getPassword();
}

@Override
public String getUsername() {
    return account.getUsername();
}

@Override
public boolean isAccountNonExpired() {
    return true;
}

@Override
public boolean isAccountNonLocked() {
    return true;
}

@Override
public boolean isCredentialsNonExpired() {
    return true;
}

@Override
public boolean isEnabled() {
    return true;
}
}

Account service

@Service
public class AccountService {

@Autowired
AccountRepository accountRepository;

@Autowired
BCryptPasswordEncoder bCryptPasswordEncoder;

@Autowired
ModelMapper modelMapper;

public boolean create(AccountDTO accountDTO) {
    if (usernameExists(accountDTO.getUsername())) {
        throw new UsernameExistsException();
    }

    Account account = modelMapper.map(accountDTO, Account.class);
    account.setPassword(bCryptPasswordEncoder.encode(account.getPassword()));
    account.setId(UUID.randomUUID().toString());
    account.setRole(Role.USER);

    if (accountRepository.save(account) != null) {
        return true;
    } else {
        throw new RuntimeException();
    }
}

public boolean remove(AccountDTO accountDTO) {
    if (!usernameExists(accountDTO.getUsername())) {
        throw new UsernameNotFoundException("Account named " + accountDTO.getUsername() + " not found");
    }

    Optional<Account> account = accountRepository.findByUsername(accountDTO.getUsername());
    accountRepository.delete(account.get());

    if (!usernameExists(accountDTO.getUsername())) {
        return true;
    }

    return false;
}

public boolean usernameExists(String username) {
    return accountRepository.findByUsername(username).isPresent();
}
}

AccountDetailsService implementing userdetailsservice

@Service
public class AccountDetailsService implements UserDetailsService {

@Autowired
AccountRepository accountRepository;

@Override
public UserDetails loadUserByUsername(String s) throws UsernameNotFoundException {
    Optional<Account> optionalAccount = accountRepository.findByUsername(s);

    if (!optionalAccount.isPresent()) {
        throw new UsernameNotFoundException("Account named " + s + " not found");
    }

    return new AccountDetails(optionalAccount.get());
}
}

Some tips:

Tip 1

@Autowired
AccountRepository accountRepository;

@Autowired
BCryptPasswordEncoder bCryptPasswordEncoder;
...

Setter injection is considered as bad practice prefer constructor injection to gain more control and to avoid NullPointerExceptions.
Why field injection is evil

Tip 2

@Id
private String id = UUID.randomUUID().toString();

Encapsulate id generation into some component class eg. IDGenerator.nextId() where IDGenerator is an interface and you would provide some implementation class for that. You'll have more control in testing.

Tip 3

Optional<Account> optionalAccount = accountRepository.findByUsername(s);

if (!optionalAccount.isPresent()) {
    throw new UsernameNotFoundException("Account named " + s + " not found");
}

return new AccountDetails(optionalAccount.get());

Using Optional like that is considered as bad practice. Something like that would be a lot better:

Account account = accountRepository.findByUsername(s).orElseThrow(UsernameNotFoundException::new)
return new AccountDetails(account)

Tip 4

throw new RuntimeException();

You should throw more specific exceptions.

Tip 5 UPDATE

I think that AccountService.remove(AccountDTO accountDTO) is too complex.
I believe in that case would be better to just write own deletion method in repository:

public void remove(AccountDTO accountDTO) {
    Objects.requireNonNull(accountDTO);
    accountRepository.deleteByUsername(accountDTO.getUsername());
}

Tip 6

AccountService and AccountDetailsService could be as one class, these classes have similar responsibilities.

Tip 7

  Then where should we use loadbyusername method?

Spring security needs class which implements this interface when we use some other user data store than in memory.
When some client app try to login / generate token this method
would be invoked by spring and whether if user exists or not spring
would respond with 400 Bad Request or 200 OK.


x86 assembly [fasm] code which takes a number and adds 155 then shifts right The following program takes a user-inputted integer and adds 155, shifts it to the right twice, and prints the output after the addition, then after the shift. It is written in fasm x86 assembly and uses the stdcall convention:

format PE Console
entry main
include 'macro/import32.inc'

; Read-Only data
section '.rdata' data readable
msg db 'Enter a number and we will do some math to it: ', 0
msg2 db 13,10, 'The final result is: %d', 0, 13, 10
p db 'pause>nul', 0
msg3 db 13, 10, '%d + 155 is %d', 0
msg4 db 'You entered: %d', 0
section '.data' data readable writeable
frmspec db '%d'
number dd 0

section '.code' code readable executable

main:
push ebp
mov ebp, esp
; no local vars needed
push msg
call [printf]
sub esp, 4 ; clean stack
push number
push frmspec
call [scanf]
sub esp, 8 ; clean stack
push [number]
push msg4
call [printf]
sub esp, 8 ; clean stack

mov eax, [number]
add eax, 155
push eax
push [number]
push msg3
call [printf]
pop ecx
pop eax
pop ecx

shr eax, 2
mov dword [number], eax
push [number]
push msg2
call [printf]
sub esp, 8
mov esp, ebp
pop ebp
push p
call [system]
sub esp, 4
push 0
call [exit]

;imports
section '.idata' import data readable
library msvcrt, 'msvcrt.dll'
import msvcrt, \
printf, 'printf', \
system, 'system', \
scanf, 'scanf', \
exit, 'exit'

push number
push frmspec
call [scanf]
sub esp, 8 ; clean stack

Cleaning up the stack is done by adding to esp. Because the stack on X86 grows downward, cleaning implies going upwards.
You make this mistake several times!

From your problem statement it's not very clear whether you want to

shift the number after the addition
shift the number as it was before the addition

Your code presently does the latter.

mov eax, [number]
add eax, 155
push eax
push [number]          <------ Original number
push msg3
call [printf]
pop ecx
pop eax                <------ Still original number
pop ecx
shr eax, 2             <------ Shifts the original number
mov dword [number], eax

frmspec db '%d'
number dd 0

Currently your program depends on the zero stored in number. In future you might want to re-use number several times and then the zero-terminator might not be there anymore.
I think it will be more robust if you wrote

frmspec db '%d', 0
number dd 0

msg2 db 13,10, 'The final result is: %d', 0, 13, 10

You've placed the zero before the carriage return/linefeed. Those won't be outputted as a consequence!

mov dword [number], eax

FASM does not require you to write this redundant size tag (dword). It serves no purpose.


HttpURLConnection response code handling This snippet from a downloader callable handles HTTP status codes. I need critique on both the style (for or do-while loop better here?) and functionality. Should I manage the delay differently? Do I need to handle InterruptedException specifically?

HttpURLConnection connection = null;
boolean connected=false;
outer:
for(int retry=0;retry<=RETRIES&&!connected;retry++)
{
 if(retry>0) {log.warning("retry "+retry+"/"+RETRIES);Thread.sleep(RETRY_DELAY_MS);}
 connection = (HttpURLConnection)entries.openConnection();
 connection.connect();
 switch(connection.getResponseCode())
 {
  case HttpURLConnection.HTTP_OK: log.fine(entries +" **OK**");connected=true;break; // fine, go on
  case HttpURLConnection.HTTP_GATEWAY_TIMEOUT: log.warning(entries+" **gateway timeout**");break;// retry
  case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstable
  default: log.severe(entries+" **unknown response code**.");break outer; // abort
 }
}
connection.disconnect();
log.severe("Aborting download of dataset.");

Readability
Let's reformat your code quickly:

    HttpURLConnection connection = null;
    boolean connected = false;
    outer: for (int retry = 0; retry <= RETRIES && !connected; retry++) {
        if (retry > 0) {
            log.warning("retry " + retry + "/" + RETRIES);
            Thread.sleep(RETRY_DELAY_MS);
        }
        connection = (HttpURLConnection) entries.openConnection();
        connection.connect();
        switch (connection.getResponseCode()) {
            case HttpURLConnection.HTTP_OK:
                log.fine(entries + " **OK**");
                connected = true;
                break; // fine, go on
            case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
                log.warning(entries + " **gateway timeout**");
                break;// retry
            case HttpURLConnection.HTTP_UNAVAILABLE:
                System.out.println(entries + "**unavailable**");
                break;// retry, server is unstable
            default:
                log.severe(entries + " **unknown response code**.");
                break outer; // abort
        }
    }
    connection.disconnect();
    log.severe("Aborting download of dataset.");

Fixed:

1-liners are hard to read:
  case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstable

spacing inside for-loop conditions:
  for(int retry=0;retry<=RETRIES&&!connected;retry++)

indentation - 1-space indentation is 'just wrong'™.

Functionality
Right, how does this code work? There are a few problems I can (now) see in here:

On HTTP_OK, it breaks out of the switch, then it exits the loop (because && !connected), and then immediately disconnects and logs a severe error? This can't be right?
HTTP_GATEWAY_TIMEOUT logs a warning.... great, but HTTP_UNAVAILABLE does a System.out.println

The InterruptedException handling is not shown here. There are articles on how to do this properly. Google up on that.
Suggestion
I recommend a sub-function, with a do-while loop (the following is a quick hack-up - untested):
private static final HttpURLConnection getConnection(URL entries) throws InterruptedException{
    int retry = 0;
    boolean delay = false;
    do {
        if (delay) {
            Thread.sleep(RETRY_DELAY_MS);
        }
        HttpURLConnection connection = (HttpURLConnection)entries.openConnection();
        switch (connection.getResponseCode()) {
            case HttpURLConnection.HTTP_OK:
                log.fine(entries + " **OK**");
                return connection; // **EXIT POINT** fine, go on
            case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
                log.warning(entries + " **gateway timeout**");
                break;// retry
            case HttpURLConnection.HTTP_UNAVAILABLE:
                log.warning(entries + "**unavailable**");
                break;// retry, server is unstable
            default:
                log.severe(entries + " **unknown response code**.");
                break; // abort
        }
        // we did not succeed with connection (or we would have returned the connection).
        connection.disconnect();
        // retry
        retry++;
        log.warning("Failed retry " + retry + "/" + RETRIES);
        delay = true;

    } while (retry < RETRIES);

    log.severe("Aborting download of dataset.");

}


SQL query for monthly bills with amounts paid The idea is to get a listing of monthly bills with amounts paid. The subquery does a total through a group by using bill_id and party_id.

I wish to know how it can be improved. I'm not comfortable with JOINs and am willing to learn.

SELECT
    bill.id as bill_id,
    master.id as party_id,
    master.name as party_name,
    `amountExcVat`,
    bill.vat,
    amountExcVat + bill.vat as 'amtIncVat',
    `purchasedDate`,
    (SELECT SUM( cashPaid ) + SUM( chequePaid )
     FROM  `partypayment`
     WHERE party_id = partyId AND bill_id = partyBillId
     GROUP BY partyId, partyBillId)
    as 'Amount Paid(cash+chq)',
    bill.status
FROM `partybill` as bill, `partymaster` as master, partypayment as payment
WHERE
    bill.partyId = master.id
    AND
    bill.partyId = payment.id
    AND
    UPPER(MONTHNAME(`purchasedDate`)) = 'OCTOBER'
    AND
    YEAR(`purchasedDate`) = '2013'

Tables are as follows. They provided by a third party so I can't modify them.

create table partymaster (
id INT(4)
,name varchar(100) PRIMARY KEY
,address varchar(200)
,contactNo1 BIGINT(14)
,contactNo2 BIGINT(14)
);

create table partypurchase (
id  INT(10)
,partyId INT(4) references partymaster(id)
,head varchar(100)
,brand varchar(100)
,size varchar(10)
,costPrice INT(6)
,sellingPrice INT(6)
,quantity INT(4)
,totalCost INT(10)
,purchasedDate date
);

create table partybill (
id INT(10) PRIMARY KEY
,partyId INT(4) references partymaster(id)
,amountExcVat INT(10)
,vat INT(10)
,amountIncVat INT(10)
,purchasedDate date
,expectedPaymentDate date
,status varchar(10)
);

create table partypayment (
id INT(10) PRIMARY KEY
,partyId INT(4) references partymaster(id)
,partyBillId INT(10) references partybill(id)
,cashPaid INT(10)
,chequePaid INT(10)
,chequeBankName varchar(50)
,chequeNumber INT(10)
,chequeDate date
,paymentDate date
);

Do you really need the subquery?

SELECT
bill.id as bill_id,
master.id as party_id,
master.name as party_name,
`amountExcVat`,
bill.vat,
amountExcVat + bill.vat as 'amtIncVat',
`purchasedDate`,
bill.status,
SUM(payment.cashPaid) + SUM(payment.chequePaid) as 'Amount Paid(cash+chq)',
FROM `partybill` as bill, `partymaster` as master, partypayment as payment
WHERE
party_id = partyId AND
bill_id = partyBillId AND
bill.partyId = master.id AND
bill.partyId = payment.id AND
UPPER(MONTHNAME(`purchasedDate`)) = 'OCTOBER' AND
YEAR(`purchasedDate`) = '2013'
GROUP BY partyId, partyBillId)

(I just wrote this without testing it so it might not work right out of the box)

Since you are already querying partypayment you should be able to do this