Simulate the "cd" command of a file system Write a function that provides change directory (cd) function for an
  abstract file system.

  Notes:

  Root path is '/'.
  Path separator is '/'.
  Parent directory is addressable as "..".
  Directory names consist only of English alphabet letters (A-Z and a-z).

  For example, new Path("/a/b/c/d").cd("../x").getPath() should return
  /a/b/c/x.

  Note: The evaluation environment uses '\' as the path separator.

It works, but is there a better way to do this?

public class Path {
    private String path;

    public Path(String path) {
        this.path = path;
    }

    public String getPath() {
        return path;
    }

    public Path cd(String newPath) {

        //1step consume newPath
        //newPath to list
        //take 1 char, head pull
        //locate it with this.path,
        //          case / ../ ./
        //          this.path end poll
        //return changed path and newPath

        //recursive cd again

        if( "".equals(this.path)||"".equals(newPath)||!this.path.startsWith("/"))
            return this;
        else if(newPath.startsWith("/")) {
            this.path = "/";
            return this.cd(newPath.length()>1?newPath.substring(1):"");

        }
        else{
            if(newPath.startsWith("..."))
                return this;
            else if(newPath.startsWith("../")) {
                this.cdup1();
                return this.cd(newPath.length()>3?newPath.substring(3):"");
            }else if(newPath.startsWith("./")) {
                return this.cd(newPath.length()>2?newPath.substring(2):"");
            }
            else if(newPath.startsWith("..")) {
                this.cdup1();
                return this.cd(newPath.length()>2?newPath.substring(2):"");
            }else if(newPath.startsWith(".")) {
                return this.cd(newPath.length()>1?newPath.substring(1):"");
            }
            else {

                String [] strarr = newPath.split("/");
                if(this.path.endsWith("/"))
                    this.path = this.path+strarr[0];
                else
                    this.path = this.path+"/"+strarr[0];

                String newPath2 ="";
                for (int i = 1; i < strarr.length; i++) {
                    String s = strarr[i];
                    newPath2=newPath2+s+"/";
                }
                return this.cd(newPath2);

            }

        }

    }

    private void cdup1(){
        if("/".equals(this.path)) return ;

        String [] patharr=this.path.split("/");
        System.out.println(patharr.length);
        StringBuffer s = new StringBuffer("/");

        for (int i = 0; i < patharr.length-1; i++) {
            if(!"".equals(patharr[i]))
                s.append( patharr[i]).append("/");
        }

        String endpath = s.toString();

        if(!"/".endsWith(endpath)&&endpath.endsWith("/")) endpath=endpath.substring(0,endpath.length()-1);

        this.path = endpath;
        System.out.println(this.path);

        return;
    }

    public static void main(String[] args) {
        Path path = new Path("/a/b/c/d");
        System.out.println(path.cd("../../../x").getPath());
    }
}

Overcomplicated

The implementation is really overcomplicated and hard to read.

  It works, but I feel there must be better way to do that?

Your instincts are good. When you know there's a better way, something you can try is start over from scratch using a different algorithm and see where that goes. Another technique is using TDD and let the tests drive your design.

Poor readability

The code is too compactly written at many places, for example:

    if( "".equals(this.path)||"".equals(newPath)||!this.path.startsWith("/"))
    // ...

    return this.cd(newPath.length()>1?newPath.substring(1):"");
    // ...

    return this.cd(newPath.length()>3?newPath.substring(3):"");

This is really hard to read like that and doesn't help the understandability of the code.
All modern IDE's have an auto-reformat function that will put spaces around operators and make the code much easier to read.

Normalization

The cd method effectively normalizes the path, for example a/b/.. becomes correctly a. But the initial path passed to the constructor is never normalized. For example new Path("/a/b/../..").cd("c").getPath() returns /a/b/../../c instead of c.

This is a bit confusing inconsistency. It would be better to normalize the initial path too.

With a good normalizer method, the implementation of cd itself could be much simpler:

public Path cd(String newPath) {
    if (isAbsolutePath(newPath)) {
        path = normalizePath(newPath);
        return this;
    }

    path = normalizePath(path + SEPARATOR + newPath);
    return this;
}

In this case, of course, normalizePath does the real heavy lifting.
It could go something like this:

private String normalizePath(String path) {
    boolean isAbsolute = isAbsolutePath(path);

    List<String> parts = new ArrayList<>();
    for (String part : path.split(SEPARATOR)) {
        if (part.isEmpty() || part.equals(".")) {
            continue;
        }
        if (part.equals("..")) {
            if (parts.isEmpty()) {
                if (isAbsolute) {
                    continue;
                }
            } else {
                if (!parts.get(parts.size() - 1).equals("..")) {
                    parts.remove(parts.size() - 1);
                    continue;
                }
            }
        }
        parts.add(part);
    }

    String prefix = isAbsolute ? SEPARATOR : "";
    return prefix + String.join(SEPARATOR, parts);
}

Bug or unclear specs?

What should new Path("a/b").cd("..").getPath() return?
I would expect a, but in fact it gives a/b.

What about new Path("a/b").cd("../../..").getPath()?
I would expect .., but it gives a/b for this too.

Special treatment of ...

There is a condition for path segments starting with .... Why is that? The description says that directory names contain only letters of the alphabet.
That makes this special treatment odd.
It seems that such path segment is illegal, and throwing an IllegalArgumentException would seem appropriate.

Testing

For something a bit tricky like this,
the main method in the class is really far too little to verify that the class works well.
Not to mention of course that testing in a main method is not a good way.
I strongly recommend to develop test cases and cover as many interesting corner cases as possible, to flush out obscure bugs.


String Matching w/o repeating the comparisons for common part of the two strings In an attempt to think up a more efficient String Matching algorithm than the naïve \$O(n \cdot k)\$ one I came up with a slightly modified approach.

In my algorithm (code in Java), I tried a little modification of the \$O(n \cdot k)\$ approach where every time a character match starting from i fails after k positions, we start from i+1th place.

In this algo however, if matching fails at k position, I resume comparison from k-1 because we know up to k-1 the characters are common.

Below is the implementation of this approach.

I will really appreciate anyone taking a look into the code and

Confirm if things work the way I explained. My tests seem to be working fine.
What is the complexity of this approach? Is it worst case \$O(n \cdot k)\$ still?
An example of a worst case input.

Code:

public class ModifiedSearch {
    private String text;

    public ModifiedSearch(String text) {
        this.text = text;
    }

    public int findPattern(String pattern) {
        int textLength = text.length();
        int patLength = pattern.length();
        int common = 0;
        boolean broken = false;

        for (int tIndex = 0; tIndex <= textLength - patLength; broken = false, tIndex++) {
            int k = 0;

            while (tIndex + common + k < textLength && common + k < patLength) {

                if (text.charAt(tIndex + common + k) != pattern.charAt(common + k)) {
                    broken = true;
                    common = (common + k - 1 < 0) ? 0 : common + k - 1 ;
                    break;
                }

                k++;
            }

            if (!broken && common + k == patLength) {
                return tIndex;
            }
        }

        return -1;
    }
}

I have tried this with a combination of inputs large and small, but this does not seem to worse out to \$O(n \cdot k)\$. Any help greatly appreciated.

Confirm if things work the way I explained. My tests seem to be working fine.

I'm afraid not. For example, for text hehehello and pattern hehello, the output is 0 instead of 2. You see, the algorithm prematurely exits after heheh, when it should continue searching from position 2.

With your modified version this test case passes, but fails for text heheehello and pattern hehello, incorrectly returning 3 instead of -1.
(In the future, please do not modify the code in the question after you received answers. Changing the code typically invalidates answers, as it did in this example too.)

See the Knuth-Morris-Pratt algorithm for something that actually works.

  What is the complexity of this approach? Is it worst case O(n⋅k) still?

The complexity of your algorithm is \$O(n + k)\$, same as that of KMP. Unfortunately it doesn't really matter, because it doesn't work correctly.

Usage

The usage of your class is a bit awkward:

int index = new ModifiedSearch(text).findPattern(pattern);

I recommend to rewrite to make it work this way:

int index = ModifiedSearch.findPattern(text, pattern);

That is, turn the class into a static utility method.
It will be lighter that you don't have to create an instance of the class.

Looking at the function name "findPattern" alone, it's not obvious what it returns. Since the function returns an index, like String.indexOf, I recommend to rename it to indexOf. Or, rename to contains and make it return a boolean.


JavaScript onKeypress validation This is a function for JavaScript validation:

var alpha = "[ A-Za-z]";
var numeric = "[0-9]";
var alphanumeric = "[ A-Za-z0-9]";

function onKeyValidate(e,charVal){
    var keynum;
    var keyChars = /[\x00\x08]/;
    var validChars = new RegExp(charVal);
    if(window.event)
    {
        keynum = e.keyCode;
    }
    else if(e.which)
    {
        keynum = e.which;
    }
    var keychar = String.fromCharCode(keynum);
    if (!validChars.test(keychar) && !keyChars.test(keychar))   {
        return false
    } else{
        return keychar;
    }
}

and HTML code:

<input type="text"  name="shipname"  onkeypress="return onKeyValidate(event,alpha);"/>
<input type="text"  name="price"   onkeypress="return onKeyValidate(event,numeric);" />

I want to know about code quality, creating issues or any alternative for this.  I need some suggestion from experts.

onKeyValidate is an okay name, but a better name could be validateKeypress.
It seems very silly to store a RegExp as a string, and then construct it every time. Why not just declare var alpha = /[ A-Za-z]/?
keyChars appears to check against \x00, the null character, and \x08, the backspace character. Neither of these can ever be passed to onKeypress, so you can just take it out.
The standard way to get the character code is event.which || event.keyCode.
event is a global; I don't think you need to pass it in.

Here's a proposed rewrite:

var alpha = /[ A-Za-z]/;
var numeric = /[0-9]/;
var alphanumeric = /[ A-Za-z0-9]/;

function validateKeypress(validChars) {
    var keyChar = String.fromCharCode(event.which || event.keyCode);
    return validChars.test(keyChar) ? keyChar : false;
}

The HTML will have to change to onkeypress="validateKeypress(alpha);".


Optimising a user's-choice sorting algorithm With the help of some nice folks at Stack Overflow, I wrote an algorithm that will give a user three buttons: option a neither option b Clicking on a button will add one to the score of a object, and it will systematically go through each match-up to get an equal amount of questions for each character.

Here's the meat of the function:

var pos1 = 0; //position in the array
var pos2 = 1; //ditto

var array = [ //array used
    {name: "apple", score: 0},
    {name: "pear", score: 0},
    {name: "cherry", score: 0},
    {name: "banana", score: 0},
    {name: "orange", score: 0},
    {name: "watermelon", score: 0},
    {name: "bread" score: 0},
    {name: "a duck" score: 0}
]

var option1 = array[0]; //keeps track of which option is shown in the button
var option2 = array[1]; //ditto

function select(slot) { //called when user presses button
    if (slot == 1) { //left button
        array[pos1].score++;
    } else if (slot == 2) { //right button
        array[pos2].score++;
    } //for neither, it does select(0)

    if (pos2 < array.length - 1) {
        pos2++;
        option2 = array[pos2];
    } else if (pos1 < array.length - 2) {
        pos1++;
        pos2 = 1 + pos1;
        option1 = array[pos1]
        option2 = array[pos2]
    } else {
        output(); //sorts and displays
        return;
    }
}

It works pretty well for smaller numbers of objects, but it gets totally absurd at higher numbers. For example, in practice, one of my sets has around 50 objects, resulting in ~1500 questions, which takes hours!

As a side note, I found this website. Ignoring the content, it's able to sort a similar number (55) of objects with only 170 questions. It's also somehow able to generate a percentage of completion, but that's just a bonus.

Conveniently, the author has the code embedded directly in the HTML, so the JavaScript is easy to view. However, I can't figure out how it works, but it's the type of efficiency I'm looking for.

You'll have to excuse my javascript, which is kind of lacking, but you can use any comparison based algorithm to make the sorting happen in \$O(n \log n)\$ comparisons, where a comparison is one of your user powered tests. I think merge sort is guaranteed to need exactly \$n \log n\$, and is relatively simple to implement, so you could implement a bottom-up mergesort by doing something along the lines of:

var array = ["apple", "pear", "cherry",  "banana", "orange", "watermelon",
             "bread", "a duck"];
var temp = [];

var merge_len = 1
var merge_lo = 0;
var merge_mid = 1;
var merge_hi = 2;
var merge_ptr_1 = 0;
var merge_ptr_2 = 1;

function mergesort_step(slot) {
    if (slot == 2) { // right button pressed
        temp.push(array[merge_ptr_2]);
        merge_ptr_2++;
    }
    else {
        temp.push(array[merge_ptr_1]);
        merge_ptr_1++;
    }

    if (merge_ptr_1 == merge_mid || merge_ptr_2 == merge_end) {
        // Copy any remaining items in 1st merged subarray to temp
        for (i = merge_ptr_1; i < merge_mid; merge_ptr_1++) {
            temp.push(array[merge_ptr_1]);
        }
        // Copy any remaining items in 2nd merged subarray to temp
        for (i = merge_ptr_2; i < merge_hi; merge_ptr_2++) {
            temp.push(array[merge_ptr_2]);
        }
        // Overwrite the two subarrays with the sorted temp
        for (i = 0; i < merge_hi - merge_lo; i++) {
            array[i + merge_lo] = temp[i];
        }
        temp = []
        // Set up the next two subarrays to merge
        if (merge_hi + merge_len >= array.length) {
            merge_lo = 0;
            merge_len *= 2;
            if (merge_len >= array.length) {
                // We are finished!
                output();
                return
            }
        }
        else {
            merge_lo = merge_hi;
        }
        merge_mid = merge_lo + merge_len;
        merge_hi = merge_mid + merge_len;
        if (merge_hi > array.length) {
            merge_hi = array.length();
        }
        merge_ptr_1 = merge_lo;
        merge_ptr_2 = merge_mid;
    }
}

Here merge_ptr_1 and merge_ptr_2 have taken the place of your pos1 and pos2, and I have skipped your option1 and option2 entirely to not bloat the code up any more.


Sum of the digits is prime or not Puzzle Description:

  A number is called lucky if the sum of its digits, as well as the sum
  of the squares of its digits is a prime number. How many numbers
  between A and B are lucky?

How can I improve performance of the following code?

import java.util.Scanner;

public class lucky_num {

    public static void main(String[] args) {

        lucky_num sr = new lucky_num();
        Scanner scanner = new Scanner(System.in);
        int no_cases = scanner.nextInt();
        for (int i = 0; i < no_cases; i++) {
            System.out.println(sr.solve(scanner.nextLong(), scanner.nextLong()));
        }
    }

    private int solve(long l, long m) {
        int count = 0;
        for (long i = l; i <= m; i++) {
            if (logic(i)) {
                count++;
            }
        }
        return count;
    }

    private boolean logic(long i) {
        return (isSUM(i) && isSUMsq(i));
    }

    private boolean isSUMsq(long i) {
        int sum = 0;
        while (i > 9) {
            long k = i % 10;
            i = i / 10;
            sum += k * k;
        }
        sum += i * i;
        return (isPrime(sum));
    }

    private boolean isSUM(long i) {
        int sum = 0;
        while (i > 9) {
            long k = i % 10;
            i = i / 10;
            sum += k;
        }
        sum += i;
        return (isPrime(sum));
    }

    private boolean isPrime(int num) {
        if(num==2)
        return true;
        // check if n is a multiple of 2
        if (num % 2 == 0 || num==1 )
            return false;
        // if not, then just check the odds
        for (int i = 3; i * i <= num; i += 2) {
            if (num % i == 0)
            return false;
        }
        return true;
    }
}

Sample input:

2
1 20
120 130

Sample output:

4
1

Constraints:

1 <= T <= 10000
1 <= A <= B <= 10^18

The maximum sum of squares-of-digits of an \$n\$-digit number is \$n\cdot9\cdot9 = n\cdot81\$.  The number of digits in a number \$B\$ is \$\lceil\log_{10}(B)\rceil\$.

Since you only need to sieve to the square root of the number you're testing for primality, you only need to run your sieve from \$3\$ to \$\sqrt{\lceil \log_{10}(B)\rceil\cdot81}\$.  Even for \$B\$ = 1 billion, this means the max you need to sieve to is \$28\$.

So, run the sieve once, and simply cache the results.

Also, if you wanted to be really cool, you could probably get some minor speed-ups by not calculating the sums-of-digits, and looping over them instead.

What I mean by that is this:  notice that the sum of digits of \$32x\$ (where \$x\$ is any digit - read that as "three hundred and twenty-\$x\$") is always going to be \$3 + 2 + x\$, and the sum-of-squares will always be \$9 + 4 + x^2\$.  Thus, instead of looping from \$320\$ to \$329\$ and calculating the sum-of-digits and sum-of-square-of-digits every time, you could just loop from \$x = 0\$ to \$x = 9\$, and check \$3+2+x\$ and \$9+4+x^2\$ for primality.

Using a bit of recursion, you could loop over all values of all digits.


String sanitisation function I have a search function on my website. In addition, elsewhere I have a place where users can submit categories for their posts. I want the categories to have an uppercase first letter and the rest lower case. I know there far more complex ways to secure a user input string, but do you think that as a fairly basic function, this is relatively secure?

    function purify_string($string){
       $script_tags = "/\<script +(.+)<\/script>+/i";
       $string = strip_tags(addslashes($string));
       $string = preg_replace($script_tags, '', $string);
       $string = trim($string);
       $string = strtolower($string);
       $string = ucfirst($string);
       return $string;
     }

Instead of trying to purify the category for the user, which may or may not have unexpected results, you should simply reject everything that does not conform to your level of standard, and let the user fix it:

function isValidCategoryName($string) {
    return preg_match("/^[A-Z][a-z0-9 ]+$/", $string);
}

If it doesn't match, prompt the user with an error message and tell him to try again. Your job isn't to try to extract a good category from a bad one.

At most you could just correct the case with ucfirst if you want.


Loop through string array with exit if test passed I need to return True if a page contains any of a list of element types. This code works.

var elements = 0;
var content = $('.TestModule');
if (content !== null && typeof (content) !== "undefined") {
    elements += content.first().find('.btn').length;
    elements += content.first().find('.textbox').length;
    elements += content.first().find('.ui-jqgrid').length;
    elements += content.first().find('.flexboxdiv').length;
    elements += content.first().find('.headerlabel').length;
}
return elements > 0;

But always runs every test which is obviously inefficient so I changed to;

var valid = false;
var content = $('.TestModule');
if (content !== null && typeof (content) !== "undefined") {
    var elementTypes = ['btn', 'textbox', 'ui-jqgrid', 'flexboxdiv', 'headerlabel'];
    for (i = 0; i < elementTypes.length; i++) {
        var num = content.first().find('.' + elementTypes[i]).length;
        if (num > 0) {
            valid = true;
            break;
        }
    }
}
return valid;

Which also works and runs the minimum number of tests. But it looks pretty clunky to me. Can you suggest a better loop/structure/test, please? I don't need a count of element types, just to know whether one or more of any type exists.

Join the string to create a jQuery selector

var elementTypes = ['btn', 'textbox', 'ui-jqgrid', 'flexboxdiv', 'headerlabel'];
var selector = '.' + elementTypes.join(', .');

return $('.TestModule').first().find(selector).length > 0;

The variable selector will return the string '.btn, .textbox, .ui-jqgrid, .flexboxdiv, .headerlabel' which can be used in the jQuery selector.

The statement

return $('.TestModule').first().find(selector).length > 0;

is equivalent to

return $('.TestModule').first().find('.btn, .textbox, .ui-jqgrid, .flexboxdiv, .headerlabel').length > 0;

after the string substitution.

To make it more dynamic, store complete selector in the array and join the array by ,-comma.

var elementTypes = ['.btn', '.textbox', '.ui-jqgrid', '.flexboxdiv', '.headerlabel', '#myId', 'input', 'input[name="firstname"]'];
var selector = elementTypes.join(', ');


Calculate volume of a cylinder with user input I'm looking for a fast OR simple way to calculate the volume of a user-entered cylinder.  Here is my solution:

#include <iostream>
using namespace std;
#include <math.h>

int main()
{
    long double pi = acos(-1);
    cout.precision(1000);
    long double radius;
    long double volume;
    long double height;
    cout << "Hello to the cylinder volume solver!" << endl;
    cout << "Please enter the cylinder's radius: ";
    cin >> radius;
    long double radius_sq = radius * radius;
    cout << endl << "Please enter the cylinder's height: ";
    cin >> height;
    volume = pi * radius_sq * height;
    cout << endl << "The cylinder's volume is " << volume << "." << endl;
    return 0;
}

It's obvious to see that I'm a new programmer from the way I wrote this....which is why I want to learn from your remarks.

Your implementation is quite simple,
but a bit hard to read:

It's a wall of code, with no vertical spacing to visual separate closely related groups
The ordering of statements is haphazard, with no discernible underlying logic

Consider this alternative:

#include <iostream>
#include <cmath>

long double calculateCylinderVolume(long double radius, long double height)
{
    return acos(-1) * radius * radius * height;
}

int main()
{
    std::cout << "Hello to the cylinder volume solver!\n";

    std::cout << "Please enter the cylinder's radius: ";
    long double radius;
    std::cin >> radius;

    std::cout << "\nPlease enter the cylinder's height: ";
    long double height;
    std::cin >> height;

    std::cout.precision(1000);
    std::cout << "\nThe cylinder's volume is "
        << calculateCylinderVolume(radius, height) << ".\n";
}

Notice that:

variables are not declared sooner than really needed
the std::cout.precision(1000); is delayed until it's actually needed
the blank lines visually separate the closely related statements, improving readability

Other minor improvements:

using namespace std is considered bad practice
return 0 at the end of main is unnecessary
\n is more efficient than std::endl
#include <cmath> instead of #include <math.h>, as pointed out by @FredLarson


Contains all elements and duplicates are taken into account The idea is to have a function which checks if all elements in a list are contained in another list. But not just the containedness has to be true, but also the times of containedness.
One could say the function has to be an extended version of containsAll , which checks only the containedness.
Here's my implementation of such a function:
fun main(args: Array<String>) {
    val toCheck = listOf(1, 2, 3, 4, 5, 6, 1, 1, 3)
    val expected = listOf(1, 1, 1, 3, 3, 4)

    val result = containsAllSameCount(toCheck, expected)
    println("Result => $result")

    val expected2 = listOf(1, 1, 3, 3, 4) // One 1 is missing!
    val result2 = containsAllSameCount(toCheck, expected2)
    println("Result 2 => $result2")
    /*
        Result => true
        Result 2 => false
     */
}

fun containsAllSameCount(toCheck: List<Int>, expected: List<Int>): Boolean {
    for (current in expected) {
        var currentInExpected = expected.filter { it == current }
        var currentInToCheck = toCheck.filter { it == current }

        if (currentInExpected.size != currentInToCheck.size) {
            return false
        }
    }

    return true
}

It's clear to me, that the filter-methods are used multiple times, when the same element is contained multiple times. Therefore I suppose that my implementation isn't the most efficient way to solve the task.
What do you think about my implementation?
What's a better way to solve the way?

The best way would be to change data structure. Instead of using a List<Int> you can use a Map<Int, Int>. If you want to have a List<Int> in the beginning, that's ok, you just need to convert it to a Map<Int, Int> and compare the maps.
Using the nice Kotlin methods in the stdlib, we can make the code into this:
fun <T> containsAllSameCount(toCheck: List<T>, expected: List<T>): Boolean {
    val toCheckCountMap = toCheck.groupBy { it }.mapValues { it.value.size }
    val expectedCountMap = expected.groupBy { it }.mapValues { it.value.size }
    return expectedCountMap.all { toCheckCountMap[it.key] == it.value }
}

I have also added generics here because the same logic applies whether it's a List<Int>, List<String> and so on.
Some other notes:

Use JUnit for proper tests.
The main method in Kotlin does not need to be declared with the args, simply fun main() { will do.
Use val instead of var whenever possible.
Instead of .filter and then checking the size, use .count


Print the length of words as input as a histogram with horizontal bars Input

A list of words separated by any number of spaces.

Output

A horizontal ASCII art histogram, where the n-th line is composed by as many asterisks (*) as the n-th word is long.

Example usage

The > signals user input, you should not input it when testing the program.

> This is an example histogram of word length
****
**
**
*******
*********
**
****
******

> a aa aaa aaaa aaaaa
*
**
***
****
*****

> double space  example
******
*****
*******

The code

puts gets.chomp.split.map{|word| '*' * word.length}.join("\n")

Looks good. There's really not a lot to review. About the only thing I can think of is that I'd like a little breathing space in the block, i.e.

{ |word| "*" * word.length }

But that's about it :)

Of course, there are also other ways to go about this, for instance:

puts gets.chomp.gsub(/\S/, "*").gsub(/\s+/, "\n")

Not that that's necessarily any better.


Running SSH commands on multiple servers At the company I work at, we have no real way to quickly execute shell commands on all of our customers systems, which often results in what we call Tiny Tim tasks, where the list of all of our systems is split between the 5 devs we have, and we simply go through one-by-one, SSHing into each one and executing the necessary commands.

To streamline this process, I created a script in Python3 using Paramiko for SSH.
The script takes the path to a private OpenSSH key from command-line args, reads a list of hosts from a file called hosts.txt (lines in this file can be commented by prepending them with --), and a list of shell commands from a file called cmds.txt. Due to the way we set up our customers' systems, each domain has 2 servers, so the script loops over each host in the list twice, prepending disc1. and disc2. respectively to send the commands to both servers.

from paramiko.client import AutoAddPolicy
from paramiko.client import SSHClient
import sys
from sys import argv

if len(argv) < 2:
    print("Invalid args!")
    print("Usage: python3 run.py <private ssh key filepath>")
    exit()

with open('cmds.txt') as c:
    cmds = c.readlines()

with open('hosts.txt') as f:
    lines = f.readlines()

for line in lines:
    for i in range(1, 3):
        line = line.strip()
        host = "disc%s.%s" % (i, line)
        if line.startswith('--'):
            print("Host %s began with --, skipping" % line)
            continue
        client = SSHClient()
        client.set_missing_host_key_policy(AutoAddPolicy)
        client.load_system_host_keys()
        try:
            print("[%s]> Connecting as root on port 22..." % host)
            client.connect(host, 22, 'root', None, None, argv[1])
        except KeyboardInterrupt:
            break
        except Exception as err:
            print("[%s]> Connection failed: %s" % (host, err))
            continue
        for c in cmds:
            print("[%s]> Executing %s..." % (host, c.strip()))
            stdin, stdout, stderr = client.exec_command(c)
            print(stdout.read().decode('utf-8'))
            print(stderr.read().decode('utf-8'))
        client.close()
        print("[%s]> Done" % host)

I ran it through PEP8 and surprisingly didn't get any errors or warnings, but it's still quite messy and I'd appreciate feedback.

Looks like Fabric is what you need.

Fabric is a pretty simple deploy tool (comparing to Ansible/Puppet/etc), that lets you execute simple shell commands over SSH (Paramiko inside).

Features you will get from it:

You can specify SSH key path using command line:

fab command -i /path/to/key.pem

or parse it from command line:

import sys
from fabric.api import *
...
env.key_filename = sys.argv[2] or '/path/to/keyfile.pem'

Describe local/remote tasks in pythonic way:
    from fabric.api import *

def local_test():
    local('nosetests .')

def remote_pull():
    run('git pull')

And chain/group/do-anything-you want to them.
You can describe host list:

hosts = [<your_hosts_list>]
env.hosts = ['disc{}.{}'.format(host, index) for host in hosts for index in xrange(1,3)]

or you can describe roles:

env.roledefs['group1'] = ['host.name.com']
env.roledefs['group2'] = ['host1.name.com', 'host2.name.com']

and use this roles/hosts in your tasks configuration then:

@roles('group1')
@runs_once
def add_tag(version):
    run('git tag -f {}'.format(version))

And many more features out of the box!


Reading data from network stream I have a function to request an update from the server. I provide the query and also indicate the expected length of the response.

    public Byte[] GetUpdate(Byte[] query, int expLength)
    {
        var response = new Byte[expLength];

        lock(Client)
        {
            Stream s = Client.GetStream();
            s.Write(query, 0, query.Length);

            var totalBytesRead = 0;
            var numAttempts = 0;

            while(totalBytesRead < expLength && numAttempts < MAX_RETRIES)
            {
                numAttempts++;
                int bytes;
                try
                {
                    bytes = s.Read(response, totalBytesRead, expLength);
                }
                catch (Exception ex)
                {
                    throw new IOException();
                }
                totalBytesRead += bytes;
            }

            if(totalBytesRead < expLength)
            {
                // should probably throw something here
                throw new IOException();
            }
        }

        return response;
    }

I don't like the way I have written this code esp with two exception throwing.
Is there a more elegant way to write the code?

You should never throw an empty exception. Imagine someone uses your code and receives an empty IOException. What happened? How do you debug this?

First, you need a message that explains the situation. Second, you should use this constructor : IOException(String, Exception). This way, the exception you throw will contain the real exception, the one that causes problem. This will help debugging your code.

if(totalBytesRead < expLength)
{
    // should probably throw something here
    throw new IOException();
}

Yeah, you probably should throw something.. meaningful! Once again, add some information in there. Think about the poor soul that would need to debug this later.

I don't think it's a bad thing to throw exceptions in two separate places, but it's a bad thing to throw meaningless exceptions (I hope the message is clear ahah)


Finding the indices where a selected player appears in a list of hashes I have an array of hashes in a YAML file. When I run the code below it is VERY slow. My YAML pwsarr.count is about 2000.

I am somewhat new to Ruby so I don't know if my code is not efficient or if I am doing something wrong. I know it loops my array before it's in the method and it loops in the method, but I wouldn't think it would make it as slow as it is.

def pwsplayerindex(inplayer)
    #returns array of indicies of selected player
    #if one player is in array twice you will get 2 indicies #s
    #pwsarr my array of hashes

arr = []

    pwsarr.each_with_index do |val,index|
            if val['player'] == inplayer
                arr << index
            end
    end
arr
end

for j in 0..pwsarr.count
    puts pwsplayerindex(pwsarr[j]['player'])
end

my array of hashes is something like this:

pwsarr =[{'player'=>'first1 last1', 'stat'=>0.0},{'player'=>'first2 last2', 'stat'=>0.0}]

Your current code is calling an \$O(n)\$ function \$n\$ times. This leads to \$O(n^2)\$ time complexity on the whole. Instead I'd make a function to change pwsarr to a Hash, where the key is the player name, and the value is a list of 'player objects'. This can then cause your program to be \$O(n)\$ as hash lookup is \$O(1)\$.

The loop can then become: (I don't know Ruby.)

pwsarr = [
    {'player'=>'first1 last1', 'stat'=>0.0},
    {'player'=>'first1 last1', 'stat'=>2.0},
    {'player'=>'first2 last2', 'stat'=>0.0}
]

def group_by(list_hash, key)
    h = Hash.new { |hash,key| hash[key] = [] }

    for hash in list_hash do
        h[hash[key]].push(hash)
    end
    h
end

pws_by_player = group_by(pwsarr, 'player')
for player in pwsarr do
    puts pws_by_player[player['player']]
end


Proper capitalization for surname like McDonald I am using the code below to capitalize a string properly. I would like to know if there is a better way to code the surname portion, as the way it stands now can become very bloated depending on how many surnames I plan on adding.

This is a follow up to: Capitalize string except for conjunction words

Dim conjunctions
Dim suffixes
Dim surnames
Set conjunctions = CreateObject("System.Collections.Arraylist")
Set suffixes = CreateObject("System.Collections.Arraylist")
Set surnames = CreateObject("System.Collections.Arraylist")

conjunctions.Add("a")
conjunctions.Add("an")
conjunctions.Add("and")
conjunctions.Add("as")
conjunctions.Add("at")
' etc....

suffixes.Add("ii")
suffixes.Add("iii")
suffixes.Add("iv")
suffixes.Add("vi")
suffixes.Add("vii")
suffixes.Add("viii")
suffixes.Add("ix")

surnames.Add("mcdonald")

for each item in xTextSplit
    xWord = item
    if conjunctions.contains(lcase(item)) then
        xWord = lcase(item)
    elseif suffixes.contains(lcase(item)) then
        xWord = ucase(item)
    elseif surnames.contains(lcase(item)) then
        xWord = "Mc" & UCase(Mid(item, 3, 1)) & Mid(item, 4)
    end if
    xCompleteWord = xCompleteWord & " " & xWord
next

queryForHeading = xCompleteWord
queryForHeading = Mid(queryForHeading, 2)
queryForHeading = UCase(Mid(queryForHeading, 1, 1)) & Mid(queryForHeading, 2)

If you are going to create a list of the last names it will get very big, but you would have a list of the names so you could instead

surnames.Add("McDonald")

And then

else
    foreach name in surnames
        if lcase(item) = lcase(name) then
            xWord = name
        end if
    next
end if

But, this would get rather long as you add the names in, which is a big downfall when talking about maintenance especially on a VBScript.


Accessing a UIView's parent UIViewController using the UIResponder chain I recently found a static C function on Stack Overflow which returns a UIViewController reference from a containing subview using the responder chain:

static UIViewController *viewControllerForView(UIView *view) {
    UIResponder *responder = view;
    do {
        responder = [responder nextResponder];
    }
    while (responder && ![responder isKindOfClass:[UIViewController class]]);
    return (UIViewController *)responder;
}

I wanted to write this in Swift 3 as a programming exercise.  My first pass looked like this:

func viewController(forView view: UIView) -> UIViewController? {

    var responder = view as? UIResponder

    while (responder != nil) {
        if let viewController = responder as? UIViewController {
            return viewController
        }
        responder = responder?.next
    }

    return nil
}

Then I tried using a repeat while loop, to see if the structure would look any better:

func viewController(forView view: UIView) -> UIViewController? {

    var responder: UIResponder? = view

    repeat {
        responder = responder?.next
        if let vc = responder as? UIViewController {
            return vc
        }
    } while responder != nil

    return nil
}

I'm really not a fan of the while responder != nil.  Is there a way I can loop while checking if the responder is a UIViewController?  Maybe I can make this a recursive function by making the parameter a generic UIResponder object.

Your code is correct and there is nothing wrong with checking if an optional is nil unless you actually need the value if it is not.

Here is an example where the comparison to nil is inappropriate:

if someOptional != nil {
 doSomethingWith(someOptional!)
}

This of course should be written as

if let something = someOptional {
  doSomethingWith(something)
}

But in your case there is nothing wrong with the check.

A recursive version would also require the check for nil, but it could be written using an if let statement. But this version would have to take a responder as parameter:

func viewController(responder: UIResponder) -> UIViewController? {
    if let vc = responder as? UIViewController {
        return vc
    }

    if let next = responder.next {
        return viewController(responder: next)
    }

    return nil
}

A more elegant way to express this would be a calculated property in an extension on UIResponder:

extension UIResponder {
    var viewController: UIViewController? {
        if let vc = self as? UIViewController {
            return vc
        }

        return next?.viewController
    }
}

Here the explicit check for nil can be eliminated thanks to optional chaining.

I would prefer your repeat/while solution though. But in the end it is a matter of taste.

The performance of the recursive version will be worse unless the compiler can do tail call elimination there. I am not sure if it will be able to do so. But this should hardly matter. If you get to a situation where this is a bottleneck you have worse problems.


Huffman encoding successive-merge function From SICP:

  Exercise 2.69.  The following
  procedure takes as its argument a list
  of symbol-frequency pairs (where no
  symbol appears in more than one pair)
  and generates a Huffman encoding tree
  according to the Huffman algorithm.

(define (generate-huffman-tree pairs)
    (successive-merge (make-leaf-set pairs)))

  Make-leaf-set is the procedure given
  above that transforms the list of
  pairs into an ordered set of leaves.
  Successive-merge is the procedure you
  must write, using make-code-tree to
  successively merge the smallest-weight
  elements of the set until there is
  only one element left, which is the
  desired Huffman tree. (This procedure
  is slightly tricky, but not really
  complicated. If you find yourself
  designing a complex procedure, then
  you are almost certainly doing
  something wrong. You can take
  significant advantage of the fact that
  we are using an ordered set
  representation.)

I wrote the following solution:

(define (generate-huffman-tree pairs)
  (successive-merge (make-leaf-set pairs)))

(define (successive-merge leaf-set)
  (define (iter result leaf-subset)
    (if (null? leaf-subset)
        result
        (let ((first-leaf (car leaf-subset)))
          (iter (make-code-tree first-leaf result) (cdr leaf-subset)))))
  (iter (make-code-tree (car leaf-set) (cadr leaf-set)) (cddr leaf-set)))

Is this a good answer?  Can it be improved?

It's been a while since I've done any Lisp/Scheme, but I'll make one style point and one algorithmic point.

Style point: it's usually more readable if you define structure projection functions with meaningful names rather than using car, cdr, etc.

Algorithmic point: the simplest way of doing this is to include some priority queue structure from the standard library.  Then you simply remove the two least items from the queue, create their combined Huffman tree, and reinsert them into the queue.  You're done when the queue contains only one item.


DAO to create, update, and delete a project I'm trying to write my first "complex" program in Java. So far, I created a local H2 database but I'm not sure about the quality of my code (I'm newbie in Java) because I don't know how I can deal with exceptions and I don't feel comfortable with the JDBC code.

 public boolean create(Project project)  {
 String id = UUID.randomUUID().toString().toUpperCase().replace("-", "");
 project.setId(id);

    try {
        Class.forName("org.h2.Driver");
        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO PROJECT VALUES(?, ?,?)");

        preparedStatement.setString(1, id);
        preparedStatement.setString(2, project.getName());
        preparedStatement.setString(3, project.getTrigram());
        int resultSet = preparedStatement.executeUpdate();

        if(resultSet!=0)
        {
            System.out.println("Project created");
            connection.close();
            return true;
        }

        else {
            System.out.println("No project was  created ");
            connection.close();
            return false;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }
    return false;

}

@Override
public boolean update(Project project){

    try {
        Class.forName("org.h2.Driver");

        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("UPDATE PROJECT SET NAME=?, TRIGRAM = ? WHERE ID=?");

        preparedStatement.setString(1, project.getName());
        preparedStatement.setString(2, project.getTrigram());
        preparedStatement.setString(3, project.getId());
        int resultSet = preparedStatement.executeUpdate();

        if(resultSet==0) {
            System.out.println("The project was not updated ");
            connection.close();
            return false;
        }
        else {
            System.out.println("Project updated" + '\n'+  project.toString());
            connection.close();
            return true;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }
    return false;
}

@Override
public boolean delete(String id) {

    try {
        Class.forName("org.h2.Driver");
        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("DELETE FROM PROJECT WHERE ID=?");
        preparedStatement.setString(1,id);
        int result = preparedStatement.executeUpdate();

        if(result!=0) {

            PreparedStatement preparedStatement2 = connection.prepareStatement("DELETE FROM MILESTONE WHERE FK_PROJECT_ID =?");
            preparedStatement2.setString(1,id);
            preparedStatement2.executeUpdate();

            PreparedStatement preparedStatement3 = connection.prepareStatement("DELETE FROM REQUIREMENT WHERE FK_PROJECT_ID =?");
            preparedStatement3.setString(1,id);
            preparedStatement3.executeUpdate();

            System.out.println("Project deleted with id = " + id);
            connection.close();
            return true;

        }

        else {
            System.out.println("No project was deleted with id = " + id);
            connection.close();
            return false;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }
    return false;
}

As you can see, the management of the exception here is very very poor. Can you help me to optimize this? Should I use finally or something like that?
I feel that there a big redundancy in my code since that I'm obliged to write many lines to create a JDBC connection and execute a SQL query. I have 6 tables, so the quality is really poor. I don't know how can I optimize this because the query changes in each method.

Nitpick

 public boolean create(Project project)  {
 String id = UUID.randomUUID().toString().toUpperCase().replace("-", "");
 project.setId(id);

The last two lines should be indented relative to the method declaration.  Perhaps this was just a copy/paste error that doesn't exist in the original code.

try

    try {
        Class.forName("org.h2.Driver");
        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO PROJECT VALUES(?, ?,?)");

        preparedStatement.setString(1, id);
        preparedStatement.setString(2, project.getName());
        preparedStatement.setString(3, project.getTrigram());
        int resultSet = preparedStatement.executeUpdate();

        if(resultSet!=0)
        {
            System.out.println("Project created");
            connection.close();
            return true;
        }

        else {
            System.out.println("No project was  created ");
            connection.close();
            return false;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }

This is a really long try block, and most of the code doesn't seem to need it.  Also, it doesn't make use of the try with resources, even though it seems to exactly cover the use case.  Instead consider breaking it into two smaller try blocks.

    try {
        Class.forName("org.h2.Driver");
    } catch (ClassNotFoundException e) {
        e.printStackTrace();
        return false;
    }

All this code does is verify that the class exists.  I would actually do this outside a try block, as it seems like a fatal error.  But this version at least isolates the class problem from the SQLException issue, as they have different responses.

    int resultSet = 0;

Technically, the 0 is unnecessary here, as it is already the default.  I find it clearer to initialize it explicitly.  Others disagree.

Removing from the try block will allow it to be used outside the block later.

    try (Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "")) {
        PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO PROJECT VALUES(?, ?,?)");

        preparedStatement.setString(1, id);
        preparedStatement.setString(2, project.getName());
        preparedStatement.setString(3, project.getTrigram());

        resultSet = preparedStatement.executeUpdate();
    } catch (SQLException e) {
        e.printStackTrace();
    }

Now you don't have to worry about explicitly closing the connection.  The try with resources will handle that automatically no matter how or when you leave the try block.

Note that in the original code, an exception after the connection was made but before the connection was explicitly closed would bypass closing the connection.  You could have used a finally block to work around this, but the try with resources is simpler and more reliable.

    if (resultSet != 0) {
        System.out.println("Project created");
        return true;
    }

    System.out.println("No project was created");
    return false;

This block of code cannot generate either of the caught exceptions.  As such, it does not need to be in the try block.  So go ahead and take it out.

Consistent formatting

There are three common ways to handle a {} block.  You use all three.  Please pick one and stick to it consistently.  Consistent formatting makes it easier to both read and modify the code.

Start and end on the same line:

    } else {

Start and end on separate lines:

    }
    else
    {

Closing } alone:

    }
    else {

Also, note that the second and third versions allow you to put extra blank lines between the braces and the else.  Please don't do that.  When you do that, it obscures that the previous block has an else.  It's much clearer if you treat the braces and the else as one symbol and always write them the same way.  Then the reader can immediately see which block ends and which will continue later.

Personally, I prefer the first form, but all three are in common use.  The Java coding standard also prefers the first form, but some prefer one of the others.  Consistent use of any of them within your code will allow most programmers to adapt.


STL queue implementation I've implemented a simple C++ STL like queue. (I've tried to follow @LokiAstari stack implementation code fashion)

#ifndef QUEUE_H
#define QUEUE_H

#include <iostream>
#include <stdexcept>

using std::cout;

template <typename T>
class queue {
public:
    struct node {
        T data;
        node *next;

        node()
            : data(0)
            , next(nullptr) {
        }

        node(T const& data, node* next)
            : data(data)
            , next(next) {
        }

        node(T&& data, node* next)
            : data(std::move(data))
            , next(next) {
        }
    };
    ~queue();
    bool empty() const;
    size_t size() const;
    T front() const;
    void push(T const& data);
    void push(T&& data);
    //void emplace (T&&... args);
    // void swap (queue& x);
    void pop();

private:
    size_t elements = 0;
    node *head = nullptr;
    node *tail = nullptr;
};

template <typename T>
queue<T>::~queue() {
    node *curr = new node();
    while(head) {
        curr = head;
        head = head->next;
        delete curr;
    }
    delete tail;
}

template <typename T>
bool queue<T>::empty() const {
    return elements == 0;
//    return head == nullptr;
}

template <typename T>
size_t queue<T>::size() const {
    return elements;
}

template <typename T>
T queue<T>::front() const {
    if(elements == 0)
        throw std::runtime_error("Invalid Action");
    return head->data;
}

template <typename T>
void  queue<T>::push(T const& data) {
    node *newNode = new node(data, nullptr);
    if(!elements) head = newNode;
    else tail->next = newNode;
    tail = newNode;
    ++elements;
}
template <typename T>
void  queue<T>::push(T&& data) {
    node *newNode = new node(std::move(data), nullptr);
    if(!elements) head = newNode;
    else tail->next = newNode;
    tail = newNode;
    ++elements;
}
template <typename T>
void  queue<T>::pop() {
    if(elements == 0)
        throw std::runtime_error("Invalid Action");
    node *tmp = new node();
    if(elements != 0) tmp = head;
    head = head->next;
    --elements;
    delete tmp;
}

#endif // QUEUE_H

I would appreciate all criticism relevant to code, style, flow, camelCase vs underscore, and so forth.

A few points that nearly jump out at me:

You've implemented it as a linked list of individually allocated nodes. This nearly guarantees poor locality of reference and for small types wastes quite a bit of space.
Your destructor leaks memory:

template <typename T>
queue<T>::~queue() {
    node *curr = new node();
    while(head) {
        curr = head;

This allocates a node (not clear why you'd want to allocate a node in the dtor, but there it is), then if head != nullptr, it overwrites that pointer with head, thus leaking the node it just allocated (and if that condition isn't met, it just flows off the end without doing any more with curr, also leaking the memory).
Your node type takes for granted that a T can be initialized from 0:

node()
    : data(0)

For a generic type, this assumption is unwarranted. You typically want to use T() to create a value-initialized object, which will be 0 for arithmetic types, a null pointer for a pointer type, an empty string for a string type, and so on.


Report on user-input chacters by their type I'm a first year student at a college that focuses heavily on self learning (SCRUM) without any classical lessons. Because of this, I basically learned everything I know from websites like this. I don't want to get any bad habits or wrong understandings. So, is this code any good? I'm not looking for optimization (wouldn't mind any tips though ;) ) since I'll be learning that through time, but rather if the general structure and my way of thinking is right.

The purpose of my code is to ask the user for a string of characters of his liking (for example a sentence) and displays the amount of vowels, consonants, numbers and other symbols it contains.

static void Main(string[] args)
{
    string repeat;
    //do while loop for if the user wants to run the program again
    do
    {
        //asigns variables
        string text;
        int vowels, consonants, numbers, otherSymbols;
        var hsVowels = new HashSet<char> { 'a', 'e', 'i', 'o', 'u' };
        var hsConsonants = new HashSet<char> { 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z'};
        var hsNumbers = new HashSet<char> { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

        //asks for input
        Console.WriteLine("Input anything and the program wil tell you how many vowels, consonants, numbers and other symbols you gave.");
        text = Console.ReadLine().ToLower();

        //calculates
        vowels = text.Count(c => hsVowels.Contains(c));
        consonants = text.Count(c => hsConsonants.Contains(c));
        numbers = text.Count(c => hsNumbers.Contains(c));
        otherSymbols = text.Length - (vowels + consonants + numbers);

        //shows the result
        Console.WriteLine("Your input has {0} vowels, {1} consonants, {2} numbers and {3} other Symbols.", vowels, consonants, numbers, otherSymbols);

        //asks if the user wants to run the program again
        Console.WriteLine("Would you like to try again? (yes/no)");
        repeat = Console.ReadLine();
        //tests if the users input was valid (yes/no)
        while (!(repeat.ToLower().Contains("yes") || repeat.ToLower().Contains("no")))
        {
            Console.WriteLine(@"Invalid input. Please answer ""yes"" or ""no"" .");
            repeat = Console.ReadLine();
        }
    } while (repeat.ToLower().Contains("yes"));
}

Reposted from Stack Overflow to Stack Exchange as recommended by sellotape

Answering your main question:

  So, is this code any good?

Your code is good in the sense that it works as expected in the context of the English alphabet. That's a good start and for a small program like this we could live with that.

But there are a couple of things to be done better.

    Console.WriteLine("Input anything and the program wil tell you how many vowels, consonants, numbers and other symbols you gave.");
    text = Console.ReadLine().ToLower();

Your prompts are a little too descriptive. Make them shorter and more "demanding" like:

Console.Write("Enter the text: ");
text = Console.ReadLine().ToLower();

Notice the use of Console.Write(...) instead of Console.WriteLine(...). The effect is, that the user enter the text on the same line after the prompt, which is common practice in the console.

If you want to inform the user of the meaning by the program, then provide a explanation once in the beginning - before to loop.

  var hsVowels = new HashSet<char> { 'a', 'e', 'i', 'o', 'u' };
  var hsConsonants = new HashSet<char> { 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z' };
  var hsNumbers = new HashSet<char> { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' };

These variables are constant and do not change per loop, so no need to instantiate them in every loop. Instead place them in the beginning of the method:

  string repeat;

  var hsVowels = new HashSet<char> { 'a', 'e', 'i', 'o', 'u' };
  var hsConsonants = new HashSet<char> { 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z' };
  var hsNumbers = new HashSet<char> { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' };

  do
  {
    //asks for input

    int vowels, consonants, numbers, otherSymbols;

Declaring variables like this is allowed, but considered bad practice. Instead use one line per variable:

    int vowels = 0;
    int consonants = 0;
    int numbers = 0;
    int otherSymbols = 0;

It's easier to find them and hence understand the code - especially for reviewers.

    //calculates
    vowels = text.Count(c => hsVowels.Contains(c));
    consonants = text.Count(c => hsConsonants.Contains(c));
    numbers = text.Count(c => hsNumbers.Contains(c));
    otherSymbols = text.Length - (vowels + consonants + numbers);

Here you use the LINQ extension Count() which is usable for many things, but here it is rather inefficient in that calling text.Count(...) for every char type will cause the text to be parsed three times. For a small text it will probably be OK, but for larger texts it will slow down the process significantly. Instead a single foreach-loop could do the same in just one iteration:

    foreach (char ch in text)
    {
      if (hsVowels.Contains(ch))
        vowels++;
      else if (hsConsonants.Contains(ch))
        consonants++;
      else if (hsNumbers.Contains(ch))
        numbers++;
      else
        otherSymbols++;
    }

It looks maybe not so smart and modern but is a lot more efficient.

    Console.WriteLine("Your input has {0} vowels, {1} consonants, {2} numbers and {3} other Symbols.", vowels, consonants, numbers, otherSymbols);

Printing the result on one line is not very readable. A tabular form will be more clear:

    Console.WriteLine("Result: ");
    Console.WriteLine($"Original Text: {text}");
    Console.WriteLine($"Vowels:        {vowels,15}");
    Console.WriteLine($"Consonants:    {consonants,15}");
    Console.WriteLine($"Numbers:       {numbers,15}");
    Console.WriteLine($"OtherSymbols:  {otherSymbols,15}");

    Console.WriteLine("Would you like to try again? (yes/no)");
    repeat = Console.ReadLine();
    //tests if the users input was valid (yes/no)
    while (!(repeat.ToLower().Contains("yes") || repeat.ToLower().Contains("no")))
    {

Here, the user is forced to type "yes" or "no". "y" or "n" would do the trick:

Console.Write("Would you like to try again? (Y/N): ");

you use repeat.ToLower() every time you use repeat so why not lower it once:

repeat = Console.ReadLine()?.ToLower();

When it comes to the overall structure you should think about splitting the code into meaningful methods. The program has three major parts:

User input
The calculation
Display of the result

and because you allow the user to enter more than one text it must run in a loop like:

  string textInput;

  while ((textInput = GetUserInput()) != null)
  {
    TextInfo info = CountElements(textInput);
    PrintTextInfo(textInput, info);
  }

This is easy to understand, and each functionality of the program can be written and maintained separately:

private string GetUserInput()
{
  Console.Clear();
  Console.Write("Enter some Text [To exit press: Enter]: ");
  string text = Console.ReadLine();
  if (string.IsNullOrWhiteSpace(text))
    return null;
  return text;
}

private void PrintTextInfo(string textInput, TextInfo info)
{
  Console.WriteLine("Result: ");
  Console.WriteLine($"Original Text: {textInput}");
  Console.WriteLine($"Vowels:        {info.Vowels,15}");
  Console.WriteLine($"Consonants:    {info.Consonants,15}");
  Console.WriteLine($"Numbers:       {info.Numbers,15}");
  Console.WriteLine($"OtherSymbols:  {info.OtherSymbols,15}");
  Console.WriteLine("Press Enter to Continue...");
  Console.ReadLine();
}

For the TextInfo info = CountElements(textInput); you could write a text analyzer class that could be instantiated like:

  CharTypeAnalyzer analyzer = new CharTypeAnalyzer("aeiouAEIOU", "bcdfghijklmnpqrstvwxyzBCDFGHIJKLMNPQRSTVWXYZ");

with the definition of vowels and consonants as string arguments. This will make it more reusable for other languages than English (the use is of course limited to languages where the terms "vowel" and "consonant" are meaningful).

Using this class will change the main method to:

static void Main()
{
  CharTypeAnalyzer analyzer = new CharTypeAnalyzer("aeiouAEIOU", "bcdfghijklmnpqrstvwxyzBCDFGHIJKLMNPQRSTVWXYZ");

  string textInput;

  while ((textInput = GetUserInput()) != null)
  {
    TextInfo info = analyzer.Analyze(textInput);
    PrintTextInfo(textInput, info);
  }
}

where TextInfo is defined as:

public struct TextInfo
{
  public int Vowels;
  public int Consonants;
  public int Numbers;
  public int OtherSymbols;
}

and CharTypeAnalyzer as:

public class CharTypeAnalyzer
{
  string _vowels;
  string _consonants;

  public CharTypeAnalyzer(string vowels, string consonants)
  {
    _vowels = vowels;
    _consonants = consonants;
  }

  public TextInfo Analyze(string textInput)
  {
    TextInfo info = new TextInfo();

    foreach (char ch in textInput)
    {
      if (IsVowel(ch))
        info.Vowels++;
      else if (IsConsonant(ch))
        info.Consonants++;
      else if (IsDigit(ch))
        info.Numbers++;
      else
        info.OtherSymbols++;
    }

    return info;
  }

  private bool IsVowel(char ch)
  {
    return _vowels.Contains(ch);
  }

  private bool IsConsonant(char ch)
  {
    return _consonants.Contains(ch);
  }

  private bool IsDigit(char ch)
  {
    return char.IsDigit(ch);
  }
}

In this way the code is easy to review and maintain, and you can easily debug and test each part separately.

It can be done in many other ways, but the essential messages are:

Keep things simple
Split the code into meaningful parts - methods and classes


To-do app, as one monster function and as several smaller functions I am learning to code by making a todo app in pure javascript. My first attempt can be seen here.

It's all cosmetic, meaning it stores no information, generates no objects or arrays, though it was sure fun to learn how to manipulate DOM elements in pure JavaScript and building my first "program"!

I am rebuilding it in a more OOP manner. In this first jsfiddle:

when(toDo, "keypress", function(event){
  if (event.key == "Enter" || event.keyCode == 13) {
    pushArray();
    toDo.value = "";
  }
});

function pushArray(){
  var nt = new Task(toDo.value, "No note yet");
  fruits.push(nt);
  var task = document.createElement("div");
  task.setAttribute("class", "taskitem");
  task.innerHTML = nt.Name;
  var t = document.createElement("input");
  t.setAttribute("class", "tasknote");
  task.appendChild(t);
  when(t, "keypress", function(event){
  if (event.key == "Enter" || event.keyCode == 13) {
    nt.Note = t.value;
    this.parentNode.innerHTML = nt.Name + " " + nt.Note;
    t.value = "";
  }
});

  result.appendChild(task);
  console.log(fruits);
}

Hitting enter fires a function that does everything. creates a new task object, pushes it to the array, displays the task and appends a note that can be updated anytime, for example -  task:get milk note:almond milk. However I eventually want to rebuild this app again using local storage, and again with ajax and JSON. I'm thinking I'm going to have a hard time injecting that functionality into this single function without breaking it. So I did it again and broke the steps down into several smaller functions that 'plug' into each other:  https://jsfiddle.net/8wm0r345/

  function Task(name, note) {
  this.Note = note;
  this.Name = name;
  this.completed = false;
}

function pushArray() {
  var nt = new Task(toDo.value, "No note yet");
  taskArray.push(nt);
  displayArray(result, nt.Name);
  appendNote(result, nt);
}

function displayArray(x, obj) {
  var task = make("div", "class", "taskitem", obj);
  x.appendChild(task);
}

function appendNote(el, obj) {
  var t = make("input", "class", "tasknote");
  el.appendChild(t);
  when(t, "keypress", submitNote.bind(null, obj, t));
}

function submitNote(newTask, theInput) {
  if (event.key == "Enter" || event.keyCode == 13) {
    newTask.Note = theInput.value;
    var n = make("div", "class", "hasNote", newTask.Note);
    var t = theInput;
    t.parentNode.insertBefore(n, t.nextSibling);
    t.parentNode.removeChild(t);
    when(n, "dblclick", function() {
      n.parentNode.insertBefore(t, n.nextSibling);
      n.parentNode.removeChild(n);
    });
  }
}

My question is: What is the recommended approach to building a small program like this? And are there any alarming issues in either code presented here?

Please don't use nt and t as variable names. If you were using them in a small limited context then it wouldn't be much of an issue, but repeatedly using them across multiple lines is going to get confusing. What's wrong with newTask and task?

In the case of var nt = new Task(toDo.value, "No note yet"); it's pretty clear what nt is there, but I'm not even sure if t is a task, or a note on the task, or user input. A clear name makes code more readable, and leaves people guessing at your intentions less.

x in displayArray is even worse. Though it is a limited context, you make it seem as though it accepts arbitrary input (x doesn't specify anything) when it clearly needs something that can have a child appended.

That said I personally like the split up functions a lot more. With individual functions, it's easier to read them and see what the functions do. It's easy for steps to get lost in a big megafunction, but when each step is it's own function you can make everything more explanatory.

However the readability principle applies again. I'm not very familiar with JS, so maybe displayArray makes sense for that, but to me I can't see why a display function is appending something to the HTML. Surely it should be another append function?


Regex validation for Email Address I need to validate whether my regex is correct for below scenario. Suggestion's if the regex is correct:

Wiki Link Local_part

The local-part of the email address may use any of these ASCII characters.[4] RFC 6531 permits Unicode characters beyond the ASCII range:

Uppercase and lowercase English letters (a–z, A–Z) (ASCII: 65–90,
97–122)
Digits 0 to 9 (ASCII: 48–57)
These special characters: ! # $ % & ' * + - / = ? ^ _ { | } ~
(limited support)
Character . (dot, period, full stop) (ASCII: 46) provided that it is
not the first or last character, and provided also that it does not
appear two or more times consecutively (e.g. John..Doe@example.com
is not allowed).
Special characters are allowed with restrictions. They are:
Comments are allowed with parentheses at either end of the local
part; e.g. "john.smith(comment)@example.com" and
"(comment)john.smith@example.com" are both equivalent to
"john.smith@example.com".
International characters above U+007F are permitted by RFC 6531,
though mail systems may restrict which characters to use when
assigning local parts.

The Regex:

^[a-z0-9][-a-z0-9.!#$%&'*+-=?^_`{|}~\/]+@([-a-z0-9]+\.)+[a-z]{2,5}$

Demo Here

I remember having read somewhere (possibly in another Code Review answer) that for an e-mail address, the simplest and most effective validation you can do is to make sure it contains an @. Making it more restrictive than that can often be a risk of invalidating some valid e-mails. You'd be surprised at some examples of valid e-mail addresses.

As an additional example, see the "almost RFC 822 compatible regex" in this answer.

Keep it simple, and don't mark some e-mail addresses that are actually valid as invalid.

email.contains("@")

If you want to be more restrictive than this, use an existing and trusted library for the validation, don't try to make another regex.

For further reading:

http://en.wikipedia.org/wiki/Email_address#Validation_and_verification
list of email addresses that can be used to test a javascript validation script on Stack Overflow
http://codefool.tumblr.com/post/15288874550/list-of-valid-and-invalid-email-addresses


Extracting an integer command-line argument and printing a sum of numbers up to that limit The below code works fine but I have the feeling that there must be something more idiomatic way to do this within the do-notation without having to resort to the two liftMs?

main = do
  n <- (liftM read (liftM head getArgs)) :: IO Int
  putStrLN . show $ sum [1..n]

If I do:

main = do
  args <- getArgs
  arg1 <- head args
  n <- read arg1 :: IO Int
  putStrLn . show $ sum [1..n]

I get:

Expected type: String
  Actual type: Char
In the first Argument of 'read', namely 'arg1'

which I do not understand. <- getArgs returns IO [String], so <- head args should return IO String, but it seems to return IO Char?! (as per error message).

So my two questions are:

what is the most idiomatic way do do this
what's wrong in the second version?

This is the code you're looking for:

main = do
  args <- getArgs
  let arg1 = head args
  let n = (read arg1) :: Int
  putStrLn . show $ sum [1..n]

Think of the <- operator as "extract". It "pulls" a value out of a monad.

But once you've pulled args out of the IO monad, you've now got a pure list of strings. It's not in the IO monad, so you can't extract from it. let instead just does a pure binding like you're used to in non-monadic code, which is what you need here.


Edit string to concatenate word numbers in javascript I would like to replace the string "The quick brown fox jumps over the lazy dog" with the string "The1 quick2 brown3 fox4 jumps5 over6 the7 lazy8 dog9".

Is there an cleaner, more elegant, sexier way?

String.prototype.concatWordNumber = function() {

   var myArray = this.split(' ');
   var myString = "";
   for (var i=0, len = myArray.length; i<len; i++)
   {
        myString += myArray[i]+[i+1]+ " " ;
   }
  return myString;
}

var text = "The quick brown fox jumps over the lazy dog";

console.log(text.concatWordNumber());

Is there an cleaner, more elegant, sexier way?

I guess regex makes it more elegant and sexier, but your implementation is fine:

String.prototype.concatWordNumber = function() {
  var i = 1;
  return this.replace(/\b\w+\b/g, function(word) {
    return word + i++;
  });
};

Or you could use map:

String.prototype.concatWordNumber = function() {
  var plusIdx = function(x,i) {
    return x + ++i;
  };
  return this.split(' ').map(plusIdx).join(' ');
};


Square spiral matrix I've written a Python script to generate the closest square spiral matrix to a given number.

I'm new to code reviews as a practice, and I'm interested in ways to improve. Please suggest improvements to the code where you see fit, particularly in regards to:

Algorithm: Is there a faster/more elegant way to generate the matrix?
Style: I've tried to adhere to PEP8.

"""
Outputs the closest square spiral matrix of an input number
"""
from math import sqrt

def is_even_square(num):
    """True if an integer is even as well as a perfect square"""
    return sqrt(num).is_integer() and (num % 2 == 0)

def find_nearest_square(num):
    """Returns the nearest even perfect square to a given integer"""
    for i in range(num):
        if is_even_square(num - i):
            return num - i
        elif is_even_square(num + i):
            return num + i

def find_lower_squares(num):
    """Returns a list of even perfect squares less than a given integer"""
    squares = []
    for i in range(num, 3, -1):
        if is_even_square(i): squares.append(i)
    return squares

def nth_row(num, n):
    """Returns the nth row of the square spiral matrix"""
    edge = int(sqrt(num))
    squares = find_lower_squares(num)
    if n == 0:
        return list(range(num, num - edge, -1))
    elif n >= edge - 1:
        return list(range(num - 3*edge + 3, num - 2*edge + 3))
    elif n < edge // 2:
        return ([squares[1] + n] + nth_row(squares[1],n-1)
              + [num - edge - n + 1])
    else:
        return ([num - 3*edge + 4 + n - edge] + nth_row(squares[1],n-1)
              + [num - 2*edge + 1 - n + edge])

def generate_square_spiral(num):
    """Generates a square spiral matrix from a given integer"""
    edge = int(sqrt(num))
    square_spiral = [[None for x in range(edge)] for y in range(edge)]
    for row in range(edge): square_spiral[row] = nth_row(num, row)
    return square_spiral

def main ():
    num = None
    while not num:
        try:
            num = int(input('Input number: '))
        except ValueError:
            print('Invalid Number')
    nearest_square = find_nearest_square(num)
    matrix = generate_square_spiral(nearest_square)
    for row in range(len(matrix[0])):
        for col in range(len(matrix)):
            if matrix[row][col] < 10:
                print('  ',matrix[row][col],' ',sep='',end='')
            elif matrix[row][col] < 100:
                print(' ',matrix[row][col],' ',sep='',end='')
            else:
                print(matrix[row][col],' ',sep='',end='')
        print(2*"\n",end='')

if __name__ == '__main__':
    main()

Your code is overall clean and well written.

You should use list comprehension:

def find_lower_squares(num):
    """Returns a list of even perfect squares less than a given integer"""
    squares = []
    for i in range(num, 3, -1):
        if is_even_square(i): squares.append(i)
    return squares

Should become:

def find_lower_squares(num):
    """Returns a list of even perfect squares less than a given integer"""
    return [i for i in range(num, 3, -1) if is_even_square(i)]

Scientific/mathematical code like this greatly benefits from automated testing, let me show you an example:

import doctest

def find_lower_squares(num):
    """
    Returns a list of even perfect squares less than a given integer

    >>> find_lower_squares(40)
    [36, 16, 4]
    """
    return [i for i in range(num, 3, -1) if is_even_square(i)]

doctest.testmod()

This gives double benefits:

Changes to the code can be made without being worried: if you break something, the error message will show up immediately
Code readability and documentation increase a lot, the reader can see some examples of the function being used and will understand it faster and better.

Avoid None-important assignment

In mid level languages such as C you must declare array first as empty and then fill them. In high level languages such as Python you should avoid such low level behaviour and insert the values in the arrays directly:

def generate_square_spiral(num):
    """
    Generates a square spiral matrix from a given integer

    >>> generate_square_spiral(5)
    [[5, 4], [2, 3]]
    >>> generate_square_spiral(17)
    [[17, 16, 15, 14], [5, 4, 3, 13], [7, 1, 2, 12], [8, 9, 10, 11]]
    """
    edge = int(sqrt(num))
    square_spiral = [nth_row(num, row) for row in range(edge)]
    return square_spiral

Also we could remove the unnecessary assignment:

edge = int(sqrt(num))
return [nth_row(num, row) for row in range(edge)]


Which way should I use while writing this method? I've got a homework that is about methods. So first part is

  Write a method that prompts the customer for the car type. There are
  three acceptable car types (E: Economy, M: Midsize, F: Fullsize). Your
  method will return a valid car type.

But the thing that I don't understand is, should I read input from the user first and then make the method give a valid car type, or should method also read from user?

char type;
System.out.println("(E) Economy  - 50 TL");
System.out.println("(M) Midsize  - 70 TL");
System.out.println("(F) Fullsize - 100 TL");
do{
    System.out.println("Enter the car type (E/M/F) : ");
    type = input.next().charAt(0);
    type = Character.toUpperCase(type);
} while (type != 'E' && type != 'M' && type != 'F' );
carType(type);

and;

public static String carType(char y){

    String x ="";
    if(y == 'E')
        x = "Economy";
    if(y == 'M')
        x = "Midsize";
    if(y == 'F')
        x = "Full size";
    return x;

Do you think this is the right way to do it? Because it says "Write a method that prompts the user..." but what's the point of using method if I move all of the code above into method?

From the way you describe the problem, it is clear that there only needs to be one method. That method is supposed to: "... prompts the customer for the car type. ... Your method will return a valid car type".

What you have presented here, is the guts of some code which prompts the user for a character value, and then another method which converts the char to a String type.

From what I can tell (and filling in some blanks) your code looks functional, but I am not certain it satisfies the requirements (for just one method).

I would start with the 'specification' for the method, and use that as a template for the code. I will make the assumption (as you have) that the method should return the 'full' name of the car type (i.e. return "Economy" instead of 'E').

The good things in your code are the use of the do {...} while(...) block. You have a good loop there, with good conditional exit criteria.

You have not shown us what input is so I will assume it is a Scanner on System.in.

If we wrap the code you have there with a method that makes sense, it would be something like:

public String promptForCarType(Scanner input) {
    .....
    return carType;
}

That would be a method that prompts the user for a car type, and returns the user's selection.

The code inside that is what you have written above. There are a few things you should consider though:

You should probably get the full input from the user (not just the first character), and make sure the user only entered a single character (data validation).
Do you know what a 'switch' statement is? It would be really neat in here (and would remove the need to call the carType(char) method).
When you have java methods that return a value, there is often no need to 'store' the return value to the end of the method... you can just return when you have it....
when dealing with user input, you should try to leave them an 'exit strategy', like "enter Q to quit"
if you do an early-return mechanism, there is no need to do any conditions in the loop, you can just do while (true) {...}

One possible method for you may be something like:

public String promptForCarType(Scanner input) {
    System.out.println("(E) Economy  - 50 TL");
    System.out.println("(M) Midsize  - 70 TL");
    System.out.println("(F) Fullsize - 100 TL");
    // add in a Q option here ....
    // the Scanner may not have input....
    while (true) { // convert to while loop.
        System.out.println("Enter the car type (E/M/F) : ");
        if (!input.hasNext()) {
            throw SomeExceptionToSayTheresNoMoreInput(....);
        }
        String userinput = input.next();
        // check that input is just one character long.....
        char type = userinput.charAt(0);
        type = Character.toUpperCase(type);
        switch (type) {
            case 'E' : return "Economy";
            .....   // other cases
            case 'Q' : throw new UserWantsToQuitException(".....");
            default : // no need for a default, the loop will just go around again
        }
    }
}


HackerRank Strange Counter I wrote a solution to HackerRank Strange Counter:

  The counter counts down in cycles.  At t = 1, the counter displays the number 3.  At each subsequent second, the number displayed by the counter decrements by 1.

  Each time the counter reaches 1, the number becomes 2× the initial number for that countdown cycle.

  The sequence goes:

    3, 2, 1, 6, 5, 4, 3, 2, 1, 12, 11, 10, 9, 8, 7, …

  Print the value of the counter the time t, where 1 ≤ t ≤ 1012.  For example, if t = 4, print 6.

My code works for inputs up to 100 million but when I enter number 1 billion or greater then I get TLE.

#include<stdio.h>
#include<iostream>

using namespace std;

int main()
{
    long long n;

    cin >> n;

    long long  x = 3;
    long long count = x;
    for(long long i = 2; i <= n;i++)
    {
        count--;
        if(i == n)
        {
            break;
        }

    else if(count == 1)
        {    i++;
             x = 2*x;
             count = x;

        }
    }

    cout << count;

    return 0;
}

Style

In C++, don't #include<stdio.h> or any other C-style .h file.  #include <iostream> should be sufficient for all of your I/O needs.  It is customary to put a space after #include.

In general, using namespace std is bad practice.  Here, you actually wasted more characters writing the "shortcut" than writing out std::cin and std::cout like you should have.

In the problem statement, the input is called t.  Why did you name your variable n in your code then?

x is a cryptic variable name.  I suggest countdown_init.

It is customary to end the output with a newline.  If you don't, then in Unix shells the command prompt would immediately follow the output on the same line, which looks ugly.

Algorithm

The reason your program exceeds the time limit is simple: this loop counts up to n, one step at a time.

for(long long i = 2; i <= n;i++)

Your algorithm is thus said to be O(t).

To speed it up, you need to fast-forward through entire countdown sequences.

#include <iostream>

int main() {
    long long countdown_init, t;
    std::cin >> t;
    for (countdown_init = 3; t > countdown_init; countdown_init *= 2) {
        t -= countdown_init;
    }
    std::cout << (countdown_init - t + 1) << '\n';
}

This solution would be O(log t), since it only loops once per countdown cycle.  We can estimate the amount of work by rounding up t to the last t of each cycle:

$$\begin{align}
t \approx&\ 3\ (\underbrace{1 + 2 + 4 + \ldots + 2^{c}}_{c\ \textrm{cycles}}) \\
  \approx&\ 3 \cdot 2^{c+1} \\
  =&\ 6 \cdot 2^c
\end{align}$$

The number of cycles \$c\$ is therefore

$$c \approx \log_2 \frac{t}{6} = O(\log t)$$


Objective-C singleton implementation Is this the right way to do a singleton in Objective-C (coming from an Android background, with a little understanding of threading)?

#import "ItemsManager.h"

#define kNumMaxSelectableItems 15

@implementation ItemsManager{
    NSMutableArray *currentItems;
}

static ItemsManager *myself;

-(id)init{
    if(self=[super init]){
        currentItems = [[NSMutableArray alloc] initWithCapacity:kNumMaxSelectableItems];
        myself = self;
    }
    return self;
}

+(ItemsManager *)getInstance{
    if(!myself){
        myself = [[ItemsManager alloc] init];
    }
    return myself;
}

-(void) doSomething{
    NSLog(@"hi");
}

-(void)dealloc{
    [currentItems release];
    currentItems = nil;
    myself = nil;

    [super dealloc];
}

@end

And with this design, I suppose I would call it like:

void main(){
    [[ItemsManager getInstance] doSomething];
}

This is the the way singletons are usually implemented in apple's code samples.

+ (ItemsManager *)sharedInstance
{
    static ItemsManager *sharedInstance = nil;
    static dispatch_once_t once;

    /* Doing the allocation inside dispatch_once queue ensures the code
       will run only once for the lifetime of the app. */

    dispatch_once(&once, ^{
        sharedInstance = [[ItemsManager alloc] init];
    });

    return sharedInstance;
}


Counting perfect squares Below is the code I have written for counting perfect squares between a given lower and upper bound.

I am using the following concept to solve this:

  Starting with 1, there are \$\sqrt{m}\$ square numbers up to and including \$m\$.

public static int CountPerfectSquares(long A, long B)
{
    int count = 0;

    //Proceed if lowerbound is less than upperbound
    if (A < B)
    {
        //negative numbers are not perfect squares
        if (A < 0 && B < 0) return count;

        //Reset lowerbound if required
        if (A <= 0) A = 1;

        //Find number of squares between A & B (INCLUSIVE lower and upperbound)
        //count = (int)Math.Sqrt(B) - (int)Math.Sqrt(A);  //Giving wrong count when LowerBound value is a whole square (For exp. CountPerfectSquares(1, 25))

        int sqrtA = (int)Math.Sqrt(A);
        int sqrtB = (int)Math.Sqrt(B);
        count = sqrtB - sqrtA;
        //To handle the case when lowerbound value is a perfect square number
        if (A == sqrtA * sqrtA)
        {
            count++;
        }
    }

    return count;
}

Can you please help me make this more efficient, if possible?

Is there any other way of handling the case when the lower bound value is a perfect square number, so that I don't have to do an extra count++?

If A was not included in the observed range, you wouldn't need the if (A == sqrtA * sqrtA) part, right? So, let's use that: instead of checking the range {A,A+1,...,B}, check the range {A-1,A,...,B}. Since you're working with integers, the two are the same:

        int sqrtA = (int)Math.Sqrt(A-1);
        int sqrtB = (int)Math.Sqrt(B);
        count = sqrtB - sqrtA;

A minor detail: when returning a predefined constant, I prefer writing the constant itself, not a variable initialized to that value:

        if (A < 0 && B < 0) return 0;

It seems more readable to me.

Also, I would replace two main if-s with if (B < 0 || B < A) return 0. This gives the same result, but in a cleaner way.

So, my version would be:

public static int CountPerfectSquares(long A, long B)
{
    // Return zero if there are obviously no perfect squares
    if (B < 0 || B < A) return 0;

    // Reset the lower bound if required
    if (A <= 0) A = 1;

    // Find the number of perfect squares between A & B (INCLUSIVE)
    return (int)Math.Sqrt(B) - (int)Math.Sqrt(A-1);  // Now giving the correct count, even when the lower bound value is a whole square (for exp. CountPerfectSquares(1, 25))
}


'Guess the Number' game in Python 3 I am new to learning code, and would love some feedback on how this little game could be improved and/or tidied up.

import random

num = random.randint(1, 100)

def user_guess():
    guess = input("Guess a number between 1 and 100: ")
    print("Your guess is", guess)
    guess = int(guess)

    while guess != num:
        if guess < num:
            guess = input("Your guess was too low: guess again!")
            print("Your guess is", guess)
            guess = int(guess)
        if guess > num:
            guess = input("Your guess was too high: guess again!")
            print("Your guess is", guess)
            guess = int(guess)
        if guess == num:
            print("Correct! Well done!")
            again = input("Would you like to play again? (Y or N): ")
            if again == "y":
                user_guess()
            break

You follow a lot of standard practices well, so the code is actually quite easy to read, but there are some ways to consolidate a bit that'll make it even simpler.

You while loop while the guess is incorrect, but you also break when the answer is correct, nullifying the need for the condition. This also has the unintended side effect that, if their answer is correct in their first guess, you never enter the loop, and therefore never congratulate the user for winning. Change this into while True: or, much better, provide some maximum number of guessed then do for _ in range(max_num_guesses):

You repeat the input/print/int call many times, but this could just be the first part of the loop instead. It alters the formatting very slightly, but makes for much more readable/maintainable code.

global variables aren't usually a good idea in the long run; consider making num a parameter instead, then call randint wherever it is you call this function.

Some last minor tweaks I would suggest: move the "guess again?" logic to outside this function--recursion isn't the most intuitive solution in this case, and could get you into trouble (recursion limits, memory "leaks", etc.); use elif when chaining mutually-exclusive if statements; convert user input to lower case when checking if they input y, especially since your input message explicitly tells them that 'Y' is a valid, expected input.

The final code looks something like the following:

def user_guess(num, max_num_guesses=10):
    for _ in range(max_num_guesses):
        guess = int(input("Guess a number between 1 and 100: "))
        print("Your guess was {}".format(guess))

        if guess < num:
            print("Your guess was too low: guess again!")
        elif guess > num:
            print("Your guess was too high: guess again!")
        elif guess == num:
            print("Correct! Well done!")
            return

    print("Sorry, you ran out of guesses.")

if __name__ == '__main__':
    while True:
        user_guess(random.randint(1, 100))

        again = input("Would you like to play again? (Y or N): ")
        if again.lower() != "y":
            print("Ok, goodbye")
            break


"Largest power of two less than N" in Python def floor_log(num, base):
    if num < 0:
       raise ValueError("Non-negative number only.")
    exponent = 0
    while num > 1:
        num >>= 1
        exponent += 1
    return base << (exponent - 1)

print floor_log(3, 2) #2
print floor_log(4, 2) #4
print floor_log(5, 2) #4
print floor_log(6, 2) #4

I frequently miss the edge cases like while num or while num > 1. What is good strategy to avoiding these mistakes? apart from this the function seems fine to me. Comments are most welcome.

If what you really want is the "largest power of 2", you should make that more explicit:

def largest_power_of_two(n):

And does the largest power of two less than n mean in terms of bits? It's just dropping all of the bits except for the left-most one. Since we can get the bit length of a number directly, we can just do that:

def largest_power_of_two(n):
    return 1 << (n.bit_length() - 1)

If you want error check, you can add the value error if you want (note only positive ns are valid, you initially allowed for 0... but what is the largest power of 2 less than or equal to 0?):

def largest_power_of_two(n):
    if n > 0:
        return 1 << (n.bit_length() - 1)
    else:
        raise ValueError("only meaningful for positive values")

Don't write loops when you don't need to.


Student Marking Calculator This is a student module marks calculator that I was asked to write for a uni revision session in year 2.

Is this a good example of modern C++ code? Is there anything I can improve on or need to change to become a better programmer?

#include <iostream>
#include <cstdlib>

float student_results[5][2];

unsigned int input_student_result(unsigned int);

float input_student_weighting(unsigned int);

unsigned int input_exam_result();

float calculate_exam_weighting();

float calculate_module_mark();

int main() {

    // Coursework 1

    for (unsigned int i = 0; i < 4; ++i) {

        student_results[i][0] = input_student_result(i);

        student_results[i][1] = input_student_weighting(i);

    }

    student_results[4][0] = input_exam_result();

    student_results[4][1] = calculate_exam_weighting();

    std::cout << "Module mark = " << calculate_module_mark() << '\n';
    std::cout << '\n';

    system("pause");

    return 0;

}

unsigned int input_student_result(unsigned int n) {

    unsigned int input_validation = 0;

    do {

        std::cout << "Enter coursework " << (n + 1) << " result <0 - 100> = ";
        std::cin >> input_validation;

    } while (!(input_validation >= 1 && input_validation <= 100));

    return input_validation;

}

float input_student_weighting(unsigned int n) {

    float input_validation = 0;

    do {

        std::cout << "Enter coursework " << (n + 1) << " weighting 0.1 or 0.2 = ";
        std::cin >> input_validation;

    } while (!(input_validation == 0.1f || input_validation == 0.2f));

    return input_validation;

}

unsigned int input_exam_result() {

    unsigned int input_validation = 0;

    do {

        std::cout << "Enter exam result <0-100> = ";
        std::cin >> input_validation;

    } while (!(input_validation >= 1 && input_validation <= 100));

    return input_validation;

}

float calculate_exam_weighting() {

    float exam_weighting = (1 - (student_results[0][1] + student_results[1][1] + student_results[2][1] + student_results[3][1]));

    std::cout << "Exam weighting = " << exam_weighting << '\n';

    return exam_weighting;

}

float calculate_module_mark() {

    float module_mark{
            (student_results[0][0] * student_results[0][1]) +
            (student_results[1][0] * student_results[1][1]) +
            (student_results[2][0] * student_results[2][1]) +
            (student_results[3][0] * student_results[3][1]) +
            (student_results[4][0] * student_results[4][1])
    };

    return module_mark;

}

Here is how you can improve your code:

Define a class instead of using an array for unrelated numbers (0 to 100 vs. 0.1 or 0.2)
Use a std::vector instead of a fixed size array
Prefer double over float, since it has higher precision
Prefer integer arithmetic over floating point arithmetic, since it won't lead to strange results that easily
Group your global functions into a class, to express that they belong together
Add error checking for all calls to std::cin >> var

By the way, you don't look like a beginner since you

don't import the complete std namespace
use \n instead of std::endl

This is already quite good.


Removing redundant lines from script Context: I've been learning Python for a few months and I'm able to write codes which do work, but they normally look very ugly and contain a lot of unnecessary code. But I normally struggle to find better ways to do things because of my limited knowledge.

I would like some advice on what I can improve on the code below. FYI - it works perfectly well, but I'm guessing there must be ways to improve it. I know I'm repeating myself in the loops, for example.

def getTableGroupTopMember_Test1(coordinates):
    '''
    Depending on the number of coordinates given, this function/code returns a unique     coordinate.
    '''

    mylist = coordinates

    for adc in activeDataConnections:
        if mylist.Count == 1:
            for table in adc:
                if table.Name == mylist[0]:
                    print "/"
        elif mylist.Count == 2:
            for table in adc:
                if table.Name == mylist[0]:
                    for topgroup in table.TopAxis.Groups:
                        if topgroup.Name == mylist[1]:
                            print topgroup.Address
        elif mylist.Count == 3:
            for table in adc:
                if table.Name == mylist[0]:
                    for topgroup in table.TopAxis.Groups:
                        if topgroup.Name == mylist[1]:
                            for topmember in topgroup:
                                if topmember.Name == mylist[2]:
                                    print topmember.Address
        else:
            print "your code is shit!"

getTableGroupTopMember_Test1(["Table10"])
getTableGroupTopMember_Test1(["Table10", "profile_julesage",])
getTableGroupTopMember_Test1(["Table10", "profile_julesage", "_25_to_34"])

Output:

/
/2
/2[3]

This is what I would do.

First, rename coordinates to coord_list so you can eliminate mylist which is kind of useless since you only use immediately assign it to coordinates. To make things slightly simpler I assigned count to coord_list.Count so now you can just check count instead of coord_list.Count. Instead of executing the loop if the Count is bad, we check that count is between 1 and 3  before entering the loop. From here it loops though the activeDataConnections and has been changed so it checks count instead of repetition of the same logic.

Here is the code:

def getTableGroupTopMember_Test1(coord_list):
    '''
    Depending on the number of coordinates given, this function/code returns a unique coordinate.
    '''
    count = coord_list.Count

    # thanks to 200_success
    if not 0 < count < 4:
        print "your code is shit!"
        return

    for adc in activeDataConnections:
        for table in adc:
            if table.Name == coord_list[0] and count == 1:
                print "/"
            elif table.Name == coord_list[0]:
                for topgroup in table.TopAxis.Groups:
                    if topgroup.Name == coord_list[1] and count == 2:
                        print topgroup.Address
                    elif topgroup.Name == coord_list[1]:
                        for topmember in topgroup:
                            if topmember.Name == coord_list[2]:
                                print topmember.Address

getTableGroupTopMember_Test1(["Table10"])
getTableGroupTopMember_Test1(["Table10", "profile_julesage",])
getTableGroupTopMember_Test1(["Table10", "profile_julesage", "_25_to_34"])