Identify duplicate characters in a String and replace them Found a JavaScript assignment on Codewars.com:

  The goal of this exercise is to convert a string to a new string where
  each character in the new string is '(' if that character appears only
  once in the original string, or ')' if that character appears more
  than once in the original string. Ignore capitalization when
  determining if a character is a duplicate.

  Examples:

"din" => "((("
"recede" => "()()()"
"Success" => ")())())"
"(( @" => "))(("

I did the assignment in Java, and came up with below code. I want to know is this the write way to code or is there a better way? If so please let me know.

private static String duplicateEncode(String word) {
    String encodeString = "";
    String decodedString = word.toLowerCase();
    for (int i = 0; i < decodedString.length(); i++) {

        if (decodedString.substring(i + 1, decodedString.length())
                .contains(String.valueOf(decodedString.charAt(i)))
                || decodedString.substring(0, i).contains(String.valueOf(decodedString.charAt(i)))) {
            encodeString += ")";
        } else {
            encodeString += "(";
        }

    }
    return encodeString;
}

First of all, nullbyte is exactly right about the use of a StringBuilder: using a += b for Strings roughly equals new StringBuilder().append(a).append(b).toString() under the hood and thus is a quite expensive operation.

Then, your method to check whether a given charcter exists in the string after a given index is s.substring(i).contains(ch) which again creates a superfluos string object via the substring-call. Instead, simply use s.indexOf(ch, i) to find the next index of ch after the given starting index (returns -1 for "not found").

Additionally, you might consider nullbyte's suggestion to hold the seen-or-not-seen status in a set, which also was my first intuition when reading the exercise. (Right now, you have runtime complexity of O(n^2) which you could reduce to O(n)). You only have to somehow come up with a method to revisit the first character, maybe by not using  a set but by usign a map from character to number of occurrences or something like that.

Naming: why decodedString? This is not decoded in any way, this is just lowercase word. I can live with encodeString (or probably better encoded), but decodedString does not look right and does not give the reader any clue about the variable's contents.


Reverse in terms of fold-right and fold-left Exercise 2.39

  Complete the following definitions of reverse
  (exercise 2.18) in terms of fold-right
  and fold-left from exercise 2.38:

(define (reverse sequence)
  (fold-right (lambda (x y) <??>) nil sequence))
(define (reverse sequence)
  (fold-left (lambda (x y) <??>) nil sequence))

I wrote this answer:

(define (fold-right op initial seq)
  (define (rec rest)
    (if (null? rest)
        initial
        (op (car rest)
            (rec (cdr rest)))))
  (rec seq))

(define (fold-left op initial seq)
  (define (iter result rest)
    (if (null? rest)
        result
        (iter (op result (car rest)) (cdr rest))))
  (iter initial seq))

(define (reverse sequence)
  (fold-right (lambda (x y)
                (append  y (list x))) null sequence))

(define (reverse-l sequence)
  (fold-left (lambda (x y) (cons y x)) null sequence))

(define a (list 1 2 3 4 5))

Your implementation of reverse using fold-left is correct.

In the case of reverse using fold-right, you may use snoc (described below) in place of append.  It looks better and its complexity is no worse than that of append, so there is no loss in efficiency:

(define (snoc e lis)
  (if (null? lis)
      (cons e lis)
      (cons (car lis) (snoc e (cdr lis)))))

(define (reverse sequence)
  (fold-right (lambda (x y)
                (snoc x y)) null sequence))

... or even more succinct:

(define (reverse sequence)
  (fold-right snoc null sequence))

It is curious that SICP's definition of fold-left swaps arguments to op (but retains the correct ordering of arguments to op in fold-right), resulting in the use of a rather tedious (lambda (x y) (cons y x)) instead of a simple cons.  SRFI-1 gets the definitions of fold and fold-right correct, of course.


Handling selections for a vending machine To summarise, the code is based of a vending machine and allows the user to choose an item once chosen, it will add 1 to the item to display how many total items the user has.

However, it is incredibly long and more or less it essentially repeats the same code again which is inefficient. How can I compress this code into smaller sections?

if selection == 1:
    item = "Chocolate"
    print ("1 for Diary Milk 2 for Maltesars 3 for Kit Kat:")
    selection = int(input("Please enter your option:"))
    if selection == 1:
        item = "Diary Milk"
        itemValue = itemValue + 1
    elif selection == 2:
        item = "Maltesars"
        itemValue = itemValue + 1

    elif selection == 3:
        item = "Kit kat"
        itemValue = itemValue + 1

elif selection ==2:
    item = "Gum"
    print ("1 for Airwaves 2 for Extra Pepermint 3 for Wriggley's Summer fruit:")
    selection = int(input("Please enter your option:"))
    if selection == 1:
        item = "Airwaves"
        itemValue = itemValue+1
    elif selection == 2:
        item = "Extra Pepermint"
        itemValue = itemValue+1

    elif selection == 3:
        item = "Wriggley's Summer fruit"

elif selection ==3:
    item = "Fizzy Drink"
    print ("1 for Coke 2 for Tango Cherry 3 for Dr Pepper:")
    selection = int(input("Please enter your option:"))
    if selection == 1:
        item = "Coke"
        itemValue = itemValue+1
    elif selection == 2:
        item = "Tango Cherry"
        itemValue = itemValue+1
    elif selection == 3:
        item = "Dr Pepper"
        itemValue = itemValue+1

The biggest thing you can do is to use string formatting to remove redundancy. Now instead of having to repeatedly type "1 for Coke 2 for Tango Cherry 3 for Dr Pepper:, you factor out the common parts, put the foods in a list, and use that list to populate the string. I also removed your first assignment to item, (ie item = "Gum") for each, because they immediately get removed. Finally, all three circumstances can be combined by indexing into a list of lists.

format_string = "1 for {0} 2 for {1} 3 for {2}:"

items = [["Diary Milk", "Maltesars", "Kit kat"],
         ["Airwaves","Extra Pepermint","Wriggley's Summer fruit"],
         ["Coke", "Tango Cherry", "Dr Pepper"]]

print(format_string.format(*items[selection - 1]))
new_selection = int(input("Please enter your option:"))
item = items[selection - 1][new_selection - 1]
itemValue += 1

Taken together, this reduces your code from 50 lines, to 10.


Safe DbContext Disposal I have a service class called ClientService, the service class is called using an interface IClientService. The service class simply does CRUD methods for the DbSet<Client> in my context class.  In my service class constructor I have:

private MyContext context;
public ClientService()
{
    context = new MyContext();
}

I then have a void in the ClientService class that can be called from the IClientService interface called Save() that does this:

public void Save()
{
    context.SaveChanges();
    context.Dispose();
    context = new MyContext();
}

Is this safe or even necessary or am I good simply saying context.SaveChanges(); in the Save() void?

You shouldn't need to call Dispose and new up another context in the Save method. SaveChanges should be good enough.  I would recommend you make ClientService IDisposable and when that class disposes then dispose of the context as well.

Update from comment

to implement IDisposable on the IClientService

public interface IClientService : IDisposable

The will now require a Dispose method on any class that implements IClientService.  In the dispose method you should call the dispose method of the context.


Change View visibility programmatically or through XML? I have an XML layout used by 2 activities. Within this layout, there's a view that is visible for Activity1 and gone for Activity2. Activity1 is used more times compared to Activity2. I would like to know which of the following methods is a better practice, if there's a better one.

Method 1: The view is gone and it's turned visible with every instance of the Activity1.

XML:

<View android:id="@+id/myView" android:visibility="gone" />

Java:

class Activity1 extends Activity {
    ((View) findViewById(R.id.myView)).setVisibility(0);
}

class Activity2 extends Activity {
}

Method 2: The view is visible and it's turned gone only when Activity2 is being used.

XML:

<View android:id="@+id/myView" android:visibility="visible" />

Java:

class Activity1 extends Activity {
}

class Activity2 extends Activity {
    ((View) findViewById(R.id.myView)).setVisibility(8);
}

So, is it better to change the visibility programmatically multiple times or fewer times?

According to View source code setting visibility through xml (line 3450) is cheaper than through setVisibility method (line 5511).

So it is better to change visibility programmatically fewer times.


Program with random number generation I am having trouble with the random number generator. For week 0 only, I want to set the starting number of roaches in House A (totalRoachesInHouseA) to 97 and the number of roaches in House B (totalRoachesInHouseB) to 79. Then, for weeks 1 to 10, I want to generate random numbers for both houses. I am trying to get the same numbers in the example simulation below.

Initial Population: Week 0: House 1 = 97; House 2 = 79
Week 1: House 1 = 119; House 2 = 109
Week 2: House 1 = 151; House 2 = 144
Week 3: House 1 = 194; House 2 = 189
Week 4: House 1 = 25; House 2 = 247
Week 5: House 1 = 118; House 2 = 235
Week 6: House 1 = 198; House 2 = 260
Week 7: House 1 = 280; House 2 = 314
Week 8: House 1 = 37; House 2 = 395
Week 9: House 1 = 187; House 2 = 374
Week 10: House 1 = 315; House 2 = 414

Here are the instructions for the project:

  Write a program which keeps track of the number of roaches in two
  adjacent houses for a number of weeks. The count of the roaches in the
  houses will be determined by the following:

  The initial count of roaches for each house is a random number between 10 and 100.
  Each week, the number of roaches increases by 30%.
  The two houses share a wall, through which the roaches may migrate from one to the other. In a given week, if one house has more roaches
  than the other, roaches from the house with the higher population
  migrate to the house with the lower population. Specifically, 30% of
  the difference (rounded down) in population migrates.
  Every four weeks, one of the houses is visited by an exterminator, resulting in a 90% reduction (rounded down) in the number of roaches
  in that house. Your implementation must use functions and local
  variables.

Here's what I have so far:

int roachesInHouseA, roachesInHouseB; //my two houses.
int temporaryHouseA, temporaryHouseB;
double totalRoachesInHouseA, totalRoachesInHouseB; //the total count of houses.
int week;

int main( )
{
    int temporaryHouseA = 0; // storage for arithmetic
    int temporaryHouseB = 0; // storage for arithmetic

    int totalRoachesInHouseA = 97; // test number for A
    int totalRoachesInHouseB = 79; // test number for B

    roachesInHouseA = rand() % 100 + 10;
    roachesInHouseB = rand() % 100 + 10;

    //The functions declaring the random count of roaches in both houses between
    //10 and 100.
    for (int Z = 1; Z < 12; Z++) // My for loop iterating up to 11 weeks.
    {
        totalRoachesInHouseA = totalRoachesInHouseA + roachesInHouseA * .3;
        totalRoachesInHouseB = totalRoachesInHouseB + roachesInHouseB * .3;

        // My function declaring that the roach population explodes by 30% weekly.
        cout << "The number of roaches in House A is " << totalRoachesInHouseA << endl;
        cout << "The number of roaches in House B is " << totalRoachesInHouseB << endl;

        if ((week == 4) || (week == 8)) // It's extermination time!
        {
            totalRoachesInHouseA = totalRoachesInHouseA - roachesInHouseA * .9;
            totalRoachesInHouseB = totalRoachesInHouseB - roachesInHouseB * .9;
        }
        else if (totalRoachesInHouseA > totalRoachesInHouseB) // Migration from
        //House A to HouseB
        {
            temporaryHouseA = totalRoachesInHouseA * .3;
            totalRoachesInHouseA = totalRoachesInHouseA - temporaryHouseA;
            totalRoachesInHouseB = totalRoachesInHouseB + temporaryHouseA;
        }
        else if (totalRoachesInHouseA < totalRoachesInHouseB) // Migration from
        // House B to House A
        {
            temporaryHouseB = totalRoachesInHouseB * .3;
            totalRoachesInHouseB = totalRoachesInHouseB - temporaryHouseB;
            totalRoachesInHouseA = totalRoachesInHouseA + temporaryHouseB;
        }
    }
}

First of all, the requirements are specific in requiring that you "use functions". I'm pretty sure the intent of that was the exact opposite of how you've structured the code, with everything in main.

You might want to consider some (more or less) top-down style development, where you write a simple main that has practically nothing except calls to lower-level functions. I'd do this by thinking of a short (word or two) summary describing each step in the specification, and use those as function names.

int main(void) {
    house_t houses[2];

    // 1. put initial population in each house:
    populate(houses[0]);
    populate(houses[1]);

    for (week = 1; week < N; week++) {
        // 2. each week, each house's population increases
        increase_pop(houses[0]);
        increase_pop(houses[1]);

        // 3. each week, a percentage of the population may migrate between the houses
        do_migration(houses[0], houses[1]);

        // 4. every four weeks, an exterminator visits one of the houses
        if (week % 4 == 0)
            exterminate(random_choice(houses, 2));

        // 5. Presumably we want to show the data for each house weekly.
        show(houses[0]);
        show(houses[1]);
    }
}

A few things about that code may be open to question -- for example, the spec doesn't seem to specify how to choose which house is exterminated at any given time. I've used a random_choice, but perhaps you think strict alternation makes more sense. In any case, however, The important point is that what we see here should be about as close as possible to a direct translation of the steps outlined in the requirement into the syntax of the target language. From there, we can define the sub-parts in relative isolation from each other.

Right now, this makes life much easier for your teacher: it's fairly easy to look through main and figure out whether it fits the requirements. He can then glance through each of the functions and see whether that function does what it's supposed to as well.

I'd also note that the same basic idea applies when school is over and you're writing real code. The big difference here is that you will spend a lot of time doing what the teacher is doing now, reading and analyzing code, not just writing it. In school, you tend to write code and (almost) never look at it again. In real life, you live with the code after you've written it.

I suspect that you have the idea of writing the code and getting it working, then breaking it up into functions. I'd advise against that. It makes the job considerably more difficult.

I'd also note how this changes the situation if (for example) you need/want to change the parameters. For example, assume you have 5 (or 50) houses in a row, and wanted to run the situation for a year instead of 10 weeks. With the code as I've written it above, that would mostly mean rewriting most of the code that explicitly refers to houses[0] or houses[1] to instead use loops and refer to houses[i]. Don't try to go overboard with this idea though. If you can make the code more flexible like this without adding extra complexity, great. At the same time, don't spend (much) extra time or work writing against possible changes in requirements that may never happen.


Bash script to send emails when web server does not respond I've made a simple bash script to check if a web server responds, and to send emails to a list of addresses if the website is down. Any suggestions as to how to improve it/ edge cases that I missed/ better styling/ etc? I'm not very familiar with idiomatic bash.

#!/bin/sh

output=$(wget http://lon3213:8111 2>&1)
pattern="connected"
path="/remote/users/my_user/projects/cmb_pinger/"
# File where I check and write the status of the server
# so I don't end up sending more e-mails for the same
# downtime. It writes 0 if no email has been sent yet
# and 1 if the email has been already sent.
log="check"
emails="email1@example.com,email2@example.com"

# Create file if it doesn't exist
if ! [[ -f "$path$log" ]]
then
    echo "0" > "$path$log"
fi

# Website cannot be reached
if [[ ! "$output" =~ "$pattern" ]]
then
    # Emails have not yet been sent
    if [[ $(cat "$path$log") == "0" ]]; then
        echo "$output" | mail -s "Dashboard is down" $emails
        echo "1" >| "$path$log"
    fi
else
    echo "0" >| "$path$log"
fi

I am impressed with the consistency of your code. You have obviously put effort in to making sure it is well presented, etc. It makes it easy to read.

The first thing I notice is the she-bang line, you have #!/bin/sh this should be #!/bin/bash... right? The script uses Bash specific features that don't exist in classic /bin/sh, such as [[. Although it may appear to work with #!/bin/sh in your system, chances are that your /bin/sh is symlinked to /bin/bash (typically in Linux). This is not something you can rely on (it will break spectacularly in Solaris, for example), so when using Bash-specific features, then you should use #!/bin/bash in the she-bang.

Then, there are some shortcuts you are missing though, and these are standard processes available on most Unix commands.... wget has an exit code! Use it.

wget saves the web content to a file in the local directory. It will not override files, so you may end up with many copies of the same content. I recommend redirecting the URL contents to the stdout by using -O - argument for wget.

Error handling is a critical part of any script. It's not always easy in bash, but it can be done.

You have used the redirection >| in a few places. This caught me by surprise. I admit I looked it up. In my experience the noclobber setting is seldom set, but that may just be for me. That's just an observation. Your site may be set up differently.

Finally, bash has function support, which allows you simplify logic substantially

Consider the following:

#!/bin/bash

url="http://lon3213:8111"
path="/tmp/cmb_pinger/"

# File where I check and write the status of the server
# so I don't end up sending more e-mails for the same
# downtime. It writes 0 if no email has been sent yet
# and 1 if the email has been already sent.

log="check"
emails="email1@example.com,email2@example.com"

error() {
    local message="$@"
    >&2 echo $message
    exit 1
}

setFlag() {
    local flag=$1
    echo $flag >| "$path$log" || error "Unable to set flag to $flag"
}

# Create file if it doesn't exist
[[ -d "$path" ]] || mkdir -p "$path" || error "Unable to create directory $path"
[[ -f "$path$log" ]] || setFlag 0

# Website cannot be reached
output=$(wget -O - $url 2>&1)
success=$?

if [[ $success -eq 0 ]]
then
    setFlag $success
else
    # Emails have not yet been sent
    if [[ $(cat "$path$log") == "0" ]]; then
        echo $output | mail -s "Dashboard is down" $emails || error "Unable to send mail to $emails"
        setFlag $success
    fi
fi


An alternative to loop constructs in Java Java allows methods to make recursive calls.  This can be dangerous because an infinite recursive call will fill up the stack. It can be useful, however, for calculations that need iteration when the number of "self calls" is limited (it save a few keystrokes as well). The code below, although not very useful, does demonstrate the technique.  What are some more practical uses of this technique and how could this code be modified for those uses?

class NoForLoop {
        int sayHello(int num) {
            System.out.println("Hello World!");
            return (1 > num) ? 1 : sayHello(num -1); //self call
        }
    public static void main (String [] args) {
        NoForLoop n = new NoForLoop();
        n.sayHello(5);
    }
}

In Java 8 you can use streams, they tend to make the code quite more compact:

IntStream.range(0, 5).forEach(i -> System.out.println("Hello World!"));

I would recommend reading the documentation of IntStream and Streams in general


Algorithm to determine if a string is all unique characters This my solution to one of the practice problems from Cracking the Coding Interview: 150 Programming Interview Questions and Solutions [Book]

  implement an algorithm to determine of a string has all unique characters. What if you cannot use additional data structures ?

public class PracticeProblems {

    public void questionOne(String input) {
        /*-- implement an algorithm to determine if a string has all unique characters.
         * What if you cannot use additional data structures?   --*/

        boolean[] chars = new boolean[26];
        int x = 0;

        for(int i = 0; i < input.length(); i++) {

            if(!chars[(int)input.toUpperCase().charAt(i) - 64]) {
                chars[(int)input.toUpperCase().charAt(i) - 64] = true;
            }
            else {
                System.out.println("not unique");
                x = -1;
                break;
            }
        }

        if(x == 0)
            System.out.println("unique");
    }

    public static void main(String[] args) {
        PracticeProblems test = new PracticeProblems();
        test.questionOne("dsfdddft");
    }
}

I was wondering if there was a better solution to this, or if there is a better way of handling the last part, where I am initializing an x variable to be able to determine if I all the characters are not unique, not to print "it is unique". If don't have the condition for the x value it always prints "it is unique".

You probably shouldn't call toUpperCase() twice on each character.

If you wanted to split your code properly (if it was for a real-life project for instance), it would make sense to make the documentation a bit better and to define a method taking a String as an argument and returning a boolean. Let's keep things simple for the time being; you can return immediately after printing "not unique". If you do so, there's no need for the test on x and there's no need for x at all.

You probably should check that the characters are in the right range before accessing chars.

I'd rather read if (c) { A } else { B } than if (!c) { B } else { A } even though it depends from one situation to another. In our case, it also allows to remove a level of nesting because of the return.

You don't need an instance of PracticeProblems at all, and the function could just be static.

Finally, I do not know if Java optimises out the different call to length() so we might ensure we don't call it every time.

public class PracticeProblems {
    public static void questionOne(String input) {
        boolean[] chars = new boolean[26];
        String upper = input.toUpperCase();

        for(int i = 0, n = upper.length(); i < n; i++)
        {
            char c = upper.charAt(i);
            if ('A' <= c && c <= 'Z')
            {
                if(chars[(int)c - 'A'])
                {
                    System.out.println("not unique");
                    return;
                }
                chars[(int)c - 'A'] = true;
            }
        }
        System.out.println("unique");
    }

    public static void main(String[] args) {
        questionOne("dsfdddft");
    }
}


Sorting a list and another list inside each item I have a list of factories. Each factory has an item of its own and a list of other items from competitors.

I need to sort the list of factories based on price of their items and also sort list of other items from competitors for each factory.

What I am doing require to sort collection of factories and loop through all factories and sort collection of their competitors. I am wondering if there is any easier way to do it.

Factory

class Factory implements Comparator<Factory>{

String item;
double price;
List<Competitor> competitors;
@Override
    public int compare(Factory o1, Factory o2) {
        if (o1.getPrice() < o2.getPrice())
            return -1;
        if (o1.getPrice() > o2.getPrice())
            return 1;
        return 0;
    }}

Competitor

class Competitor implements Comparator<Competitor>{
String facName;
double price;
@Override
public int compare(Competitor o1, Competitor o2) {
        if (o1.getPrice() < o2.getPrice())
            return -1;
        if (o1.getPrice() > o2.getPrice())
            return 1;
        return 0;
}

Sort

Collection.sort(factoriesList); //sort list of factories
//sort list of competitors of each factory
for(int i=0;i<factoriesList.size();i++){
   Collection.sort(factoriesList.getCometitors());
}

Sample result

Factory1 Item1 222  Com1 444 Com2 555
Factory2 Item2 223  Com3 555 Com4 676
Factory3 Item3 224  Com1 543
Factory4 Item4 225  Com9 322

There are others concerns with your code, without going into the sort:

Getter returning mutable data

getCompetitors() returns directly the internal list stored by your factory object. This is generally not a good idea: it means a client of Factory can modify its internal structure, which defeats the OOP principle. It would be preferable instead to have a method sortCompetitors(), that would sort the list, without leaking it:

private List<Competitor> competitors;

public void sortCompetitors() {
    Collections.sort(competitors);
}

and remove completely the method getCompetitors().

Comparator, Comparable

class Factory implements Comparator<Factory>

will be problematic in the future. It seems what you want would be to use Comparable instead, but even this isn't a good idea in this case. There is a difference between the two: a class is Comparable when it can compare itself to another class of the same type, which is what you are doing here: one Factory is comparing itself to another object. On the other hand, a Comparator is a class that is comparing 2 objects of the same type (it does not compare this with another object).

Therefore:

It is wrong to have your class implement Comparator: it means the class is doing too much: it knows itself and it is also responsible for comparing any 2 factories (See also on Stack Overflow). More often than not, there are several ways to compare 2 objects given a context: sort by name or sort by price...
It is also probably wrong to have your class implements Comparable, but for a different reason. As said before, Comparable is used to compare this with another object of the same type. But this interface is closely related to equals and hashCode: any class that is Comparable should provide a consistent equals method (It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y))). In your case, it would imply that two factories are equal when their price is equal, which is probably not what you want.

The solution here is not to make your class implements Comparator and define a custom comparator class, like

class FactoryPriceComparator implements Comparator<Factory>

Using built-in methods

Your compare methods are currently doing:

@Override
public int compare(Factory o1, Factory o2) {
    if (o1.getPrice() < o2.getPrice())
        return -1;
    if (o1.getPrice() > o2.getPrice())
        return 1;
    return 0;
}

This can be written more concisely with the built-in Double.compare (since Java 7), which also properly handles NaN, -0.0 and 0.0, contrary to your current code:

public int compare(Factory o1, Factory o2) {
    return Double.compare(o1.getPrice(), o2.getPrice());
}

Your whole comparator would then become:

public class FactoryPriceComparator implements Comparator<Factory> {
    @Override
    public int compare(Factory o1, Factory o2) {
        return Double.compare(o1.getPrice(), o2.getPrice());
    }
}

Note that you would have the same implementation for the Comparator<Competitor>.

Java 8 API

If you're using Java 8, you can even get rid of the above FactoryPriceComparator and use the built-in Comparator.comparingDouble(keyExtractor), which creates a comparator comparing the double values returned by the key extractor. In this case, the key extractor could be the method reference Factory::getPrice (resp. Competitor::getPrice). So you could simply have:

Collections.sort(factoriesList, Comparator.comparingDouble(Factory::getPrice));

or even

factoriesList.sort(Comparator.comparingDouble(Factory::getPrice));

Finally:

  What I am doing require to sort collection of factories and loop through all factories and sort collection of their competitors

This is actually the proper way of doing it: when you sort a Factory, you cannot sort the inner competitors at the same time, because different objects are being compared.


Users management php class After a little bit of troubleshoots, I've finished this simple users registration and login class. It uses PDO to connect to a mysql database, I've tried also to manage the session when an user do the login or logout. The main objective of this code form me was to make an exercise for using correctly the PDO and related functions, and to start improving my coding skills to manage account creations and login using PHP OOP.

<?php
reqiure_once 'config.php';
class User{

private $db = null;
private $email;
private $username;
private $password;
private $uid;
private $sessioncode;

public function __construct(\PDO $db){

    $this->db = $db;
}

public function createUser($email,$username,$password){

    if($this->checkEmail($email)){
        echo 'Email address already exsist.';
    } elseif($this->checkUsername($username)){
        echo 'Username already exsist.';
    } else {
        $this->password = password_hash($password,PASSWORD_BCRYPT);
        $stmt = $this->db->prepare('INSERT INTO users (email,username,password) VALUES (?, ?, ? )');
        if($stmt->execute(array($email,$username,$this->password))){
        echo 'Account successful created';
        }
    }
}

public function loginUser($username,$password){

    $stmt = $this->db->prepare('SELECT id,username,password FROM users WHERE username = ?');
        $stmt->execute(array($username));
            if($stmt->rowCount() > 0){
                $result = $stmt->fetch(PDO::FETCH_OBJ);
                if(password_verify($password, $result->password)){
                $this->setSession($result->username,$result->id);

            } else {
                echo 'wrong password';
                }
            } else {
                echo 'Username or email does not exist in the database.';
            }
}

public function logoutUser($uid){

    $this->unsetSession($uid);
}

private function checkEmail($email){

    $stmt = $this->db->prepare('SELECT email FROM users WHERE email = ?');
        $stmt->execute(array($email));
        if($stmt->rowCount() > 0){
        return true;
        } else {
        return false;
        }
}

private function checkUsername($username){

    $stmt = $this->db->prepare('SELECT username FROM users WHERE username = ?');
        $stmt->execute(array($username));
        if($stmt->rowCount() > 0){
        return true;
        } else {
        return false;
        }
}

private function setSession($username,$uid){

    $this->sessioncode = bin2hex(rand());
    $_SESSION['id'] = $uid;
    $_SESSION['username'] = $username;
    $_SESSION['session_code'] = $this->sessioncode;
        $stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
        if($stmt->execute(array($this->sessioncode,$uid))){
        header('Location: u/dashboard.php');
    }
}

private function unsetSession($uid){

    $this->uid = $uid;
    $this->sessioncode = '';
    $stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
        if($stmt->execute(array($this->sessioncode,$this->uid))){
        session_unset();
        session_destroy();
        header('Location: ../index.php');
    }
}

}
?>

Whole user management system is very hard to do properly even with structural code. When you get a grasp of entire logic & security nuances OOP itself takes it to the next level, because this logic will be hard to encapsulate in objects of separate concerns - session, cookies, http request and response, domain models, view models, factories, routing... potential abstraction leaks are everywhere. That said, let's take a look at your class...

From OOP perspective

Output:

This class is not concerned about output - you don't know its format (json/html) nor channel/destination (http/console/log file) - you will validate passed values against your data model, but nothing more. For example when registering new user, the response should be the same user data (withut password - not needed anymore) with user id or class specific validation errors attached like (bool) usernameConflict, (bool) emailConflict - that's it. The rest is up to clients of this class (they might not want to reveal users' emails in this indirect way)

Database:

You should separate yourself from database - it's not relevant to your application where data comes from. Use port & adapter technique - interface to query methods (taking only required parameters) and its PDO implementation.

Session:

Session at this point is a global dependency and hidden structural dependency (there needs to be session_start() somewhere). Remove it from this class and return what it needs instead. It should be something that provides user id & not only that. There will be other parties interested in authentication result, so return user data without password hash and other "secrets".

Handling hashes:

This is the only concern of your class: get credentials for concrete user (defined by username, email, cookie token id) and its hash signature (password hash, persistent cookie token, ...), compare result & return data useful to others - beyond this class hashes should be no use to anyone. You won't have methods covering entire login and logout procedure - just a part of it.

and side-effects...

There will be also some side-effect logic in this class like possible password rehashing on algorithm's change (after succesful match), removing cookie tokens (id+hash) on mismatch, hashing token (id+token is generated outside and passed as response cookie)... etc.

Some security concerns:

After succesful login session id should be regenerated (see: session
fixation attack)
After failed login/registration - don't inform that same email is in database (mentioned in "output" section). Registering user with existing email might be handled by sending some (merge/restore) options in activation email.
Session code doesn't protect you from anything - it
all happens inside application environment (only session id goes outside). Holding user's id for authentication (in case user is deactivated/removed) is enough
Make sure that httpOnly and Secure flags are set for session-type cookies (https might be hard to setup for localhost env, but it's neccessary on remote)
All forms should be signed/marked with CSRF tokens - new session id = generate new token is most convenient approach (every form with new token doensn't add much to protection, but causes problems with multiple open tabs, back button...)
More...


Reorder columns in Array I am reordering columns in an array and removing one column. The mapping is as follows:

| Item   | In index | Out Index |
|--------|----------|-----------|
| Item1  | 1        | 6         |
| Item2  | 2        | 4         |
| Item3  | 3        | 5         |
| Item4  | 4        | 7         |
| Item5  | 5        | 8         |
| Item6  | 6        | 9         |
| Item7  | 7        | 10        |
| Item8  | 8        | #N/A      |
| Item9  | 9        | 1         |
| Item10 | 10       | 2         |
| Item11 | 11       | 3         |

So, with the following data in range A1:K2 of the active sheet:

| Category 1 | RX9 | East Midlands | 4588 | 14:47:36 | 00:08:25 | 00:14:52 |   | 01/10/17 | 09/02/18 | England |
|------------|-----|---------------|------|----------|----------|----------|---|----------|----------|---------|
| Category 1 | RX9 | East Midlands | 4588 | 14:47:36 | 00:08:25 | 00:14:52 |   | 01/10/17 | 09/02/18 | England |

I get, after running the code:

| 01/10/17 | 09/02/18 06:09 | England | RX9 | East Midlands | Category 1 | 4588 | 0.616388889 | 0.005844907 | 0.010324074 |
|----------|----------------|---------|-----|---------------|------------|------|-------------|-------------|-------------|
| 01/10/17 | 09/02/18 06:09 | England | RX9 | East Midlands | Category 1 | 4588 | 0.616388889 | 0.005844907 | 0.010324074 |

Notes:

I don't expect the array number of rows to ever exceed 12,240.
The time columns have become doubles in the output. Sorted with code to format the relevant columns - not pertinent to question.

Is there a more efficient way to do this without making arrays left, right and centre?

Option Explicit
Public Const OutputColumnsTotal As Long = 10

Private Sub test()

    Dim tempArr() As Variant

    With ActiveSheet

        tempArr = .Range("A1:K2").Value
        tempArr = ShuffleArrayColumns(tempArr)

        .Range("A5").Resize(UBound(tempArr, 1), UBound(tempArr, 2)) = tempArr

    End With

End Sub

Private Function ShuffleArrayColumns(ByRef tempArr As Variant) As Variant

    If Not UBound(tempArr, 2) - 1 = OutputColumnsTotal Then

        Debug.Print "Array tempArr as wrong # columns in " & Application.VBE.Activecodepane.CodeModule
        Exit Function

    Else

        Dim i As Long
        Dim tempArr2() As Variant
        ReDim tempArr2(1 To UBound(tempArr, 1), 1 To OutputColumnsTotal)

        For i = LBound(tempArr, 1) To UBound(tempArr, 1)

            tempArr2(i, 1) = Format$(tempArr(i, 9),"yyyy-mm-dd") 'to preserve UK date format. Sheet is formatted to display "mmm-yy".
            tempArr2(i, 2) = tempArr(i, 10)
            tempArr2(i, 3) = tempArr(i, 11)
            tempArr2(i, 4) = tempArr(i, 2)
            tempArr2(i, 5) = tempArr(i, 3)
            tempArr2(i, 6) = tempArr(i, 1)
            tempArr2(i, 7) = tempArr(i, 4)
            tempArr2(i, 8) = tempArr(i, 5)
            tempArr2(i, 9) = tempArr(i, 6)
            tempArr2(i, 10) = tempArr(i, 7)

        Next i

    End If

    ShuffleArrayColumns = tempArr2

End Function

Another approach is to work with individual columns:

Option Explicit

Public Sub ShuffleColumns()

    Const ROW_OFFSET As Long = 3

    Dim fr As Long, fc As Long, lr As Long, frx As Long, lrx As Long
    Dim res As Variant, arr As Variant, i As Long

    With Sheet1

        fr = .UsedRange.Row
        fc = .UsedRange.Column
        lr = .Cells(fr, fc).End(xlDown).Row

        frx = lr + ROW_OFFSET   'Next first row
        lrx = (frx - fr) + lr   'Next last row

        res = Array(9, 10, 11, 2, 3, 1, 4, 5, 6, 7) 'IN columns mapped to OUT columns

        fc = fc - 1 'Optimized for the loop

        For i = LBound(res) To UBound(res)

            arr = .Range(.Cells(fr, res(i) + fc), .Cells(lr, res(i) + fc))  'IN column

           .Range(.Cells(frx, i + 1 + fc), .Cells(lrx, i + 1 + fc)) = arr   'OUT column

        Next

       .Range(.Cells(frx, fc + 1), .Cells(lrx, fc + 1)).NumberFormat = "yyyy-mm-dd"

    End With
End Sub

.

Note: Public Const OutputColumnsTotal As Long = 10 is global but it is used only in ShuffleArrayColumns() - it's advisable to declare constants and vars as close to their scope as possible

.

Test results with OP code:

Test results with this code:

Performance: for 100,000 rows - 2.167 sec (OP code) vs 1.277 sec (this code)

Edit

Testing with different optimizations for the FOR loop:

Version 2

Public Sub ShuffleColumnsUnoptimizedFC()
    Const ROW_OFFSET As Long = 3

    Dim fr As Long, fc As Long, lr As Long, frx As Long, lrx As Long
    Dim res As Variant, arr As Variant, i As Long, t As Double
    With Sheet1
        t = Timer
        fr = .UsedRange.Row
        fc = .UsedRange.Column
        lr = .Cells(fr, fc).End(xlDown).Row

        frx = lr + ROW_OFFSET
        lrx = (frx - fr) + lr

        res = Array(9, 10, 11, 2, 3, 1, 4, 5, 6, 7)
        For i = LBound(res) To UBound(res)
            arr = .Range(.Cells(fr, res(i) + fc - 1), .Cells(lr, res(i) + fc - 1))
           .Range(.Cells(frx, i + 1 + fc - 1), .Cells(lrx, i + 1 + fc - 1)) = arr
        Next
       .Range(.Cells(frx, fc), .Cells(lrx, fc)).NumberFormat = "yyyy-mm-dd"
        Debug.Print "Rows: " & lr & "; Time: " & Format("0.000", Timer - t) & " sec"
    End With
End Sub

Version 3

Public Sub ShuffleColumnsUnoptimizedLoop()
    Const ROW_OFFSET As Long = 3

    Dim fr As Long, fc As Long, lr As Long, frx As Long, lrx As Long
    Dim res As Variant, arr As Variant, i As Long, t As Double
    With Sheet1
        t = Timer
        fr = .UsedRange.Row
        fc = .UsedRange.Column
        lr = .Cells(fr, fc).End(xlDown).Row

        frx = lr + ROW_OFFSET
        lrx = (frx - fr) + lr

        res = Array(9, 10, 11, 2, 3, 1, 4, 5, 6, 7)
        For i = LBound(res) To UBound(res)
            arr = .Range(.Cells(fr, res(i) + fc - 1), .Cells(lr, res(i) + fc - 1))
           .Range(.Cells(frx, i + 1 + fc - 1), .Cells(lrx, i + 1 + fc - 1)) = arr
            If i = 1 Then
                .Range(.Cells(frx, fc), .Cells(lrx, fc)).NumberFormat = "yyyy-mm-dd"
            End If
        Next
        Debug.Print "Rows: " & lr & "; Time: " & Format("0.000", Timer - t) & " sec"
    End With
End Sub

After 3 tests with 500,000 rows

Rows: 500,000; Time: 7.01171875 sec (v1)
Rows: 500,000; Time: 7.05078125 sec (v2)
Rows: 500,000; Time: 7.08984375 sec (v3)

The idea is to move all unnecessary operations outside the loop - repetition will amplify any minor effort - exponentially if the loops are nested.


Interpreting the time EDIT: Alternate answer on overflow: https://stackoverflow.com/questions/38487692/how-can-i-interpret-the-time-with-less-code/38492144#38492144

But I really want to keep Jonah's answer here too. Just let me know if I really have to close either this one or stackoverflow's question.

I really don't like sticking many ifs together. I think it looks like a child stacking. But if anyone knows how to do the same thing I'm doing with this code but with less lines, it would be great.

var time = data[i].data4;
                    var summary = '' + time + ' minutes since last login.
';
                    if(time >= 60) {
                        var hour= tiempo / 60;
                        summary = '' + Math.floor(hora) + ' hours since last login.
';
                        if(hour>= 24) {
                            var day = hour/ 24;
                            summary= '' + Math.floor(day) + ' days since last login.
';
                            if(dia >= 7) {
                                var week= day / 7;
                                summary = '' + Math.floor(week) + ' weeks since last login.
';
                                if(week >= 4) {
                                    var month = week / 4;
                                    summary = '' + Math.floor(mes) + ' months since last login.
';
                                }
                            }
                        }
                        s += summary;
                    } else {
                        s += summary;
                    }

For some context: data[i].data4 is from a column that is an operation of DATEDIFF(mi, someDate, getdate()).

As you can see, the returned results would be in minutes and from then on I start checking how many hours, days, and so on from that and I would even love to add between each line an (if it's greater than 1) so I can change the wording to singular instead of plural but I think that's too many lines just for a decision of whether to simply write 'minute' instead of 'minutes'.

Here's a rewrite without any ifs:

function humanTime(minutes) {
  var units          = ['months' , 'weeks' , 'days' , 'hours'];
  var minutesPerUnit = [40320    , 10080   , 1440   , 60];
  var timeInUnits    = minutesPerUnit.map(x => Math.floor(minutes / x));
  var time           = timeInUnits.find(x => x > 0);
  var unit           = units[timeInUnits.indexOf(time)];
  return time + ' ' + unit;
}

Note I removed the <td>s from this code as it's mainly a view (formatting) concern.  This allows this function to remain more focused: it simply takes a number of minutes and converts it to hours/days/weeks/months as appropriate.

You might also consider returning an object of the form: {time: 6, unit: 'days'} instead of a string as I have, to make the separation of data and view even stronger.


Function autoloading in bash I'm wondering if anyone can code review my code below.  That's the core code of my little framework for bash in GitHub.

## ==========================================================================================
## FUNCTION : autoload_functions
## PURPOSE  : To check functions that are required, try to autoload them if NOT YET defined
##
## INPUT: array of $req_func
## OUTPUT: exit 1 - if any required function cannot be autoloaded (meaning, not found in standard
##          library location.
## ==========================================================================================
autoload_functions()
{
    local req_func=$*
    local ret_val=0

    ## check first parameter. If it's a directory, then
    ## it is where functions should be loaded from
    local new_libdir=$1
    [[ -d $new_libdir ]] && {
      shift
      LIB_DIR=$new_libdir
     }

    req_func=$* ## redeclare

    ## iterate through all $req_func, set $ret_val to 1
    ## if any required function cannot be autoloaded
    for each_func in $req_func; do
        func_name=`basename $each_func`
        ## if function undefined, try to auto-include (reference from specified location)
        type $func_name &> /dev/null || source $LIB_DIR/${each_func}.bash.inc

        ## recheck, ensure it has been defined.
        type $func_name &> /dev/null || (echo "ERROR: Missing required ${each_func}"; ret_val=1)
    done

    ## abort (exit 1) if any required func cannot be autoloaded
    [ $ret_val -eq 1 ] && exit $ret_val
} ## END: autoload_functions()

Bug because of subshell

You are losing the value of ret_val in a subshell here:

... || (echo "ERROR: Missing required ${each_func}"; ret_val=1)

Variable assignments in a subshell are local to the subshell, so the variable in your function won't change. Instead of a subshell, you just want to group these commands using { ...; } like this:

... || { echo "ERROR: Missing required ${each_func}"; ret_val=1; }

Unnecessary variable

You don't need the req_func variable. The top of your function could have been written simpler like this:

autoload_functions()
{
    local ret_val=0

    ## check first parameter. If it's a directory, then
    ## it is where functions should be loaded from
    local new_libdir=$1
    [[ -d $new_libdir ]] && {
      shift
      LIB_DIR=$new_libdir
    }

    ## iterate through all arguments, set $ret_val to 1
    ## if any required function cannot be autoloaded
    for each_func; do
        # ...

Prefer $(...)

The `...` form for command substitution is deprecated, use $(...) instead:

func_name=$(basename $each_func)

Quoting variables with paths

This line will not work if LIB_DIR contains spaces:

type $func_name &> /dev/null || source $LIB_DIR/${each_func}.bash.inc

This would be safer:

type $func_name &> /dev/null || source "$LIB_DIR"/${each_func}.bash.inc

Though this is probably a bit paranoid.

The same goes for $func_name and ${each_func} too,
but that would seem even more paranoid.
(Who in his right mind would create bash scripts with spaces in the name?)

Naming

The names you chose for some variables are not great. I'd recommend these alternatives:

Instead of each_func: func_path or func_relpath (as in relative path from LIB_DIR)
Instead of ret_val: exit_code, because you may exit with this


FizzBuzz in SQLite 2 This query performs a FizzBuzz in SQLite 2.8.17 without creating any tables or sprocs.

First attempt:

select case when fb <> '' then fb else n end from (
    select g + f * 2 + e * 4 + d * 8
        + c * 16 + b * 32 + a * 64 as n,
    coalesce(fizz, '') || coalesce(buzz, '') as fb
    from
    (select 0 union select 1 as a),
    (select 0 union select 1 as b),
    (select 0 union select 1 as c),
    (select 0 union select 1 as d),
    (select 0 union select 1 as e),
    (select 0 union select 1 as f),
    (select 0 union select 1 as g)
    left join (select 'Fizz' as fizz, 3 as fizzstep)
        on n % fizzstep = 0
    left join (select 'Buzz' as buzz, 5 as buzzstep)
        on n % buzzstep = 0
    where n between 1 and 100
);

Second attempt:

select case
    when n % 3 = 0 and n % 5 = 0 then 'FizzBuzz'
    when n % 3 = 0 then 'Fizz'
    when n % 5 = 0 then 'Buzz'
    else n
end from (
    select g + f * 2 + e * 4 + d * 8 +
        c * 16 + b * 32 + a * 64 as n from
    (select 0 union select 1 as a),
    (select 0 union select 1 as b),
    (select 0 union select 1 as c),
    (select 0 union select 1 as d),
    (select 0 union select 1 as e),
    (select 0 union select 1 as f),
    (select 0 union select 1 as g)
    where n between 1 and 100
);

Can it be improved, either by:

making it shorter / less redundant, or
making it significantly faster, or
making it more portable so it also works in another RDBMS (including SQLite 3)?

Note that CTEs are not available until later versions of SQLite. Also note that this won't work as written on sqlfiddle.com; they're using SQLite 3.

I will have to admit I personally prefer the First attempt, but both seem like they would run fast and deliver as expected.

The only piece I don't like so much, which is found in both attempts, is this:

    select g + f * 2 + e * 4 + d * 8
    + c * 16 + b * 32 + a * 64 as n

I think this could really use some parentheses for clarity, if nothing else.

    select (
    g + (f * 2) + (e * 4) + (d * 8)
    + (c * 16) + (b * 32) + (a * 64)
    ) as n


Should my Json storage handle exceptions? I made a class to conviniently serialize/deserialize data. Now I'm stuck with the following question. Should I handle exceptions (use try-catch) inside JsonStorage or inside ConfigManager and other classes that might use JsonStorage in the future?

public class JsonStorage : IDataStorage
{
    private readonly IFileSystem fileSystem;
    private readonly ILogger logger;

    public JsonStorage(IFileSystem fileSystem, ILogger logger)
    {
        this.fileSystem = fileSystem;
        this.logger = logger;
    }

    public T RestoreObject<T>(string path)
    {
        T result = default;

        try
        {
            if (!fileSystem.Path.IsPathFullyQualified(path))
                throw new ArgumentException("Invalid path.", nameof(path));

            string json = fileSystem.File.ReadAllText($"{path}.json");
            result = JsonConvert.DeserializeObject<T>(json);
        }
        catch (Exception ex)
        {
            logger.Log($"Object can't be restored. {ex.Message}");
        }

        return result;
    }

    public void StoreObject(object obj, string path)
    {
        string file = $"{path}.json";

        try
        {
            if (!fileSystem.Path.IsPathFullyQualified(path))
                throw new ArgumentException("Invalid path.", nameof(path));

            fileSystem.Directory.CreateDirectory(fileSystem.Path.GetDirectoryName(file));
            var json = JsonConvert.SerializeObject(obj, Formatting.Indented);
            fileSystem.File.WriteAllText(file, json);
        }
        catch (Exception ex)
        {
            logger.Log($"Object can't be stored. {ex.Message}");
        }
    }
}

public static class ConfigManager
{
    private static readonly string configPath = @"SomePath\Config";

    public static BotConfig LoadConfig()
    {
        return Unity.Resolve<IDataStorage>().RestoreObject<BotConfig>(configPath);
    }

    public static void SaveConfig(BotConfig config)
    {
        Unity.Resolve<IDataStorage>().StoreObject(config, configPath);
    }
}

ConfigManager - example class that holds a path to Config.json and loads/saves it (using JsonStorage internaly).

For now I'm catching exceptions inside JsonStorage and using a logger instance to print exception messages to the console.
I don't know if there's a downside to that. Should JsonStorage have its own logger? Should it throw an exception (just like ReadAllText/WriteAllText) when something goes wrong with path/files?
Or maybe there is a good practice and some better ways to handle exceptions (in my particular case)?

This is how I see things...

Throwing ArgumentException insinde a try/catch that at the same time swollows it is pointless. You use ArgumentException to inform the user that he called your API in a wrong way. This is not something one should hangle because he won't see it until he looks at the logs. The app should crash in this case.
The message of the ArgumentException is too generic. When it happens, the user already knows something is wrong. Tell him how to fix that by giving him a hint what the correct path should be and what path he actually used.
The way you are handling exceptions that might be thrown by the json-serializer is unexpected (non-standard) because the user will wonder why he got a null without noticing anything. public library APIs should always throw unless they are called TryDoSomething and return a bool.
Yet another flaw of this pattern is that it doesn't help anyone to find the source of the exception. You neither add the filename to it nor the object that it tried to de/serialize. This is very important piece of information and should be included.

So, what you can/should do instead is to throw a new exception that adds more information about what happened e.g.:

public T RestoreObject<T>(string path)
{
    if (!fileSystem.Path.IsPathFullyQualified(path))
            throw new ArgumentException($"Invalid path: '{path}'. It must by fully qualified.", nameof(path));

    try
    {
        string json = fileSystem.File.ReadAllText($"{path}.json");
        return JsonConvert.DeserializeObject<T>(json);
    }
    catch (Exception ex)
    {
        throw new JsonStorageException($"Could not restore object '{typeof(T).Name}' from '{path}'.", ex);
    }
}

You'll be happy to see the type and file name when something goes wrong.


Simple but (hopefully) safe C nodes for data structures Nodes will be for simply linked lists, circular lists, maybe later on extending them to work with graphs. I'm trying to be as defensive as possible, will this leak memory? It runs perfectly I'm just concerned about coding style and best practices. Especially regarding the allocation I'm not 100% convinced of my solution. Is it okay to allocate memory for the next component inside the constructor?

// I would have preferred to declare the typedef together with the struct,
// but this doesn't work.
typedef struct node Node;

struct node {
    int value;
    Node *next;
};

Node *create_node(int value) {

    Node *result = malloc(sizeof(Node));
    // How necessary are these checks? Under which circumstances
    // could this return NULL?
    if (result == NULL) {
        return NULL;
    }

    result->next = malloc(sizeof(Node));
    if (result->next == NULL) {
        free(result);
        return NULL;
    }

    result->value = value;

    return result;
}

void destroy_node(Node *node) {
    if (node != NULL) {
        //This seems necessary. Or is it not? Could I delete the parent
        // node and keep the children alive?
        destroy_node(node->next);
        free(node);
    }
}

int main() {

    Node *first = create_node(42);
    printf("%d\n", first->value);

    Node *second = create_node(45);
    first->next = second;
    printf("%d\n", first->next->value);

    destroy_node(first);

    return 0;
}

It's very misleading that destroy_node (singular) actually recursively frees all nodes in the chain.

It's very weird that create_node allocates two nodes rather than one.  In fact, you do have a memory leak in your main():

first->next = second;

There, you have just overwritten the 42 node's next pointer without having freed it.

To answer this question…

// How necessary are these checks? Under which circumstances
// could this return NULL?
if (result == NULL) {
    return NULL;
}

If malloc() fails (because no more memory is available), it will return NULL.  Yes, it is good practice to check whether malloc() failed.  (In practice, if you are so low on memory that you can't even allocate a single linked-list node, it's very difficult to recover gracefully.  Your program is going to die, and there's not much you can do about it.)


Flask web application, with handling for user login status I am trying to change some variable in base html-template which other html pages inherite from after entering user in personal account.

The idea is as follows:

there is base html template and there depends on user status (logged in or logged out) are buttons Registration and Enter/Exit.
if user is logged in then I want Enter button and Registration button to be dissappeared and instead of Enter button I want Exit button to be there
in case if user is not logged in then two buttons should be: Enter and Registration

I managed to accomplish this task using flask, jinja2 and python but my backend code looks unbearably ugly because there is too much of duplication

Here is my python code:

this function executes before user gets to any html page and determines its status

from flask import g
from flask import render_template
@app.url_value_preprocessor
def send_user_login_status(endpoint, values):
    if current_user.is_authenticated: # if logged in
        g.login_button_class = u'logout'
        g.login_button = u'Exit'

        g.signin_button = u''
        g.signin_button_class = u'signout'
    else: # if not logged in
        g.login_button_class = u'login'
        g.login_button = u'Enter'

        g.signin_button = u'Registration'
        g.signin_button_class = u'signin'

    return render_template("base.html",
                           login_button=g.login_button,
                           login_button_class=g.login_button_class,
                           signin_button=g.signin_button,
                           signin_button_class=g.signin_button_class,
                           )

And now I must every time pass variables to html page in order this page to be with needed buttons.

For example home page:

@app.route('/')
def home():
    return render_template("index.html",
                           login_button=g.login_button,
                           login_button_class=g.login_button_class,
                           signin_button=g.signin_button,
                           signin_button_class=g.signin_button_class,
                           )

or about us page:

@app.route('/about')
def about():
    # send_user_login_status()
    return render_template("about.html",
                           login_button=g.login_button,
                           login_button_class=g.login_button_class,
                           signin_button=g.signin_button,
                           signin_button_class=g.signin_button_class,
                         )

This is my python logout function:

@app.route('/logout', methods=['POST'])
def logout():
    login_status = logout_user()
    if login_status:
        g.login_button_class = u'login'
        g.login_button = u'Enter'

        g.signin_button = u'Registration'
        g.signin_button_class = u'signin'

        jsonify({'result': True})
        return redirect('/')

And as you can see there is too much of duplication because I need to pass this depends-on-login-status variables to each html page.

Whereas html code looks nice:

base.html

<!DOCTYPE html>

 <meta charset="UTF-8"/>

 {{ login_button }}

{{ signin_button }}

                {% block content %}
                {% endblock %}

index.html

{% extends "base.html" %}
{% block content %}

Home

{% endblock %}

about.html

{% extends "base.html" %}
{% block content %}

About

{% endblock %}

Despite html code indeed looks nice, is there where you want to point out your efforts

Jinja supplies conditional blocks (like in python) where you can define which buttons you want to appear on screen, depending if user is logged in or not

This is a bare example

<div>
        {%+ if something %}yay{% endif %}
</div>

Following your example, your condition for user authentication can go in the template directly

{% if current_user.is_authenticated %}
  Hi {{ current_user.name }}!
{% endif %}

Feel free to read a bit more about options on templates on the following link for Jinja documentation

http://jinja.pocoo.org/docs/2.10/templates/

Enjoy the coding :)


array with reusable elements I need array to store elements, but I don't want to make memory allocations at runtime. So I reuse elements. Some fields are never changes so as I reuse elements I need to assign those fields just once.

This is what I wrote, only sceleton, need to add "errors test" etc, but enough to demonstrate the idea:

public sealed class ResizeableArray<T> where T : class, new()
{

    // made public to use in "foreach", but better to be private
    public T[] array;
    public int Count { get; private set; }

    public ResizeableArray(uint maxLength = 128)
    {
        if (maxLength <= 0) throw new ArgumentOutOfRangeException("maxLength");
        array = new T[maxLength];
        for (int i = 0; i < maxLength; i++)
        {
            array[i] = new T();
        }
        Count = 0;
    }

    public T this[int key]
    {
        get { return array[key]; }
    }

    public void Clear()
    {
        Count = 0;
    }

    public T Add()
    {
        return array[Count++];
    }

    public void RemoveRange(int index, int count)
    {
        var newSize = Count - count;
        for (int i = index; i < newSize; i++)
        {
            array[i] = array[i + count];
        }
        Count = newSize;
    }

    public void RemoveAt(int index)
    {
        throw new NotImplementedException();
    }

}

Usage is simple, for example

var element = array.Add();
// configure, assign only "mutable" fields
element.field1 = value1;
element.field2 = value2;

What do you think? Is it good class or you can suggest something better?

upd posting final version I use in production. I think this class can be usefull if you constantly need to reconfigure some array. It's probably will be faster no to allocate new object over and over (but need to measure). Also it's probably less error prone to have always the same instance of the object.

public sealed class ResizeableArray<T> where T : class, new()
{
    private T[] array;
    public int Count { get; private set; }

    public ResizeableArray(uint maxLength = 128)
    {
        if (maxLength <= 0) throw new ArgumentOutOfRangeException("maxLength");
        array = new T[maxLength];
        for (int i = 0; i < maxLength; i++)
        {
            array[i] = new T();
        }
        MaxLength = maxLength;
        Count = 0;
    }

    public uint MaxLength { get; private set; }

    public T this[int key]
    {
        get { return array[key]; }
    }

    public void Clear()
    {
        Count = 0;
    }

    public T Add()
    {
        return array[Count++];
    }

    // sloooow. don't use it
    public T InsertAt0()
    {
        var zeroOrder = array[Count];
        for (int i = Count; i > 0; i--)
        {
            array[i] = array[i - 1];
        }
        array[0] = zeroOrder;
        return zeroOrder;
    }

}

my question is about idea in general

It's flawed, in several ways.

Premature Optimization

The code smell I get is that the whole point is avoiding instantiating and then disposing Array elements.  Reasons we stay away from optimizing up front

We end up spending too much time on it
Corrupting our business design for the sake of something that we don't even know is an issue.
We don't know yet where and how much actual performance is affected.
We cannot know if our up front optimization is in fact better than what we would have done; it could be worse!

Resizing?

I thought this is what we're trying to avoid.

Where is the "resizing"? I assume it will be in Add() and Remove(). If there is no resizing - the size is fixed at instantiation, then the class name is wrong.

The conventional wisdom says resizing Array is less performant than resizing a List. In fact the C#/.NET team went to great lengths to make sure List automatic resizing performs well. Do you have the time and other resources to make your home-spun Array resizing worth not using what the .NET framework already gives you?

Wrong Perspective

I see the client having to write his code in terms of Arrays and array elements when it should be in terms of your business objects - TradeOrder, for example.

The structure - the array - should not be the emphasis in your design. If you keep going down this road you are in for ugly maintenance problems over time.

Give the business classes "collection friendly" capabilities

I imagine sorting, searching, preventing duplicates, uniqueness, etc. might be important qualities when we make a collection of things. So TradeOrder should implement IEquatable and IComparable.

ResizeableArray, or any other TradeOrder user's code, should not be doing this:

if (trade1.stockName == trade2.stockName && trade1.shares == trade2.shares && ...)

Client code should be able to do this:

if (trade1.Equals(trade2))

Should ResizableArray inherit Array?

Array already implements foreach, for example. You'll get all the built-in goodness and it will be exposed at the 'class level' to the client. There's lots of nifty search and sort methods that take advantage of ICompareable implementation.

  Some fields are never changes so as I reuse elements I need to assign those fields just once

Memory Leaks

As ResizableArray gets used it will have empty and occupied elements scattered throughout the Array. You will end up writing code to scan the array for every Add(). Otherwise you'll be adding new elements when there are empty elements available.

It looks like we leave TradeOrder objects in unused array elements. This is the case at instantiation, clearly. Also removing things involves Count but not nulling the reference there. I guarantee you'll be spending lots of extra code and lots of debugging time trying to keep the Count in synch with the actual active objects.

We don't know what an empty element is

The Count is being incremented/decremented but we're leaving objects in place. I'm assuming that at some point we're done with a given object, in which case it should be disposed of. Besides the memory issue, how do we know what array elements are in use and which ones we can over-write?

  // made public to use in "foreach", but better to be private
      public T[] array;

NO.

ResizeableArray should implement IEnumerable.

You are forcing client code to be written like this:

foreach (var trade in tradeOrders.array)

When they should write:

foreach (var trade in tradeOrders)

This is a violation of OO principles. Single Responsibility, Law of Demeter (least knowledge), Encapsulation ... a discussion for another time.

As a practical matter do not force the client to have to know how to manipulate the class properties. ResizableArray should know how to iterate itself.

The client should not tell the ResizeableArray object to resize, or how.


clone of HTML tables of data Need help to refactor this code to look like its done by a pro. Or just put some advice or links to useful stuff.
The idea is to clone a div with two empty tables for two separate arrays of data.
div is cloned depending on the longest array length between the two. Then append row of data to them, if rows reach max number the loop continues on another cloned table. loop continues even if the one row is finished appending

        //content1
        //7 row max

        //content2
        //21 row max

Javascript part
let d1 = @json($data1); //get data
let d2 = @json($data2); //get data

let ar1 = [];
let ar2 = [];
let s = 0;

for (let i = 7; i < d1.length; i+=7) {
    ar1.push(d1.slice(i, i+7));           //slice data 1
}

for (let i = 21; i < d2.length; i+=21) {
    ar2.push(d2.slice(i, i+21));          //slice data 2
}

ar1.length > ar2.length ? s = ar1.length : s = ar2.length; //take longest array length

for (let v = 0; v < s-1; v++) { //clone div using longest array length
     $("#d-page-0").clone().prop("id", "d-page-"+(v+1)).insertAfter("#d-page-"+v); //clone div, edit id
     $("#d-page-"+(v+1)).find("#e-tbl-0", "table").prop("id", "e-tbl-"+(v+1)); //edit 1st tbl id
     $("#d-page-"+(v+1)).find("#w-tbl-0", "table").prop("id", "w-tbl-"+(v+1)); //edit 2nd tbl id
}

//append 1st data to 1st table
 for (let x = 0; x < ar1.length; x++) {
      let k = ar1[x];
      for (let y = 0; y < k.length; y++) {
           $('#e-tbl-'+x).append(
                `

                      ar1[x][y].content

`
            );
      }
}

//append 2nd data to 2nd table
 for (let x = 0; x < ar2.length; x++) {
      let k = ar2[x];
      for (let y = 0; y < k.length; y++) {
           $('#w-tbl-'+x).append(
                `

                      ar1[x][y].content

`
            );
      }
}

working sample
https://jsfiddle.net/eeneg/pgk3dhac/57/

I've refactored your code below to add some of the repetitive items into functions and tried to improve your variable names a little.
The new variable names and function names should clearly indicate their intended purpose/data.
A rule of thumb worth remembering is the concept of DRY (Do not Repeat Yourself).

const createArraywithJsonData = function(count, data) {
    let array = [];

    for (let i = count; i < data.length; i += count) {
        array.push(data.slice(i, i + count));
    }

    return array;
}

const editTableId = function(selector, counter) {
    $(`#d-page-${(counter+1)}`).find(`#${selector}-0`, "table").prop("id", `${selector}-${(counter+1)}`);
}

const appendDataToTable = function(array, property, selector) {
    for (let x = 0; x < array.length; x++) {
        let k = array[x];

        for (let y = 0; y < k.length; y++) {
            $(`#${selector}-${x}`).append(`<tr><td>${array[x][y][property]}</td></tr>`);
        }
    }
}

let d1 = [
    {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }, {
        a: "1"
    }
];
let d2 = [
    {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }, {
        b: "2"
    }
];

let ar1 = createArraywithJsonData(7, d1);
let ar2 = createArraywithJsonData(21, d2);
let longestArrayLength = ar1.length > ar2.length ? ar1.length : ar2.length;

for (let v = 0; v < longestArrayLength - 1; v++) { //clone div using longest array length
    $("#d-page-0").clone().prop("id", `d-page-${(v + 1)}`).insertAfter(`#d-page-${v}`); //clone div, edit id
    editTableId('e-tbl', v);
    editTableId('w-tbl', v);
}

appendDataToTable(ar1, 'a', 'e-tbl');
appendDataToTable(ar2, 'b', 'w-tbl');

<!DOCTYPE html>

    <title></title>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>

tbl1

tbl2


Sieve of Eratosthenes Primes Efficiency I'm fairly new to Java and was trying to make a program that's generates the primes from 2 - 100. I originally had a design of my own making but it was fairly inefficient so I did some research and come across the Sieve of Eratosthenes, which is supposedly the the efficient way to find primes from 2 - n.

public class SieveofEratosthenes {
    public static int[] primes(int num) {
        int primeCounter = 0;
        int[] numPrimes;
        // Boolean array made 1 element longer because SOE starts at 2
        // i.e. index 2 will correspond to 2, instead of index 0 corresponding to 2 (0, 1 will be unused)
        boolean[] primes = new boolean[num + 1];
        // Fill primes array with true
        for (int i = 2; i < primes.length; i++) {
            primes[i] = true;
        }

        /*
         * Calculate primes up to num using Sieve of Eratosthenes algorithm
         */
        for (int i = 2; i <= Math.sqrt(num); i++) {
            if (primes[i]) {
                for (int k = i * i; k <= num; k += i) {
                    if (primes[k]) {
                        primes[k] = false;
                        primeCounter++;
                    }
                }
            }
        }

        /*
         *  Return array
         */
        numPrimes = new int[(num - 1)- primeCounter];  // Set size equal to number of primes in primes
        primeCounter = 0; // Reuse variable
        for (int i = 2; i < primes.length; i++) {
            if (primes[i]) {
                numPrimes[primeCounter] = i;
                primeCounter++;
            }
        }
        return numPrimes;
    }// End primes()
}// End class

I don't think I have a complete grasp on the concept of the Sieve of Eratosthenes algorithm, but I'm pretty sure I've got most of the key points.

How efficient is my code? Are there any particularly popular ways of implementing the Sieve of Eratosthenes into Java?

Your code is mostly right. primeCounter is a misleadingly named variable, since it counts non-primes. And, similarly, primes isn't an array of primes, but rather an array of flags indicating prime-ness. It would be better to name this one isPrime.

There is an Arrays.fill method that you may want to use for clarity:

Arrays.fill(isPrime, true);

And it may be easier to simply add the primes as you go though, and return an ArrayList<int>:

/*
 * Calculate primes up to num using Sieve of Eratosthenes algorithm
 */
ArrayList<int> primes = new ArrayList<int>();
for (int i = 2; i <= num; i++) {
    if (isPrime[i]) {
        primes.add(i);
        for (int k = i * i; k <= num; k += i) {
            isPrime[k] = false;
        }
    }
}
return primes;

The other advantage of ArrayList is that you don't have to have a separate index to keep track of where in your array you're inserting into - let the container do its job!


Registering or logging in when creating a new comment object This service object allows users without a session to create a comment, and register OR login at the same time.

If a user is signed out he must enter an email and a password along with the comment.

The service checks if a user exists using the email, then attempts to authenticate the user. Otherwise, it creates a new user.

Three code smells:

User errors are added to the comment. There is no user object in the view, just a params hash with that name.
If user validation fails, comment validation does not trigger, meaning if the comment was also blank they need another round trip to find out. Probably not significant though.
User data is not repopulated on validation failure.

Question: I'm proud of writing a nifty object like that, but it stills feels a little 'wrong-ish' if there is such a word. Is there a better way to handle this type of scenario?

class CommentService
  def self.by(params, user)
    if user
      new params, user
    else
      new params, find_or_new(params)
    end
  end

  def self.find_or_new(params)
    User.find_by(email: params[:user][:email]) || User.new
  end

  attr_reader :params, :comment, :user

  def initialize(params, user)
    @params, @user = params, user
    @comment = Comment.new(comment_params)
  end

  def save
    if user.new_record?
      self.user.attributes = user_params
      user.save ? persist : add_user_errors
    else
      if params[:user].present?
        user.authenticate(params[:user][:password]) ? persist : add_auth_error
      else
        persist
      end
    end
  end

  def post
    @post ||= Post.find(params[:post_id])
  end

  private

  def add_user_errors
    user.errors.each do |k,v|
      comment.errors.add(k, v)
    end
    false
  end

  def add_auth_error
    comment.errors[:base] << "Invalid email or password."
    false
  end

  def user_params
    params.require(:user).permit(:name, :email, :password)
  end

  def comment_params
    params.require(:comment).permit(:text)
  end

  def persist
    comment.author = user
    comment.post = post
    comment.save
  end
end

Controller code:

  def create
    service  = CommentService.by(params, current_user)
    @post = service.post
    if service.save
      redirect_to @post
    else
      @comment = service.comment
      render 'new'
    end
  end

View code:

= form_for [@post, @comment] do |f|
  = f.text_area :comment
  / No user object here. Just tossing a user hash into the params.
  - unless signed_in?
    = fields_for :user do |uf|
      = uf.text_field :email
      = uf.password_field :password

  = f.submit

My opinions:

The class methods by and others are unnecessary.
The service object should be initialized by feeding objects instead of params.
Build a guest user is not job of CommentService.

Then code like these:

# Controller
def create
  user     = current_user || build_guest_user
  @comment = user.new(params[:comment])
  service  = CommentService.new(@comment, user)
  if service.save
     if user.guest?
       redirect_to 'guest_user_sign_in_or_register'
     else
       redirect :back, notice: "Comment created"
     end
  else
     # blah blah
  end
end

# ApplicationController
def build_guest_user
  # your logic here
end

# CommentService
def initialize(comment, user)
  @comment = comment
  @user    = user
end

def save
  if @comment.save
    # extra service logic here
  else
    false
  end
end

Add

Mohamad, think about this, when a visitor hit a "Post Comment" button, he will reach create.

If he's logged in, that's fine.

If he not logged in, he will be treated as a guest user, no matter registered or not. -- Then treated by build_guest_user method.

Then, after comment saved, the controller can judge if he is a guest user or not. If not, nothing to do. If he is, an Ajax template will be rendered back asking his signing in or register.

If he signed in, the comment under the guest will be moved to his user account. And this guest user will be deleted, guest session be replaced by user session.

If he register, same as above.

If he refuse to either sign in or register, the guest user and his comment will be kept there.


Implementation of merge sort in Python I was working on an assignment and I was hoping to know if the merge sort algorithm I wrote in Python was not making any unnecessary computations.

def sort(some_list):

    if len(some_list) > 1:
        return merge(sort(some_list.__getslice__(0, len(some_list)/2)),\
         sort(some_list.__getslice__(len(some_list)/2, len(some_list))))
    else:
        return some_list

def merge(list_left, list_right):

    sorted_list = []

    while len(list_left) > 0 or len(list_right) > 0:

        if len(list_left) == 0:
            sorted_list.append(list_right.pop(0))

        elif len(list_right) == 0:
            sorted_list.append(list_left.pop(0))

        elif list_left[0] < list_right[0]:
            sorted_list.append(list_left.pop(0))

        else:
            sorted_list.append(list_right.pop(0))
    return sorted_list

I did some testing by calling time function to check how much time it took to merge a list of numbers of size 100,000 with different presortedness values (where a presortedness = 0 means it's completely random and presortedness = 1 means the list is already sorted).

I'm using a core i7 processor and working from PyCharm on an Ubuntu 14.04 OS.

This was my result:

-------Test for Merge Sort----------------
Presortedness = 0.00 (completely random)
Time elapsed = 1.90153600

Presortedness = 0.25
Time elapsed = 1.89535800

Presortedness = 0.50
Time elapsed = 1.90894200

Presortedness = 0.75
Time elapsed = 1.90660100

Presortedness = 1.00 (sorted)
Time elapsed = 1.79297100

It is actually a terribly inefficient implementation... Some things to keep in mind:

Do not use the __getslice__ method explicitly.  It is a relic of a time long gone, and you only need to know about it if subclassing objects written in CPython. Not your case. And your code can be written in an infinitely more readable form as:

def sort(some_list):
    if len(some_list) > 1:
       return merge(sort(some_list[:len(some_list)/2],
                     sort(some_list[len(some_list)/2:]))
    else:
        return some_list

Slicing a Python list creates a copy of the list. This is not what you typically want in a merge sort.  To avoid this, you can leave your original list unsliced, and pass around indices to the intervals being sorted and merged.  Such an in-place merge sort could be written as:

def sort(list_, lo=None, hi=None):
    if lo is None:
        lo = 0
    if hi is None:
        hi = len(list_)
    if hi - lo <= 1:
        return
    mid = lo + (hi - lo) // 2
    sort(list_, lo, mid)
    sort(list_, mid, hi)
    merge(list_, lo, mid, hi)

def merge(list_, lo, mid, hi):
    temp = list_[lo:mid]
    left = 0
    right = mid
    write = 0
    while left < mid - lo and right < hi:
        if list_[right] < temp[left]:
            list_[write] = list_[right]
            right += 1
        else:
            list_[write] = temp[left]
            left += 1
        write += 1
    if left < mid - lo
        list_[write:] = temp[left:]

This is not a very Pythonic implementation.  It actually looks like C or Java translated line by line into Python.  But there is not much way around it.  Unless you simply do sort(list_), which is the Pythonic way of getting a listy sorted.


Perform SELECT INTO Statement and add Primary Key afterwards How can I imrpove the performance of this T-SQL statement?

SELECT Code, Description
INTO [DBApps_Pulse_WarehouseAreas].[dbo].[Tbl_Partmaster]
FROM SYSDataBridge.dbo.View_Pulse_Partmaster_Synchronized

ALTER TABLE [DBApps_Pulse_WarehouseAreas].[dbo].[Tbl_Partmaster] ADD PRIMARY KEY (Code)

Your current code effectively writes the table twice.

Once as a heap and then again as a clustered index (with an intermediate sort step) after you add the primary key.

I'd just create the table upfront with the desired primary key and insert into it.

USE DBApps_Pulse_WarehouseAreas;

CREATE TABLE [dbo].[Tbl_Partmaster]
  (
     Code        VARCHAR(10) NOT NULL CONSTRAINT PK_Tbl_Partmaster PRIMARY KEY,
     Description NVARCHAR(500) NOT NULL
  );

INSERT INTO [dbo].[Tbl_Partmaster]  WITH (TABLOCKX)
SELECT Code,
       Description
FROM   SYSDataBridge.dbo.View_Pulse_Partmaster_Synchronized;

For at least three versions of SQL Server an insert into an empty B tree has been able to be minimally logged.

In order to avoid page splits there might be a sort operator in the execution plan if the estimated number of rows is sufficiently high (or if the source has an index that could potentially present the selected columns in order of code this direct insert might avoid the need to sort all together).

Finally avoid Hungarian notation such as Tbl_. At best it adds nothing but clutter. At worst at some point you might want to refactor the database and transparently replace the table with a view without updating calling code. Then the Tbl_ prefix would be down right misleading.


Two-player match-picking game I'm looking for some feedback on my first program. It's a small game you play with matches.

It's a two-player game, where the players take turns picking matches (1, 2 or 3) and cannot pick the same number of matches as the other player: if player one picks 2 matches, player two will have to pick either 1 or 3 matches. The player who cannot pick anymore matches loses the game, which means there are two end scenarios : when there are no more matches, or there is 1 but the other player picked 1 during the previous round.

Is there a more efficient way to do something (performance-wise or with less code to write)? Can I improve the overall presentation/organization of the code? (am I unaware of a convention?) Any advice or criticism is very welcome.

import java.util.Scanner ;

public class JeuDeNim2 {

public static void main(String[] args) {

    Scanner sc = new Scanner(System.in) ;

    int totalMatches ;
    do {
        System.out.println("How many matches do you want to play with? "
        + "(from 6 to 60)") ;
        totalMatches = sc.nextInt() ;
    }
    while (totalMatches < 6 || totalMatches > 60) ;

    int matchesThisTurn ;
    int matchesPreviousTurn = 0 ;
    int round = 0 ;
    int player = 0 ;
    int previousPlayer ;

    while (true) {

        round++ ;
        previousPlayer = player ;
        player = round % 2 ;
        if (player == 0) {
            player = 2 ;
        }

        while (true) {

            if (totalMatches == 1) {
                System.out.println("There is only one match left on "
                + "the table") ;
            } else {
                System.out.println("There are " + totalMatches
                + " matches on the table") ;
            }

            if (round == 1 || round == 2) {
                System.out.println("Player " + player + ": How many "
                + "matches do you want to pick? (1, 2 or 3)") ;
            } else {
                System.out.println("Player " + player + ": How many "
                + "matches do you want to pick this turn?") ;
            }

            matchesThisTurn = sc.nextInt() ;

            if (matchesThisTurn < 1 || matchesThisTurn > 3) {
                System.out.println("Wrong entry: you have to pick 1, 2 "
                + "or 3 matches") ;
                continue ;
            }
            if (matchesThisTurn == matchesPreviousTurn) {
                System.out.println("You cannot pick the same number of "
                + "matches as Player " + previousPlayer) ;
                continue ;
            }
            if (totalMatches == 1 && matchesThisTurn > totalMatches) {
                System.out.println("You cannot pick " + matchesThisTurn
                + " matches: there is only one match left") ;
                continue ;
            }
            if (matchesThisTurn > totalMatches) {
                System.out.println("You cannot pick " + matchesThisTurn
                + " matches: there are only " + totalMatches
                + " matches left") ;
                continue ;
            }
            break ;
        }

        totalMatches -= matchesThisTurn ;
        matchesPreviousTurn = matchesThisTurn ;

        if (totalMatches == 0) {
            System.out.println("*** There are no more matches! ***") ;
            break ;
        }
        if (totalMatches == 1 && matchesThisTurn == 1) {
            System.out.println("*** There is only one match left, but "
            + "Player " + player + " already took one! ***") ;
            break ;
        }
    }

    System.out.println("*** The game is over! Player " + player
    + " Wins! ***") ;
}
}

Java is an Object Oriented language... and I recognize that objects are not always the right solution to a problem.... The generally accepted "opposites" of object-orientation, though, are "procedural", and "functional". It is common (for example, in C) to write code as a collection of procedures, or (in haskell) to write it as functions.

In almost no languages, though, is it common to have no procedures, no objects, and no functions... well, except shell scripts, I guess. Your program has no methods, no objects, no functions, except the main method.

You need to break your code down in to parts that do logically isolated things, and then call those reusable chunks when needed.

Function 1

The first function I would extract, is a start-of-game match-count:

private static int getStartCount(Scanner sc) {
    while(true) {
        System.out.println("How many matches do you want to play with? "
                + "(from 6 to 60)") ;
        int totalMatches = sc.nextInt() ;
        if (totalMatches >= 6 && totalMatches <= 60) {
            return totalMatches;
        }
    }
}

Note, in that function I converted it from a do-while loop, to an infinite while-loop, with an early-return if the input is valid. I have nothing against do-while loops, but I find this early-return system simpler in terms of variable management. Note that the int totalMatches has a smaller scope than your code. You can call the code with:

int totalMatches = getStartCount(sc);

Function 2 (and 3)

Right, here's your input issue.... you have a big while loop, that plays each round, and inside that it has a user-input loop. The conditions on that loop are... confusing. What you have, is a prompt, some validation, and if everything is OK, you break. If there's a problem, you loop again. Again, extracting a function with an early return value, would be useful.

As an aside, when it comes to user input, it is almost always better to inform the user what input would be valid before requesting the input. Telling them they made a mistake afterwards is great, but telling them what their options are before, is better. Consider these two functions, the first function computes what would be a valid input, the second function uses that information to prompt the user:

private static int[] computeAllow(int totalMatches, int previous) {
    if (totalMatches == 1) {
        return new int[]{1};
    }
    if (totalMatches == 2 && previous < 3 && previous > 0) {
        return previous == 1 ? new int[]{2} : new int[]{1};
    }
    switch (previous) {
        case 0:
            return new int[]{1,2,3};
        case 1:
            return new int[]{2,3};
        case 2:
            return new int[]{1,3};
        case 3:
            return new int[]{1,2};

    }
    throw new IllegalStateException("Unexpected previous count " + previous);
}

Then, use this function like:

private static int getPlayerPick(Scanner sc, int totalMatches, int player, int previous) {
    if (totalMatches == 1) {
        System.out.println("There is only one match left on "
                + "the table") ;
    } else {
        System.out.println("There are " + totalMatches
                + " matches on the table") ;
    }

    final int[] allow = computeAllow(totalMatches, previous);

    while (true) {

        System.out.printf("Player %d: How many matches do you want to pick? %s",
                player, Arrays.toString(allow));

        final int matchesThisTurn = sc.nextInt();
        for (int a : allow) {
            if (a == matchesThisTurn) {
                // Valid input.
                return matchesThisTurn;
            }
        }

        System.out.printf("Wrong entry: there are %d matches, "
            + "the last player selected %d, "
            + "which means you can only select one of %s\n",
                totalMatches, previous, Arrays.toString(allow));
    }
}

Main

Puting this all together, the main method becomes much simpler:

public static void main(String[] args) {

    try (Scanner sc = new Scanner(System.in) ;) {

        int totalMatches = getStartCount(sc);

        int matchesPreviousTurn = 0 ;
        int round = 0 ;
        int player = 0 ;

        do {

            //previousPlayer = player ;
            player = 1 + (round % 2);
            round++ ;

            int matchesThisTurn = getPlayerPick(sc, totalMatches, player, matchesPreviousTurn);

            totalMatches -= matchesThisTurn ;
            matchesPreviousTurn = matchesThisTurn ;

        } while (totalMatches > 1 || totalMatches == 1 && matchesPreviousTurn != 1);

        System.out.println("*** The game is over! Player " + player
                + " Wins! ***") ;
    }
}

It is still probably too busy, but you can at least see what's going on now.

Note how the user is prompted with valid values before they are entered now:

How many matches do you want to play with? (from 6 to 60)
10
There are 10 matches on the table
Player 1: How many matches do you want to pick? [1, 2, 3]
4
Wrong entry: there are 10 matches, the last player selected 0, which means you can only select one of [1, 2, 3]
Player 1: How many matches do you want to pick? [1, 2, 3]
2
There are 8 matches on the table
Player 2: How many matches do you want to pick? [1, 3]
2
Wrong entry: there are 8 matches, the last player selected 2, which means you can only select one of [1, 3]
Player 2: How many matches do you want to pick? [1, 3]
3
There are 5 matches on the table
Player 1: How many matches do you want to pick? [1, 2]
3
Wrong entry: there are 5 matches, the last player selected 3, which means you can only select one of [1, 2]
Player 1: How many matches do you want to pick? [1, 2]
2
There are 3 matches on the table
Player 2: How many matches do you want to pick? [1, 3]
1
There are 2 matches on the table
Player 1: How many matches do you want to pick? [2]
2
*** The game is over! Player 1 Wins! ***


Class CatalogWalker can walk thru a list of catalogs, calculate and report some statistics Just for review and may be useful for somebody

from os import walk
from time import time

from abc import ABCMeta, abstractmethod

class Reporter(object):
    __metaclass__=ABCMeta

    @abstractmethod
    def report(self, catalogs_cntr, files_ctnr, speed):
        """ output catalogs_cntr, files_ctnr, speed to somewhere """
        #pass

class ConsoleIndicator(Reporter):
    def __init__(self, out_splitter):
        self.__out_splitter=out_splitter

    def report(self, catalogs_cntr, files_ctnr, speed):
        if self.__out_splitter and self.__out_splitter.pipes:
            _result = "\rcataloges:" + repr(catalogs_cntr).rjust(8)
            _result += "\tfiles:" + repr(files_ctnr).rjust(13)
            _result += "\tspeed: %12.3f"%speed + " files/s"

            self.__out_splitter.write(_result)
            self.__out_splitter.flush()

class CatalogsWalker(object):
    """ """

    def __init__(self, catalogs, reporter=None):
        """ """

        self.__files_cntr, self.__catalogs_cntr, self.__start_time, self.__speed = 0, 0, 0, 0
        self.__reporter = reporter
        self.__catalogs = catalogs if hasattr(catalogs, "__iter__") else [catalogs]

    def __iter__(self):
        self.__files_cntr, self.__catalogs_cntr, self.__start_time, self.__speed = 0, 0, 0, 0
        self.__start_time=time()
        for catalog in self.__catalogs:
            for p,d,ns in walk(catalog):
                _ = d;
                self.__catalogs_cntr+=1
                for n in ns:
                    self.__files_cntr+=1
                    self.__speed=self.__files_cntr/(time()-self.__start_time)
                    if self.__reporter:
                        self.__reporter.report(self.__catalogs_cntr, self.__files_cntr, self.__speed)
                    yield p,n

    @property
    def reporter(self):
        return self.__reporter

    @property
    def catalog_counter(self):
        return self.__catalogs_cntr

    @property
    def file_counter(self):
        return self.__files_cntr

    @property
    def start_time(self):
        return self.__start_time

    @property
    def speed(self):
        return self.__speed

Docstrings

I'm not a big fan of the empty-docstring fakeout """ """. It would be really useful to see some actual documentation of what this is for and how to use it.

Names

You have a lot of underscores. Per the style guide:

  Generally, double leading underscores should be used only to avoid name conflicts with attributes in classes designed to be subclassed.

Given that you use the unmangled names as properties, this is clearly not the case; you should use a single underscore to indicate private by convention.

I certainly wouldn't prepend an underscore on local names (e.g. _result), as they can't be accessed from outside the method anyway.

You have a few too-short names, too; p? d? ns? n? Your code would be much more readable with more meaningful names.

Strings

I would have written e.g.

_result = "\rcataloges:" + repr(catalogs_cntr).rjust(8)

as

result = '\rcataloges:{!r:>8}'.format(catalogs_cntr)

and I definitely wouldn't mix and match; why:

_result += "\tspeed: %12.3f"%speed + " files/s"

rather than

_result += "\tspeed: %12.3f files/s" % speed

You could also consider building a list of elements, then joining them together; this makes it easier to add, rearrange and remove fields or change the separator:

result = []
result.append("cataloges:{!r:>8}".format(catalogs_cntr))
result.append("files:{!r:>13}".format(files_ctnr))
result.append("speed: {:12.3f} files/s".format(speed))
result = '\r' + '\t'.join(result)

Code

You have lines that are too long - you should limit to 80 characters. You can reduce this using Python's continuation, e.g.

self.__reporter.report(self.__catalogs_cntr, self.__files_cntr, self.__speed)

becomes

self.__reporter.report(self.__catalogs_cntr,
                       self.__files_cntr,
                       self.__speed)

This is particularly odd-looking:

for p,d,ns in walk(catalog):
    _ = d;

The semi-colon ; is redundant, and you won't see it very often in Python code (except where trying to fit multiple statements on one line, which is rarely the right choice). If you aren't going to use d, don't assign it at all, and note the whitespace after commas:

for p, _, ns in walk(catalog):