Simple streaming parser to extract lines It is meant to be given strings over time and output all NewLine delimited substrings. The ColumnReader is meant to be a solution for Delimiter-separated-value files, like CSV or TSV.

using System;

namespace fbstj
{
    public sealed class LineReader
    {
        public string NewLine { get; set; }

        string _buffer = "";

        public void Parse(string text)
        {
            _buffer += text;

            var lastline = _buffer.LastIndexOf(NewLine);
            if (lastline == -1)
                return;

            var lines = _buffer.Substring(0, lastline).Split(NewLine.ToCharArray(), StringSplitOptions.RemoveEmptyEntries);
            _buffer = _buffer.Substring(lastline);

            foreach (var line in lines)
                Receive(line);
        }

        public event Action<string> Receive = (line) => { };
    }

    public struct ColumnReader
    {
        public readonly string Delimiter;
        public readonly LineReader Reader;

        public ColumnReader(string newline, string delimiter) : this()
        {
            Reader = new LineReader { NewLine = newline };
            Delimiter = delimiter;
            Receive += (columns) => { };
            Reader.Receive += _receive_line;
        }

        private void _receive_line(string line)
        {
            Receive(line.Split(Delimiter.ToCharArray()));
        }

        public event Action<string[]> Receive;
    }
}

My main use-case is reading lines from a SerialPort:

var parser = new IO.LineReader { NewLine = port.NewLine };
port.DataReceived += (o, e) => parser.Parse(port.ReadExisting());
parser.Receive += (line) => { };

This looks like they used to be fields and then you made them properties:

public readonly string Delimiter;
public readonly LineReader Reader;

IMHO that means they should get a getter and setter, like this:

public string Delimiter { get; private set; }
public LineReader Reader { get; private set; }

String.LastIndexOf is culture-specific, so consider using a StringComparison.


Number-guessing game in C# I have just written a solution to the following problem:

  We all know the classic "guessing game" with higher or lower prompts. lets do a role reversal; you create a program that will guess numbers between 1-100, and respond appropriately based on whether users say that the number is too high or too low. Try to make a program that can guess your number based on user input and great code!

I would like suggestions on how I can make my solution less complicated and if there is a more efficient way to solve the problem.

Here is my implementation.  I have tried to implement a binary search strategy to solve the problem.

public class Guesser
{
    private List<int> _possibleAnswers = new List<int>();
    private bool _isCorrectNumber = false;
    private bool _higher = false;
    private int _numberOfGuesses = 0;
    private int _answer;

    public Guesser(int min, int max)
    {
        for (var i = min; i <= max; i++)
            _possibleAnswers.Add(i);
    }

    public void Guess()
    {
        while (!_isCorrectNumber)
        {
            var middleOfAnswers = _possibleAnswers.Max() - (_possibleAnswers.Count / 2);

            Console.WriteLine("Is the number {0}?", middleOfAnswers);

            var hint = Console.ReadLine().ToLower();

            _isCorrectNumber = hint.Equals("yes");

            if (_isCorrectNumber)
            {
                _answer = middleOfAnswers;
                continue;
            }

            _higher = hint.ToLower().Equals("higher");

            if (_higher)
            {
                var count = _possibleAnswers.FindIndex(guess => guess.Equals(middleOfAnswers)) + 1;
                _possibleAnswers.RemoveRange(0, count);
            }
            else
            {
                var startIndex = _possibleAnswers.FindIndex(guess => guess.Equals(middleOfAnswers));
                var count = _possibleAnswers.FindIndex(guess => guess.Equals(_possibleAnswers.Max())) - startIndex + 1;

                _possibleAnswers.RemoveRange(startIndex, count);
            }
            _numberOfGuesses++;
        }
        Console.WriteLine("The number was {0} and it took me {1} guess{2}",
            _answer, _numberOfGuesses, _numberOfGuesses > 1 ? "es" : string.Empty);
    }
}

Any suggestions on how I can improve this would be great.  I'd also like to know if I have understood the concept behind a binary search and implemented it correctly.

It seems to me that you understand the concept correctly, but your implementation is horrible. That's because you don't need the whole _possibleAnswers list at all, you should just keep two numbers: the lower bound and the upper bound, you can compute everything you need from that.

There are also some smaller issues:

Don't use Max() if you know that the maximum number will be always the last one. You can instead use Last(), that will be much faster. And a something similar applies to the way you compute count in the else branch: you already know that FindIndex() will return the index of the last item in the list, you don't need to walk the list twice for that.
C# is not Java, you should use == to compare strings instead of Equals(), it's much more readable.
You won't need _isCorrectNumber, if you use break instead of continue. That would make your code shorter and less confusing. (I thought something like “wait, why does he continue when he wants to end iterating?” when I read that part of the code.)

Binary search should take only O(log n) time, but your code is O(n2 log n), which is really bad. The n2 part is caused by using Max() inside of the FindIndex() lambda, if you fixed that by simply computing the Max() once before calling FindIndex() (which is still horrible, see #1 above), it would make your code O(n log n), although that is still bad.


Letter Combinations of a Phone Number I was asked to code a solution in Python in 15 mins. Print all possible word combinations for a given phone number. Numbers 0 and 1 will not be part of the phone number, the numbers can be repeated, phone numbers can be of any length.
For example if the number is 245:

All combinations of 'ABC', 'GHI' and 'JKL" is:
['AGJ', 'AGK', 'AGL', 'AHJ', 'AHK', 'AHL', 'AIJ', 'AIK', 'AIL', 'BGJ', 'BGK', 'BGL', 'BHJ', 'BHK', 'BHL', 'BIJ', 'BIK', 'BIL', 'CGJ', 'CGK', 'CGL', 'CHJ', 'CHK', 'CHL', 'CIJ', 'CIK', 'CIL']

In 15 mins, all I could come up with was to combine 2 numbers at a time and then combine the result with the third number and so on.

numbs = {'2':'ABC', '3':'DEF', '4':'GHI', '5':'JKL', '6':'MNO', '7':'PQRS', '8':'TUV', '9':'WXYZ'}

def combo(phnum):
    if len(phnum) == 1: return list(numbs[phnum[0]])
    else: result = combo(phnum[:-1])
    return [(ch1+ch2) for ch1 in result for ch2 in numbs[phnum[-1]]]

After I going home, I checked out itertools and found out about product. Came up with the following one liner. Is there a better way to solve this problem?

from itertools import product
def combo(phnum):
    return [''.join(tup) for tup in list(product(*[numbs[ch] for ch in phnum]))]

Solution overall seems nice, although:

casting to list in list comprehension is not needed
product returns iterator, so you also may want to generate iterator if you mainly plan to iterate over possibilities
I'd strongly advice using better names. Characters in source code costs nothing and maintaining code afterwards is much easier

My attempt would be:

import itertools

letters_map = {'2':'ABC', '3':'DEF', '4':'GHI', '5':'JKL',
               '6':'MNO', '7':'PQRS', '8':'TUV', '9':'WXYZ'}

def possible_words(phone_number):
    letters_to_combine = (letters_map[digit] for digit in phone_number)
    for letters_group in itertools.product(*letters_to_combine):
        yield ''.join(letters_group)

print list(possible_words("23567"))


Median of numbers stored in array Problem is to calculate median of numbers. My idea is :

Fill array of integer with numbers using Arraymake() function
copy the address into a pointer created in main() function
pass that array to Median() function and calculate that median

Code:

    #include <stdio.h>
    #include <stdlib.h>

    int* Arraymake(size_t);//Function for creating array of numbers.
    void  Median(size_t ,int* );//Function for calculate the median.

    int main(int argc, char **argv)
    {
        size_t count;//Size of the list of numbers.
        puts("Size of the list:");
        scanf("%Iu",&count);
        int *ArResult=Arraymake(count);//Result of the first function
        Median(count,ArResult);
        free(ArResult);
        return 0;
    }

    int* Arraymake(size_t ln)   //ln~lenght
    {
        int* Array=(int*) malloc(ln*sizeof(int));
        int number ,i; //numbers filling the array

        puts("Please input numbers.\n");

        for(i=0;i<(ln);i++)
            {
                scanf("%i", &number);
                Array[i]=number;
            }
            return Array;
    }

    void  Median(size_t ln,int* Array)
    {
        float median;

        if((ln)%2==0)
            {
                    median= (Array[(ln)/2]+Array[(ln/2 )-1] )/2;
                    printf("%f",median);

            }
        else
            {
                    median= Array[((ln-1)/2)];
                    printf("%f",median);
            }
}

I want to find a way to optimize my variables and make my code more self-documented? How can I improve my code?

1. Arraymake:

Change the name to MakeArray as it makes more sense or name it MakeArrayFromInput to make it more descriptive to what it actually does
Name the parameter length, as in : int* MakeArray(size_t length) - this
is descriptive and you don't have to put the comment explaining what it does
Name local variables in camelCase, so array instead of Array
Matter of preference but consider declaring array as int *array instead of int* array - you won't forget that the variable itself is a pointer and you have to declare additional pointers in the same statement with *s - int *array, *anotherPointer;
int number ,i; //numbers filling the array - comment is redundant and you don't have to declare i here, you can do it in the for - speaking of which:
for(i=0;i<(ln);i++) can become for(int i = 0; i < length; i++) - parentheses over ln are redundant, you should use better spacing to make it more readable.

2. Median:

The same story with the parameter and naming, also pass the array first, then the length : void Median(int *array, size_t length)
You don't have to declare the median variable before the if
Parentheses in the if are redundant : if(length % 2 == 0)
The calculation of the median in the case of an even number of elements is flawed - you do an integer division as both operands of / are ints - you'd have to make at least one of them a float e.g. like this median= (Array[(ln)/2]+Array[(ln/2 )-1] )/2.0; - see the 2.0 - this is a double literal, a floating-point number, not an int
You can shorten the calculation a bit by changing array[(length - 1) / 2]; to array[length / 2]; in the case of an odd number - the integer division will truncate the result into what you need
The function should do one thing - you're calculating the median and printing it in the same function. Consider making the function Median return the value of the median and print it somewhere else

The function then should look more or less like this :

float Median(int *array, size_t length)
{
    if(length % 2 == 0)
    {
        float median = (array[length / 2] + array[length / 2 - 1]) / 2.0;
        return median;
    }
    else
    {
        float median = array[length / 2];
        return median;

        //or just simply
        return array[length / 2];
    }
}

Something more complex, the function can be greatly simplified by using the ternary operator ?: :

float Median(int *array, size_t length)
{
    return length % 2 == 0 ?
           (array[length / 2] + array[length / 2 - 1]) / 2.0 :
           array[length / 2];
}

However, this is just trivia and you should aim to make your code as readable as possible

3. main

I think that all of the comments are redundant here, everything is self-documenting
Naming : ArResult should be something descriptive and simple - the most simple way would be to name it just array
Spacing in function parameters e.g.
scanf("%Iu", &count); instead of scanf("%Iu",&count);

4. General advice

Work on the spacing - try to put spaces between operands and operators as in if(length % 2 == 0), in places like for loops : for(int i = 0; i < length; i++) and in function parameters : scanf("%Iu", &count);
Put comments before the thing they're describing, not at the end of the line - this way if you want to make it a multi-line comment you won't have problems and it's more readable that way e.g.

like this:

//Function for creating array of numbers.
int* MakeArray(size_t);
//Function for calculating the median.
int Median(int*, size_t);

This is just an example - I think the function names are descriptive enough not to put comments there, they will clutter the view :

int* MakeArray(size_t);
int Median(int*, size_t);

EDIT

What I didn't initially remember is that to calculate the median, you have to use a sorted array. You should probably do that separately

int *array = MakeArray(count);
qsort(array, count, sizeof(int), compare)
float result = Median(count, array);

The qsort is a function from stdlib.h. You have to also declare a function compare to use in it e.g.

int compare(const void* a, const void* b)
{
     int int_a = *((int*) a);
     int int_b = *((int*) b);

     if(int_a == int_b) return 0;
     else if (int_a < int_b) return -1;
     else return 1;
}

This is taken from https://stackoverflow.com/a/3893967/7931009


Compute the XOR of all the elements in the upper left triangular of a square matrix Given a dimension \$D\$. We generate a \$D\times D\$ matrix with elements that start from \$0\$. For example, if we're given a dimension \$4\$ this is what our matrix looks like:
$$
    \begin{bmatrix}
     0 & 1 & 2 & 3 \\
     4 & 5 & 6 & 7 \\
     8 & 9 & 10& 11\\
     12& 13& 14& 15\\
    \end{bmatrix}
$$

Now compute the XOR all of the elements in the upper triangular of the square matrix

0 ^ 1 ^ 2 ^ 3 ^ 4 ^ 5 ^ 6 ^ 8 ^ 9 ^ 12 gives us 10

def computeXOR(dimension):
    XOR = 0
    begin = 0
    for i in range(dimension):
        for j in range(dimension-i):
            XOR ^= begin + (j + i*dimension )
    return XOR

#computeXOR(4) returns 10

I'm trying to think of a faster algorithm to XOR all of the elements in the upper triangular of a square matrix. My current solution is \$O(n^2)\$.

You will need to look at \$O(n^2)\$ elements, therefore there is no algorithm that runs faster than \$O(n^2)\$.

That is, unless you have an explicit formula for the solution which abuses the fact that the elements are all consecutive.

Notice what these XOR to:

      cumulative XOR
0 000 000
1 001 001 |
2 010 011 |
3 011 000 |
4 100 100 |
5 101 001
6 110 111
7 111 000

We'll notice a few patterns. The cumulative XOR of all elements \$\lt 2^k\$ for any k is 0.

Here's another, more useful pattern: for any run of \$2^k\$ consecutive elements (e.g. see markers above), all binary digits except the most significant bit will repeat. Therefore we can explicitly compute the XOR of an arbitrarily large sequence \$a \ldots b\$ of length \$2^k\$ in one step (it's all 0s except the most significant digit is b - max(a, largestPowerOf2LessThanB) % 2, I think, where \$\mathrm{largestPowerOf2LessThanB} = 2^{\lfloor\log_2(b)\rfloor}\$).

Therefore we can calculate the sum of any arbitrarily large sequence in roughly \$O(\log(k))\$ steps.

Thus you can make an algorithm with running time \$O(n\log(n))\$ using this explicit formula.

You can maybe make it even faster if you can calculate contiguous blocks of the matrix?


Logical gates with integer values I have created this code for both boolean and integer values to display a truth table for an "AND","OR","XOR", "NOT" gate. However I think that my code needs reviewing as it could be simplified.

public class LogicalOpTable {
    public static void main(String[] args){

        boolean p,q;

        System.out.println("P\tQ\tAND\tOR\tXOR\tNOT");

        p = false;
        q = false;
        System.out.print(p + "\t" + q + "\t" + (p&&q) + "\t");
        System.out.println((p||q)+"\t"+(p^q)+"\t"+(!p));

        p = false;
        q = true;
        System.out.print(p + "\t" + q + "\t" + (p&&q) + "\t");
        System.out.println((p||q)+"\t"+(p^q)+"\t"+(!p));

        p = true;
        q = false;
        System.out.print(p + "\t" + q + "\t" + (p&&q) + "\t");
        System.out.println((p||q)+"\t"+(p^q)+"\t"+(!p));

        p = true;
        q = true;
        System.out.print(p + "\t" + q + "\t" + (p&&q) + "\t");
        System.out.println((p||q)+"\t"+(p^q)+"\t"+(!p));

        System.out.println();

        withBinary();

    }

    public static void withBinary(){

        System.out.println("A\tB\tAND\tOR\tXOR\tNOT");
        int a = 0;
        int b = 0;
        int and = a&b;
        int or = a|b;
        int xor = a^b;
        int not  = a;

        if(a==0 && b == 0 )
            not = 1;
            System.out.println(a + "\t" + b + "\t" + and + "\t" + or + "\t" + xor + "\t" + (not));

        b=1;
        and = a&b;
        or = a|b;
        xor = a^b;
        not = a;
        if(a==0 && b == 1)
            System.out.println(a + "\t" + b + "\t" + and + "\t" + or + "\t" + xor + "\t" + (not));

        a=1;
        b=0;
        not = b;
        and = a&b;
        or = a|b;
        xor = a^b;
        not = a;
        if(a==1 && b == 0)
            System.out.println(a + "\t" + b + "\t" + and + "\t" + or + "\t" + xor + "\t" + (not));

        a=1;
        b=1;
        not = 0;
        and = a&b;
        or = a|b;
        xor = a^b;
        if(a==1 && b == 1)
            System.out.println(a + "\t" + b + "\t" + and + "\t" + or + "\t" + xor + "\t" + (not));
        }
}

First part

You want to iterate through all possible combinations of for a pair of booleans, so you can make the code reflect that explicitly and make it simpler:

    System.out.println("P\tQ\tAND\tOR\tXOR\tNOT");

    for (boolean p : new boolean[] {true, false}) {
        for (boolean q : new boolean[] {true, false}) {
            System.out.print(p + "\t" + q + "\t" + (p&&q) + "\t");
            System.out.println((p||q)+"\t"+(p^q)+"\t"+(!p));
        }
    }
    System.out.println();

Second part

You have something suspicious in your if statements. As you don't use braces, this code smells of a copy&paste bug:

    if(a==0 && b == 0 )
        not = 1;
        System.out.println(a + "\t" + b + "\t" + and + "\t" + or + "\t" + xor + "\t" + (not));

If the condition is met, it will execute the line not = 1;. The second line will be executed independently of the condition. I strongly suggest you use braces even for 1-line blocks, like this:

    if (a == 0 && b == 0) {
        not = 1;
        System.out.println(a + "\t" + b + "\t" + and + "\t" + or + "\t" + xor + "\t" + (not));
    }

This way you can avoid this kind of bugs.

Now, applying the same reasoning as with the previous method, you can rewrite it as:

    System.out.println("A\tB\tAND\tOR\tXOR\tNOT");
    for (int a : new int[] {0, 1}) {
        for (int b : new int[] {0, 1} ) {
            System.out.println(a + "\t" + b + "\t" + (a & b) + "\t" + (a | b) + "\t" + (a ^ b) + "\t" + ~a);
        }
    }

BUT What do you want to accomplish with the NOT operation? Do you mean the Bitwise Complement? I used that, but maybe you want a function that returns 0 when it's 1, and 1 when it's 0. In that case, you need to replace the ~a with something like (a == 0) ? 1 : 0.

So the whole code could be reduced to just:

public static void printTable() {
    System.out.println("P\tQ\tAND\tOR\tXOR\tNOT");
    for (boolean p : new boolean[]{true, false}) {
        for (boolean q : new boolean[]{true, false}) {
            System.out.print(p + "\t" + q + "\t" + (p&&q) + "\t");
            System.out.println((p||q)+"\t"+(p^q)+"\t"+(!p));
          }
    }
    System.out.println();
    System.out.println("A\tB\tAND\tOR\tXOR\tNOT");
    for (int a : new int[]{ 0, 1}) {
        for (int b : new int[]{0, 1} ) {
            System.out.println(a + "\t" + b + "\t" + (a & b) + "\t" + (a | b) + "\t" + (a ^ b) + "\t" + ~a);
        }
    }
}


Class Seperation vs Polymorphism I'm interested in optimizing my code. Can someone help me understand the best way to trim this code down with DRY methodology?

So I've got these two classes here preforming WMI queries on two separate Win32 classes. My questions are:

Should I keep them separated in different classes based on the Win32 class for organizational purposes?
Should I use a parent partial class and split these into different files?
Should I use methods with optional parameters or overloads in order to reduce the number of classes here?
Why is the selected method best?

public class Win32OperatingSystem
{
    public ulong GetFreePhysicalMemory()
    {
        return GetProperty("FreePhysicalMemory");
    }

    public ulong GetTotalVirtualMemorySize()
    {
        return GetProperty("TotalVirtualMemorySize");
    }

    public ulong GetFreeVirtualMemory()
    {
        return GetProperty("FreeVirtualMemory");
    }

    private ulong GetProperty(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }

    private ulong BytesToMegaBytes(ulong bytes)
    {
        return bytes / (ulong)1024;
    }
}

public class Win32ComputerSystem
{
    public string GetName()
    {
        return GetProperty("Name");
    }

    public string GetManufacturer()
    {
        return GetProperty("Manufacturer");
    }

    public string GetModel()
    {
        return GetProperty("Model");
    }

    private string GetProperty(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_ComputerSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return "Unable to retrieve " + propertyName + " from Win32_ComputerSystem!";
            return enu.Current[propertyName].ToString();
        }
    }
}

Questions in this series

This is the first question in the series.

First iteration of DRY refactoring

Second iteration of DRY refactoring.

Genericizing PropertyValues

How about leveraging Enum.ToString()?

Given these two enums:

public enum Win32OperatingSystem
{
    FreePhysicalMemory,
    TotalVirtualMemorySize,
    FreeVirtualMemory
}

public enum Win32ComputerSystem
{
    Name,
    Manufacturer,
    Model
}

Now you can have a simple method:

    public ulong SelectWin32Property(Win32OperatingSystem property)
    {
        var propertyName = property.ToString();
        var moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }

If you wanted an overload that takes a Win32ComputerSystem parameter, you'd have to deal with the different return value:

    public string SelectWin32Property(Win32ComputerSystem property)
    {
        var propertyName = property.ToString();
        var moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_ComputerSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return "Unable to retrieve " + propertyName + " from Win32_ComputerSystem!";
            return enu.Current[propertyName].ToString();
        }
    }

The easiest would probably be to box the numeric types into a string, so all overloads would return a string representation of their values. This might not be practical, but at least you don't need to write a new method nor to change your interface to support a new property.

Alternatively you could box results in an object, but that wouldn't be pretty, I'd much prefer dealing with a string than exposing object.

The ideal would be to avoid boxing the numeric return types altogether, so the methods have to live in separate classes - this is where generics come handy:

public abstract class Win32QueryBase<TResult, TProperty>
{
    public abstract TResult SelectWin32Property(TProperty property);
}

This can be implemented by this class:

public class Win32OperatingSystemQuery : Win32QueryBase<ulong, Win32OperatingSystem>
{
    public override ulong SelectWin32Property(Win32OperatingSystem property)
    {
        // ...
    }
}

And this class:

public class Win32ComputerSystemQuery : Win32QueryBase<string, Win32ComputerSystem>
{
    public override string SelectWin32Property(Win32ComputerSystem property)
    {
        // ...
    }
}


Java code to convert between hex & binary strings I have two methods in my code which are using the majority of the CPU time that convert between binary and hex strings and the reverse. I use a static lookup map to convert the individual characters but it's still slow given the amount of data I'm working with.

public static String hexToBin(String s)
{
    StringBuilder binaryString = new StringBuilder();

    for(int i = 0; i < s.length(); i++)
    {
        // This string loop can be quite long sometimes > 3000 chars
        binaryString.append(binMap.get(s.charAt(i)));
    }

    return binaryString.toString();
}

public static String binToHex(String s)
{
    if (s.length() % 4 != 0)
    {
        s = Utils.leftPad4(s, (int)(4 * Math.ceil((double) s.length() / 4.0))); // Pad out any single bit values
    }

    StringBuilder hexString = new StringBuilder();
    StringBuilder chars = new StringBuilder();

    for (int i = 0; i < s.length(); i += 4)
    {
        chars.append(s.charAt(i));
        chars.append(s.charAt(i + 1));
        chars.append(s.charAt(i + 2));
        chars.append(s.charAt(i + 3));

        hexString.append(hexMap.get(chars.toString()));

        chars.delete(0, chars.length());
    }

    return hexString.toString();
}

/* The lookup tables */
private static Map<Character, String> binMap = new HashMap<>();
private static Map<String, Character> hexMap = new HashMap<>();

public Types()
{
    binMap.put('0', "0000");    binMap.put('1', "0001");    binMap.put('2', "0010");
    binMap.put('3', "0011");    binMap.put('4', "0100");    binMap.put('5', "0101");
    binMap.put('6', "0110");    binMap.put('7', "0111");    binMap.put('8', "1000");
    binMap.put('9', "1001");    binMap.put('A', "1010");    binMap.put('B', "1011");
    binMap.put('C', "1100");    binMap.put('D', "1101");    binMap.put('E', "1110");
    binMap.put('F', "1111");

    for (Character k : binMap.keySet())
    {
        hexMap.put(binMap.get(k), k);
    }
}

There are several things that hinder the performance of your code:

You are using a map to convert from hex digits to binary strings. However map lookups are slow, compared to simple array indexing or a switch statement . What you need to do, instead of using a map, just use a switch statement, when converting from hex to bin:

    switch (s.charAt(i))
    {
      case '0':
        binaryString.append("0000");
        break;
      ...
      case 'F':
        binaryString.append("1111");
        break;
    }

However, this approach probably will not be as efficent when converting from bin to hex.

Because it is easy to calculate the length of a bin string from the length of a hex string, you can easily preallocate StringBuilder's with the calculated size. This will save you time, because the will be no memory reallocations.
If you code is not multithreaded, you could preallocate the StringBuilder's themselves and store them in some private fields of your class. This way you'd avoid spending time for unnecessary creation of the object, that would almost immediately be destroyed.


Fixed size priority queue I've tried implementing priority queue with fixed size in Java using TreeSet and overriding add() method:

public class FixedSizePriorityQueue<E> extends TreeSet<E> {
    private final int capacity;

    public FixedSizePriorityQueue(final int capacity) {
        this.capacity = capacity;
    }

    public FixedSizePriorityQueue(
            final int capacity,
            final Comparator<? super E> comparator) {

        super(comparator);
        this.capacity = capacity;
    }

    @Override
    public boolean add(final E e) {
        // initialized with 0 or less than zero capacity
        if (capacity <= 0) {
            return false;
        }

        // keep adding until we fill the queue
        if (size() < capacity) {
            return super.add(e);
        }

        if (comparator() != null
                && comparator().compare(this.last(), e) < 0) {
            pollLast();
            return super.add(e);
        }

        return false;
    }
}

I would really appreciate your thoughts, hints and comments.

Two thoughts:

If there is other code and you need all the features of a TreeSet great, otherwise delegate to a TreeSet member variable.
Your code if (capacity <= 0) is somewhat superfluous because of the next test if (size() < capacity)


Function to return last line's length of a string I am completely re-writing the textbox GUI widget.
In order to set the position cursor after the last character in the last line of a text, it is good if you know the exact column of it. So I created a small function that calculates this. It is meant to work sonicly.

int get_last_line_length (char *string)
{
    const int len = strlen(string) - 1;
    int i;

    for(i = len; i != -1 && string[i] != '\n'; i--);

    return (len - i);
}

It's good that you've used strlen, but there's actually another standard function that could be useful here. Instead of your reverse loop, you can use strrchr to find a pointer to the last occurrence of a character (or NULL if said character is not found). As discussed in the comments, this could be (and likely is) slower in the case when a newline is not found, but unless this function is called very often or is in a very performance critical path, I can't imagine it will be an issue. If you do need to optimize it to your original implementation, I would pull the reverse loop logic out into a function.

Anyway, a bit of a review:

Your string parameter should be a pointer to const since it's not modified.
Personal style thing: I'm not a fan of return (...). If it matters, it also tends to be pretty rare style wise in C.
You should probably explicitly document what happens if a line break is not found in the text. I wouldn't be sure whether strlen(string) or -1 we be returned.


AngularJS Nested Directives In an effort to learn more about AngularJS directives, I decided to create a simple, edit-in-place directive. I built a simple directive that would display a value, along with an edit link and the edit link would toggle visibility of an input and the save/cancel links. From there, I wanted a setup where I could make multiple properties of an object editable, and I wanted to share the link controls and trigger updates to each property together.

I decided to create a wrapper directive to handle the links and ended up with a working version in which I ended up broadcasting events from the parent(wrapper) directive and taking action in the nested directives based on that. I feel like there is probably a better approach, but this is what I was able to make work.

The HTML:

    <link rel="stylesheet" href="style.css" />

          <wrap-in-place>
            <value-in-place value="p.fname"></value-in-place>
            <value-in-place value="p.lname"></value-in-place>
          </wrap-in-place>

The JavaScript:

  var app = angular.module('editInPlaceApp',['myDirectives']);
  app.controller('MainController', ['$scope', 'PersonService', function($scope, PersonService){
    $scope.people = PersonService.loadPeople();
  }])
  .factory('PersonService', function(){
    var factory = {};

    factory.loadPeople = function(){
      var peeps = [{"id":"abc0", fname:"John", lname:"Doe"},
        {"id":"abc1", fname:"Jane", lname:"Smith"},
        {"id":"abc2", fname:"Bob", lname:"Jones"}];

      return peeps;
    };

    return factory;
  });

  angular.module('myDirectives',[])
  .directive('wrapInPlace', function(){
    return {
      restrict: 'E',
      transclude: true,
      scope:true,
      template: ' edit save cancel',
      link: function($scope, element, attrs){
        $scope.edit = function(){
          $scope.editChildren = true;
          $scope.$broadcast('editChildren',{"val":$scope.editChildren});
        };

        $scope.cancel = function(){
          $scope.editChildren = false;
          $scope.$broadcast('editChildren',{"val":$scope.editChildren});
        };

        $scope.saveChildren = function(){
          $scope.editChildren = false;
          $scope.$broadcast('saveChildren');
        };
      }
    };
  })
  .directive('valueInPlace', function(){
    return {
      restrict: 'E',
      scope: {
        value: '='
      },
      template: '{{value}}<input type="text" ng-model="updatedValue" ng-show="editing">',
      link: function ( $scope, element, attrs ) {
        $scope.editing = false;
        $scope.updatedValue = $scope.value;

        $scope.$on('editChildren', function(event, args){
          $scope.editing = args.val;
          if(!args.val){
            $scope.updatedValue = $scope.value;
          }
        });

        $scope.$on('saveChildren', function(event, args){
          $scope.editing = false;
          $scope.value = $scope.updatedValue;
        });

      }
    };
  });

The full working example can be found here.

It is always better to follow component approach in angularjs. Even if your using angularjs prior to version 1.5.x you can use directives to emulate this approach thus making each directive completely isolated manageable code chunk. it is always better to avoid usage of $scope. it leads to creation of scope soup. To avoid this use controllerAs namespace. and rather than broadcasting events which might lead to uncertain behavious use services to enable communication between directives. This will make your code more manageable and readable and in the future you could easily port to angular2 if you want.


Animating login screens I know there is probably a better way to have coded this. Can anyone be of help? I am not an expert JavaScript coder.

$(function() {
    $('#clientLoginText').hover(function() {
        $('#clientLoginBox').stop().animate({ "height" : '80px'}, 700);
    }, function() {
        //$('#clientLoginBox').stop().delay(500).animate({ "height" : '0px'}, 700);
    });
});

$(function() {
    $('#clientLoginBox').hover(function() {
    }, function() {
        $('#clientLoginBox').stop().delay(500).animate({ "height" : '0px'}, 700);
    });
});

Well here is a tidied up version that shouldn't change the functionality at all:

$(function() {
    $('#clientLoginText').mouseenter(function() {
        $('#clientLoginBox').stop().animate({ "height" : '80px'}, 700);
    });

    $('#clientLoginBox').mouseleave(function() {
        $(this).stop().delay(500).animate({ "height" : '0px'}, 700);
    });
});

About the changes:

You only need a single document.ready function: the only good reason I can think of for having two is if you need to put them in separate JS files.
You had used the .hover() function, which is shorthand for .mouseenter() and .mouseleave(), but in the first use you passed an empty function for the mouseleave part and in the second use you passed an empty function for the mouseenter part, so why not just code the first directly as .mouseenter() and the second as .mouseleave()?
Within an event handler you can refer to the element that triggered the event as this, or $(this) if you need it as a jQuery object. More efficient than getting jQuery to find it again from its id.


The Next Palindrome: Is my code efficient? I was trying to solve the Next Palindrome problem listed in SPOJ in Ruby language. Please find the problem statement here. Though i came up with the below solution, i could not get the execution time below 1.4s even after tweaking this code numerous times. Where can this code be optimized in terms of memory and execution time or is there a better approach than the solution i came up with?

def next_palindrome
  number = gets.chomp
  len = number.length
  if len == 1
    return "11\n" if number == "9"
    return number.next!<<"\n"
  end
  if len.even?
    f = len/2
    return number[0..f-1]<<number[0..f-1].reverse<<"\n" if number[0..f-1]>number[f..-1]
    number = number[0..f-1].next!
    return number<<number[0..f-1].reverse<<"\n"
  else
    f = (len-1)/2-1
    return number[0..f+1]<<number[0..f].reverse<<"\n" if number[0..f]>number[f+2..-1]
    number = number[0..f+1].next!
    return number<<number[0..f].reverse<<"\n"
  end
end

def input
  gets.chomp.to_i.times {print next_palindrome}
end

input

Cache cache cache! :) All those substrings you take should be stored in variables, rather than using the expensive substring methods. Furthermore, comparing strings can also be expensive. When you store the substring in a variable, you could parse the integer and keep it for comparisons later.

Advanced:

Since you never need to look at the last half number (if it's a palindrome it must be the reverse of the first digits), you can just store two variables: big_half and small_half. for even-digit numbers, they will be equal, but for odd-digit numbers big_half will have the middle digit. This makes creating a that is adjacent to the number (either just smaller or just larger) as simple as big_half + small_half.reverse. If a test shows it's the smaller one, increment big_half and generate a new small_half from it, then add together as above.


Compare all files to each other I will start by saying I have gotten this to work, and the question is about how improve the script to make it better/faster.
I need to compare a series of files to each other. Let's say I have the below files:
filename0   filename1   filename2

What I have done is:
Z=0 # Beginning number
Y=2 # Final number

X=$Z # Initial file number
((W=Y+1)) # Number for ranges

while [[ $Z -lt $W ]]
do
    while [[ $X -lt $W ]]
    do
        obabel filename$Z.xyz -oconfabreport -xf filename$X.xyz -xr 0.2 > Results.$Z.$X
    ((X=X+1))
    done
    ((Z=Z+1))
    ((X=Z))
done

It compares files like this:
filename0 filename0
filename0 filename1
filename0 filename2
filename1 filename1
filename1 filename2
filename2 filename2

I am using openbabel's confabreport tool to find RMSD of structures. Is there a better way of doing this in bash scripting? Such as skipping a self comparison or ones that have already been compared? Or just a faster way of achieving the same thing? Please let me know if I need to provide any additional information, or if this question would be better somewhere else.

Welcome to Code Review!
Shellcheck
Use the amazing shellcheck utility to verify for common practices when writing bash/shell scripts. It reports missing shebang.
Loop using seq
Since you know the lower and upper bounds, and those are pure integers, use the seq command to iterate through them. You won't need to do custom increments and variable swaps.
Inner loop
Begin your inner loop with a value starting from current outer loop value + 1.
Variable
Using more descriptive variable names helps in understanding your code.
Parallelization
To achieve faster results, you can maybe use parallel to do the diff comparison for different files. If the set of files are small, you can also run the obabel tool in background, or disown the command from script entirely (depending on your requirements).

#!/bin/bash

LOWER_BOUND=0
UPPER_BOUND=2

for i in $(seq "$LOWER_BOUND" "$UPPER_BOUND")
do
    ((INNER_START=i+1))
    for j in $(seq "$INNER_START" "$UPPER_BOUND")
    do
        obabel "filename${i}.xyz" -oconfabreport -xf "filename${j}.xyz" -xr 0.2 > "Results.${i}.${j}"
    done
done

I'll leave parallelization to your own implementation. You can refer this excellent demonstration of parallel command.


Given 2 disjoint sets of intervals, find the intersections def getIntersection(set_item1, set_item2):
    (l11, l12) = set_item1
    (l21, l22) = set_item2
    if (l11 > l21 and l11 < l22) or (l12 > l21 and l12 < l22) or (l21 > l11 and l21 < l12) or (l22 > l11 and l22 < l12):
        if l11 > l21:
            start = l11
        else: start = l21

        if l12 > l22:
            end = l22
        else:
            end = l12
        return (start ,end)
    return None

def returnInersection(set1, set2):
    for l1 in set1:
        for l2 in set2:
            ret = getIntersection(l1, l2)
            if ret:
                print ret

I think above one works fine but it runs in O(n2). Is there any better solution?

Example input can be set1=[(1, 6), (10, 15), (20, 30)] and set2=[(2, 5), (7, 13)].

borrowing for Oscar Smith's solution

If the intervalsX are ordered, you can simply use:

def return_intersections(intervals1, intervals2):
    iter1 = iter(intervals1)
    iter2 = iter(intervals2)

    interval1 = next(iter1)
    interval2 = next(iter2)

    while True:
        intersection = getIntersection(interval1, interval2)
        if intersection:
            yield intersection
            try:
                if intersection[1] == interval1[1]:
                    interval1 = next(iter1)
                else:
                    interval2 = next(iter2)
            except StopIteration:
                return

        try:
            while interval1[0] > interval2[1]:
                interval2 = next(iter2)
            while interval2[0] > interval1[1]:
                interval1 = next(iter1)
        except StopIteration:
            return

list(return_intersections(set1, set2))

[(2, 5), (10, 13)]

in Python pre-3.5 all the try-except boilerplate wouldn't be necessary, but pep-476 changed this.

The advantage of this is that it works for any stream of ordered intervals. So intervalsX can be a generator, list, tuple,...


C++11 class similar to .Net's ManualResetEvent, but without the ability to "reset" The goal is to block all threads that call WaitOne(). When Set() is called, those threads continue. Any calls to WaitOne(), after Set(), return immediately. The challenge is to make sure that all calls to WaitOne() are either "before" or "after" the call to Set(), so that no threads see _state false, but miss the notify_all. I'm looking for feedback on performance and correctness.

#ifndef NO_RESET_EVENT_H
#define NO_RESET_EVENT_H

#include <condition_variable>
#include <atomic>

class NoResetEvent
{
public:
    NoResetEvent() : state(false) {}
    NoResetEvent(const NoResetEvent& other) = delete;

    void WaitOne() {
        std::unique_lock<std::mutex> lock(sync);
        while (!state) {
            underlying.wait(lock);
        }
    }

    void Set() {
        std::unique_lock<std::mutex> lock(sync);
        state = true;
        underlying.notify_all();
    }

private:
    std::condition_variable underlying;
    std::mutex sync;
    volatile bool state;
};

#endif

Looks good.

The only thing I would change is:

volatile bool _state;

into

 std::atomic<bool>  _state;

The issue with volatile is that it does not universally provide that guarantee that updates are visible to all threads (which std::atomic does).

Could simplify the wait() call.

    while (!_state) {
        _underlying.wait(lock);
    }

    // could be:
    _underlying.wait(lock,[&state](){return state;});

Personally I don't like prefix '_' on identifiers. The rules for using '_' as a prefix are complicated. Even if you know the rules not everybody that reads your code will.


Check if path exists directed graph, without using collection I have written code to find whether the path exists between source and destination in a directed graph. I have used BFS and instead of ArrayList or queue. I have written my own helper class.

Please let me know how the code can be improved.

public class Graph {

    private int numVertex;
    // Helper class, which can act like Adjacency List or Queue for BFS search.
    private HelperClass[] adjList;

    public Graph(int n) {
        numVertex = n;
        adjList = new HelperClass[n];
        for (int i = 0; i < n; i++) {
            adjList[i] = new HelperClass();
        }
    }

    public void setAdjacencyList(HelperClass[] adjList) {
        this.adjList = adjList;

    }

    // Assuming that source and destination will be in the range of vertices.
    public boolean isPathExists(int source, int destination) {
        if (source == destination)
            return true;
        // else do BFS until the destination is found or all the nodes are
        // visited.
        HelperClass hc = new HelperClass();
        boolean[] isVisited = new boolean[numVertex];
        hc.add(source);
        isVisited[source] = true;
        while (!hc.isEmpty()) {
            int current = hc.poll();
            if (current == destination) {
                return true;
            }
            int leftIndex = adjList[current].first;
            int rightIndex = adjList[current].last;
            while (leftIndex < rightIndex) {
                int temp = adjList[current].array[leftIndex++];
                if (!isVisited[temp]) {
                    hc.add(temp);
                }

            }
        }
        return false;
    }

    public static class HelperClass {
        // assuming the maximum array size will not be more than 10000.
        int[] array = new int[10000];
        int first = 0;
        int last = 0;

        public void add(int x) {
            array[last++] = x;
        }

        public int poll() {
            if (!isEmpty()) {
                return array[first++];
            } else {
                return -1;
            }
        }

        public boolean isEmpty() {
            return first == last ? true : false;
        }

    }

}

Performance

HelperClass used a fix amount memory, due to the predetermined array size. This looks like a waste.

Single responsibility principle

HelperClass is used in 2 different ways:

As a queue, that can grow at its end and shrink at its beginning
As an array to store the neighbors of each vertex

I suspect this dual use has lead to the terrible meaningless name of this class.

Instead of doing different things, it's recommended to design classes that do one thing and do it well. This is called the single responsibility principle, one of the first crucial considerations when creating a new class.

If you want to write your own queue, that's fine, but focus on its most important responsibility, which is efficient push, poll, isEmpty operations.

When your class has a single responsibility, naming becomes easier too, often obvious.

For the storage of neighbors, it's unclear what you really need. You haven't shared how they are initialized, so it's impossible to comment on that. A fixed array might be good enough, who knows.

Pointless Graph constructor

The constructor initializes the vertices empty, with no neighbors. That seems rather useless. Perhaps you want to move the functionality of the setAdjacencyList method inside the constructor.


TCP client and server supporting six simple commands For a class, I was given an assignment to code a simple TCP connection between a server and a client. Once the TCP handshake is done, the client sends inquiries to the server. It's a 2 second conversation, if that.
I need some feedback on my code. Please keep in mind that I am new to Python. The prof kinda threw the class to the wolves on this.

This is the server code:

import socket
import time
import sys
#from thread import *

def Main():
        s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        host = socket.gethostname()
        port = 8008

        s.bind((host, port))

        print ("Port, that's a wine right? " + str(port))

        s.listen(1)
        c, addr = s.accept()
        print ("200 SRP version 1.0 ready")
        print ("Connection from: " + str(addr))
        while True:
            data = c.recv(1024)
            data = str(data).upper()
            print ("sending: " + str(data))
            c.send(data)
            print ("from connected user: " + str(data))
            print ("Welcome, would you like a jelly baby?")

            if not data:
                        break
            if True:
                if data.startswith('HELO'):
                    try:
                        c.sendall("210 HELO " +  host + " This is Dalek High Command. You will obey all orders without question!")
                    except:
                        c.sendall("510 You are unauthorized! You will be exterminated!")
                    break
                elif data.startswith('REQTIME'):
                    try:
                        c.sendall ("220 I obey!  The time is " + time.strftime("%H%:M%:S"))
                    except:
                        c.sendall ("520 The time is unavailable!  Exterminate!")
                        break
                elif data.startswith('REQDATE'):
                        try:
                        c.sendall ("230 I obey!  The date is " + time.strftime("%Y/%m/%d"))
                    except:
                        c.sendall ("530 The date is unavailable!  Exterminate!")
                        break
                elif data.startswith('ECHO'):
                    try:
                        data = str(data).upper()
                        c.sendall ("240 Data received! " + data)
                    except:
                        c.sendall ("540 ECHO!  I cannot obey!  Assist!  Assist!  Cannot comply!  Exterminate!")
                        break
                elif data.startswith('REQIP'):
                    try:
                        c.sendall ("250 IP has been identified! " + host)
                    except:
                        c.sendall ("550 Alert!  I cannot obey! Your IP cannot be retrieved!")
                        break
                elif data.startswith('BYE'):
                    try:
                        c.sendall ("600 Alonzy! Bowties are cool!  See you later!")
                    except:
                        break
sys.exit()
c.close()

This is the client:

import socket

def Main():
        host = ''
        port = 8008

        s = socket.socket()
        s.connect((host, port))

        message = s.recv(1024)
        print('Received from server: ' + str(data))

        for x in range(0, 5):
            if x == 0:
                data = 'helo'
                s.send(data)
            elif x == 1:
                data = 'reqtime'
            elif x == 2:
                data = 'reqdate'
            elif x == 3:
                data = 'echo'
            elif x == 4:
                data = 'reqid'
            elif x == 5:
                data = 'bye'

        s.send(data)
        print(str(data))
        x = x + 1

I would recommend that you read PEP 8, the Python style guide.

PEP 8 says:

  Use 4 spaces per indentation level.

You use four spaces in some places, but in others you use eight.  At the very least you should be consistent, but it is best if you consistently use four spaces.  In fact, looking at your code, your indentation is completely shot.  I spy with my little eye an IndentationError in the elif data.startswith('REQDATE'): block.  Your break statements in those if blocks are not consistent either.

print("sending: " + str(data))

One line up you defined data as a string.  Python objects don't just drift into new types.  They don't change unless you change them.  You don't need the str part here.  Just do this:

print("sending: " + data)

You convert data to a string in several other places, but there also it is useless.  That's like translating a German sentence into English and then translating that sentence into English every time I want to use it.  What is the purpose of translating English into English?

if True:

True is always True, so that if block will always be executed.  Why do you use an if block if you always want to execute what is inside?

if data.startswith('HELO'):
    try:
        c.sendall(...)
    except:
        c.sendall(...)
    break
elif data.startswith('REQTIME'):
    try:
        c.sendall(...)
    except:
        c.sendall(...)
        break
...

You have some duplicate code there.  That is, it would be duplicate if you were consistent with what you wanted.  There are two things that change in these: The string given to the first c.sendall() and the string given to the second c.sendall().  I would create a dictionary:

messages = {
    'HELO': (
        "210 HELO " + host + " This is Dalek High Command.  You will obey all orders without question!",
        "510 You are unauthorized! You will be exterminated!"),
    'REQTIME': (
        "220 I obey!  The time is " + time.strftime("%H%:M%:S"),
        "520 The time is unavailable!  Exterminate!"),
    'REQDATE': (
        "230 I obey!  The date is " + time.strftime("%Y/%m/%d"),
        "530 The date is unavailable!  Exterminate!"),
    'ECHO': (
        "240 Data received! " + data.upper(),
        "540 ECHO!  I cannot obey!  Assist!  Assist!  Cannot comply!  Exterminate!"),
    'REQIP': (
        "250 IP has been identified! " + host,
        "550 Alert!  I cannot obey! Your IP cannot be retrieved!"),
}

for key, value in messages.items():
    if data.startswith(key):
        try:
            c.sendall(value[0])
        except:
            c.sendall(value[1])
        break
else:
    if data.startswith('BYE'):
        try:
            c.sendall ("600 Alonzy! Bowties are cool!  See you later!")
        except:
            break

sys.exit()
c.close()

You exit the program and then expect to be able to close c?  Perhaps that's a debugging call that you forgot to take out, but sys.exit() is unnecessary here.

for x in range(0, 5):
    if x == 0:
        data = 'helo'
        s.send(data)
    elif x == 1:
        data = 'reqtime'
    elif x == 2:
        data = 'reqdate'
    elif x == 3:
        data = 'echo'
    elif x == 4:
        data = 'reqid'
    elif x == 5:
        data = 'bye'

s.send(data)
print(str(data))
x = x + 1

I'm guessing that the s.send(data) and below lines were meant to be inside of the for loop.  If they were, there is a much simpler way to do this:

for data in ("helo", "reqtime", "reqdate", "echo", "reqid", "bye"):
    s.send(data)
    print(data)


Function to grab stdin, stdout, stderr of a child process I've managed to create a function to pipe the stdin, stdout and stderr of a child process and provide them as file descriptors to read and write to:

pid_t opencmd(pipes, path, argv)
    int *const pipes;
    const char *const path;
    char *const *const argv;
{
    if (!pipes || !path || !argv)
        goto error_in;

    int in[2];
    int out[2];
    int err[2];
    pid_t pid;

    /*
     * Creating pipes in between ...
     */
    if (pipe(in))
        goto error_in;
    if (pipe(out))
        goto error_out;
    if (pipe(err))
        goto error_err;

    /*
     * Starting actual processing ...
     */
    pid = fork();
    switch (pid) {
     case -1:       /* Error */
         goto error_fork;
     case 0:        /* Child */
         close(in[1]);
         close(out[0]);
         close(err[0]);

         dup2(in[0], 0);    /* redirect child stdin to in[0] */
         dup2(out[1], 1);   /* redirect child stdout to out[1] */
         dup2(err[1], 2);   /* redirect child stderr to err[1] */
         execv(path, argv); /* actual command execution */
         return -1; /* shall never be executed */
     default:       /* Parent */
         close(in[0]);  /* no need to read its stdin */
         close(out[1]); /* no need to write to its stdout */
         close(err[1]); /* no need to write to its stderr */

         pipes[0] = in[1];  /* Write to child stdin */
         pipes[1] = out[0]; /* Read child stdout */
         pipes[2] = err[0]; /* Read child stderr */
         return pid;
    }
/* This section is never be reached without a goto */
  error_fork:
    close(err[0]);
    close(err[1]);
  error_err:
    close(out[0]);
    close(out[1]);
  error_out:
    close(in[0]);
    close(in[1]);
  error_in:
    return -1;
}

A cleaning function for it:

int closecmd(pid, pipes)
    const pid_t pid;
    int *pipes;
{
    int status;

    close(pipes[0]);
    close(pipes[1]);
    close(pipes[2]);
    waitpid(pid, &status, 0);
    return status;
}

Are there any issues I'm overlooking with them?

There might be good use cases for goto,
but this is not one of them.

It's best when code reads from top to bottom.
The goto statements break that readable flow.
Since the jump-to labels are quite far,
I'm forced to scroll to the end to see what they do.
After I scrolled, I find some code using variables defined at the beginning,
which I haven't memorized earlier,
so now I have to go back to top.
Effectively I'm making several jumps back and forth to understand how this works,
and then a few more jumps to verify carefully that it should work well.

This is a nightmare, and it doesn't have to be.
If you replace the goto statements with the code at their jump-to labels,
the code becomes:

if (pipe(in)) {
    return -1;
}
if (pipe(out)) {
    close(in[0]);
    close(in[1]);
    return -1;
}
if (pipe(err)) {
    close(out[0]);
    close(out[1]);
    close(in[0]);
    close(in[1]);
    return -1;
}

This is a lot more readable:
everything I need to understand these guard statements are right there above.

You probably don't like the duplication when closing in,
and again later in the code if fork failed,
closing in and out.
It's true that code duplication is not good,
but I think it's the lesser of evils here,
and it's outweighed by the improvement in readability.

If you still disagree, and consider avoiding the duplication more important,
then you can improve this technique with goto by using more descriptive labels,
for example:

  close_err_and_out_and_in_and_return_with_failure:
    close(err[0]);
    close(err[1]);
  close_out_and_in_and_return_with_failure:
    close(out[0]);
    close(out[1]);
  close_in_and_return_with_failure:
    close(in[0]);
    close(in[1]);
  return_with_failure:
    return -1;

Avoid comments when possible.
Instead of a comment,
try to code in a way to make the comment unnecessary.
For example:

/* This section is never be reached without a goto */

This comment could be replaced with this statement:

return pid;

Not only this is short and sweet,
it has the great benefit that since it's code,
it is enforced: the lines below it will not be reached,
no matter what changes in the code above the return statement.

And we're kind of back to the pesky goto statements.
Without the goto statements,
this concern here about the comment and the logic wouldn't exist in the first place.


JavaScript function which checks the count of words in a string Task to accomplish: Made a function which counts the amount of words in a sentence.

Please don't put attention to the HTML. Is just for testing-purposes.

var sentence = document.querySelector('#sentence');
var exec = document.querySelector('#exec');
var result = document.querySelector('#result');

// ## THE ACTUAL FUNCTION ###########
// ## Rest is just for testing ######

// Reports the count of words which are
//   contained in assigned string.

// Parameter (sentence): Are string is
//   expected. The string doesn't have
//   to be empty. It has to consist of
//   other characters then just blanks.

// Return a number: The count of words
//   in the assigned string.
//   In case of an error -1 is returned.

function getCountOfWords(sentence) {
  if (sentence.length === 0 ||
      sentence.search(/\w+/) === -1)
    return -1;

  if (sentence.search(/\s+/) === -1)
    return 1;

  return (sentence.match(/(^|\s)\w+/g)).length;
}

// ## END ACTUAL FUNCTION ###########

function execHandler() {
  result.value = getCountOfWords(sentence.value);
}

exec.addEventListener('click', execHandler);
.wrap {
  margin: 20px auto;
  width: 900px;
}

.wrap label {
  display: inline-block;
  width: 150px;
}

.wrap div {
  margin: 20px 0;
}

      Sentence to check:

    <input type="text" id="sentence"
           value="one Two2 Three 4four five123"
           size="80" />

    <button id="exec">Exec</button>

      Count of words:

    <input type="text" id="result" />

I have tried to catch everything which could wrong.
But perhaps there are cases which I haven't thought about?

Are their better (more elegant) ways to achieve the described task?

This seems a bit like reinventing the wheel. I'm not sure on what you're trying to achieve here, but it doesn't look like you're accounting for hyphen separated words, so data-type counts as one word.

It looks like to me that this is just splitting on the space, and it could be replaced by String.split(' ').length

I'm sorry if I missed the point here. But I'm gonna need you to explain it better why you need such a function in the first place.

var sentence = document.querySelector('#sentence');
var exec = document.querySelector('#exec');
var result = document.querySelector('#result');

function execHandler() {
  result.value = sentence.value.split(' ').length;
}

exec.addEventListener('click', execHandler);
.wrap {
  margin: 20px auto;
  width: 900px;
}

.wrap label {
  display: inline-block;
  width: 150px;
}

.wrap div {
  margin: 20px 0;
}

      Sentence to check:

    <input type="text" id="sentence"
           value="one Two2 Three 4four five123"
           size="80" />

    <button id="exec">Exec</button>

      Count of words:

    <input type="text" id="result" />


Simple address book in C++ I am trying to increase my understanding of classes, so I tried to implement a simple address book program that holds ten names with that names corresponding address. The program works correctly and I am proud of what I have done so far, but I am looking to see if there is any aspect that can be made better, and or needs to be changed.

Class:

#pragma once
#include <iostream>
#include <string>

class Person
{
    friend std::istream &read(std::istream &is, Person add_book[10]);
    friend std::ostream &print(std::ostream &os, const Person add_book[10]);

public:
    Person& Person::sort_addbook(Person add_book[10]);
    Person();
    ~Person();
private:
    std::string name;
    std::string address;
};

Member functions:

#include "Person.h"

std::istream &read(std::istream &is,Person add_book[10]) {
    for (int count = 0; count < 10; count++) {
        is >> add_book[count].name; // get name
        std::getline(is,add_book[count].address); // get address from input
    }
    return is;
}

std::ostream &print(std::ostream &os, const Person add_book[10]) {
    for (int count = 0; count < 10; count++) {
        os << "Name: " << add_book[count].name << std::endl << "Adress: "
        << add_book[count].address << std::endl << std::endl; //output contents of address book
}
    return os;
}

//use bubble sort to sort address book alphabetically by name
Person& Person::sort_addbook(Person add_book[10]) {
    for (int pass = 0; pass < 10; pass++) {
        for (int i = 0; i < 9; i++) {
            if (add_book[pass].name < add_book[i].name)
                swap(add_book[pass].name, add_book[i].name);
        }
    }
    return *this;
}

Person::Person()
{
}

Person::~Person()
{
}

Main function:

#include <iostream>
#include "Person.h"

/*create address book that stores name + address,
sort that adress into alphabetical order, print address book.*/

int main() {

    Person address_book[10]; //stores name in column 1, address in column 2

    std::cout << "Enter name, and then address: " << std::endl;
    read(std::cin,address_book); //get user input

    address_book[10].sort_addbook(address_book); //sort addressbook
    std::cout << std::endl;

   print(std::cout, address_book); //print contents of addressbook

   system("pause");
}

It's a good effort; the important thing is to keep practicing and keep learning.  I see a number of things that may help you improve your program.

Don't overspecify member functions

The declaration of the sort_address member function in the header file is currently this:

Person& Person::sort_addbook(Person add_book[10]);

However, the Person:: is neither needed nor wanted here.  Instead, it can and should be written like this:

Person& sort_addbook(Person add_book[10]);

That make it both easier to read and satisfies the compiler when you turn up the warning level.

Let the compiler generate code where it can

The Person constructor and destructor do nothing.  The compiler could generate that for you, so you should let it.  You can simply omit both the declaration and the implementations for both of those.

Eliminate "magic numbers"

This code includes "magic numbers," that is, unnamed constants such as 10.  Generally it's better to avoid that and give such constants meaningful names.  That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "10" and then trying to determine if this particular 10 means the length of the array or some other constant that happens to have the same value.

Don't use #pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions.  By definition, all #pragma are non-standard.  For portable code, you should use the standard include guards.  Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Don't use system("pausse")

There are two reasons not to use system("cls") or system("pause").  The first is that it is not portable to other operating systems which you may or may not care about now.  The second is that it's a security hole, which you absolutely must care about.  Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything.  First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system.  Then rewrite the contents of those functions to do what you want using C++.  One simple replacement for that might be

void pause() {
    std::string nothing;
    std::cin >> nothing;
}

Prefer resizeable containers to fixed size

Rather than using a fixed size of 10, it may be preferable to use something like a std::vector.

Think carefully about object responsibilities

A Person is not an address_book.  A Person is an individual, while an address_book represents a collection of Person objects.  For that reason, the sort_addbook shouldn't really be a member function of the Person object.  I'd recommend implementing an AddressBook object instead and having it include a sort method.

Use a stream operators where practical

As with the point above, I'd recommend that the Person object use the << and >> operators rather than read and print.  For example, they might be implemented like this:

friend std::istream& operator>>(std::istream& in, Person &p) {
    in >> p.name;
    std::getline(in, p.address);
    return in;
}
friend std::ostream& operator<<(std::ostream& out, const Person &p) {
    return out << "Name: " << p.name
        << "\nAdress: " << p.address << '\n';
}

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream.  This can be time-consuming in a program with a lot of I/O and is rarely actually needed.  It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one.  Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Prefer standard algorithms

Instead of writing your own sort, you could use the std::sort instead.  To do that, you'll need to implement a comparison operator.  Here's one way to do that, as a member function of the Person class:

bool operator<(const Person& other) const {
    return name < other.name;
}

Putting it together

Here's a way that this could be implemented.  Note that this code does not use std::vector and keeps the fixed size, but it does use all of the other suggestions:

int main() {
    constexpr unsigned ADDRESS_COUNT{10};
    Person address_book[ADDRESS_COUNT];

    std::cout << "Enter name, and then address: " << std::endl;
    for (size_t i = 0; i < ADDRESS_COUNT; ++i) {
        std::cin >> address_book[i];
    }

    std::sort(&address_book[0], &address_book[ADDRESS_COUNT]);
    for (size_t i = 0; i < ADDRESS_COUNT; ++i) {
        std::cout << address_book[i] << '\n';
    }
}


Count unique digits in a given number How do I find the number of unique digits in a given number.

e.g
if 'input1' is 252 it must return 1, since 5 is the non-repeating digit;
if 'input1' is 25000 it must return 2, since digits 2 and 5 are the only non-repeating ones.

Is there a better way to solve this problem?

// COUNT UNIQUE DIGITS

int nonRepeatDigitsCount(int input1){
int i=0,j,k;
int n=input1;
int count=0,check,c2=0;

// FINDING NUMBER OF DIGITS ON THE GIVEN NUMBER
while(n>0){

    n=n/10;
    c2++;
    }
//CREATING ARRAY OF THE 'c2' SIZE
int arr[c2];
n=input1;
//STORING INDIVIDUAL DIGITS IN THE ARRAY
while(n>0)
{
    arr[i]=n%10;
    n=n/10;
    i++;

}
// CONDITION TO FIND NON REPEATING DIGITS
for(j=0;j<c2;j++)
{
    check=0;

    for(k=0;k<c2;k++)
    {
          if(j!=k)
          {
           if(arr[j]==arr[k])
           {
            check++;
           }
          }

    }

    if(check==0)
     {
        count++;
     }
 }

return count;
}

This is needlessly complicated and inefficient. Rather than making an array of all digits in the array, you should make an array of counted occurrences. Since a digit can have a value 0 to 9, this array size will be 10. For each occurrence, increase the value of the index in the array corresponding to the digit by 1.

This also means that you can count non-repeating characters on the fly while iterating through the number. When the number of occurrences is exactly 1, it should be counted. If more than that, it shouldn't.

This makes the code behave the same no matter the number of digits used as input.

Example:

#include <stdio.h>

int non_repeating (int val)
{
  if(val == 0) // special case, value 0 gives 1 digit
  {
    return 1;
  }

  int digit_count[10]={0};
  int non_rep=0;

  for(; val!=0; val/=10)
  {
    int i = val%10;
    digit_count[i]++;

    if(digit_count[i]==1)
    {
      non_rep++;
    }
    else if(digit_count[i]==2)
    {
      non_rep--;
    }
  }

  return non_rep;
}

int main (void)
{
  printf("%d %d\n", 252,        non_repeating(252));
  printf("%d %d\n", 25000,      non_repeating(25000));
  printf("%d %d\n", 1234567890, non_repeating(1234567890));
  printf("%d %d\n", 0,          non_repeating(0));
}

Output:

252 1
25000 2
1234567890 10
0 1


Calling an Excel User Form from Outlook I have an Outlook Module that contains the VBA Code below. The VBA code opens an Excel Add-In called TMAPAddIn.xlam, which then displays a User Form. I exported the Module and gave it to other colleagues so that they can import it to their Outlook, this allowed them to make a custom button on their Outlook Quick Access Toolbar.

The code works great, takes about 5 seconds for the User Form to display and the code executes without a problem. The file is open as ReadOnly to ensure multiple users can run the add in at the same time, without ever having to worry about them modifying its contents. I would like to know if my code can be written better for speed / efficiency.

I've written this code only for purposes of ensuring everyone runs and opens the latest version of TMAPAddIn.xlam, unfortunately I don't have the resources or time to develop a COM Add-In.

Code

Sub LaunchTMAP()
Dim ExApp As Object
Set ExApp = CreateObject("Excel.Application")

Dim informationalBox As UserForm2
Set informationalBox = New UserForm2
informationalBox.Show 'letting the user know the workbook is downloading so they dont think Outlook froze

Dim ExWbk As Workbook
Set ExWbk = ExApp.Workbooks.Open("Z/TMAPAddIn.xlam", ReadOnly:=True) 'Z is a network drive

informationalBox.Hide

ExWbk.Application.Run "Module1.example"

ExApp = Nothing
ExWbk = Nothing
End Sub

I would propose rearranging and simplifying the code down to this:

Sub LaunchTMAP()
    '' let user know the workbook is downloading so they dont think Outlook froze
    Dim infoBox As New UserForm2
    Call infoBox.Show

    ''  open excel, open the workbook, run the subroutine, then close excel
    Dim xl As New Excel.Application
    Dim wb As Excel.Workbook
    Set wb = ExApp.Workbooks.Open("Z/TMAPAddIn.xlam", ReadOnly:=True) 'Z is a network drive
    Call xl.Run("Module1.example")
    Call xl.Quit

    ''  hide the userform
    Call infoBox.Hide
End Sub

This should not make your code run any faster, but it should make it feel more responsive. By moving the infoBox dim and show to before the new excel window is openned, you make the infoBox appear earlier. This is because the 5 second delay comes from opening excel. I've added a quit call to make sure that you are closing out the excel window when oyu are done with it. I've also gone switched you away from having the CreateObject call, as you appear to already be using the "Microsoft Excel XX.0 Object Library" reference. I also used Dim ... As New .. notation where posible to reduce the number of lines.


Parsing expressions with combinators I'm very new to Haskell's ReadP library, and the entire concept of parser combinators, so I was wondering whether there are better ways to do some things in this program:

import Text.ParserCombinators.ReadP
import Control.Applicative
import Data.List

data Operator = Add | Subtract

instance Show Operator where
    show Add = "+"
    show Subtract = "-"

instance Read Operator where
    readsPrec _ "+" = [(Add, "")]
    readsPrec _ "-" = [(Subtract, "")]
    readsPrec _ _ = []

data Expression = Number Int
                | Infix { left :: Expression, op :: Operator, right :: Expression }

instance Show Expression where
    show (Number x) = show x
    show (Infix left op right) = "(" ++ (show left) ++ " " ++ (show op) ++ " " ++ (show right) ++ ")"

digit :: ReadP Char
digit = satisfy $ \char -> char >= '0' && char <= '9'

number :: ReadP Expression
number = fmap (Number . read) (many1 digit)

operator :: ReadP Operator
operator = fmap read (string "+" <|> string "-")

expression :: ReadP Expression
expression = do
    skipSpaces
    left <- number
    skipSpaces
    op <- Control.Applicative.optional operator
    case op of
        Nothing -> return left
        Just op -> do
            skipSpaces
            right <- expression
            return (Infix left op right)

parseExpression :: String -> Maybe Expression
parseExpression input = case readP_to_S expression input of
    [] -> Nothing
    xs -> (Just . fst . last) xs

The main area where I'm looking for improvements is the expression function, and specifically the things which seem improvable are the repeated calls to skipSpaces and the case expression to check whether an operator was parsed, but of course if you notice anything else that would be helpful too!

First, a standard approach for dealing with the skipSpaces issue is to define a higher-order parser combinator, traditionally called lexeme:

lexeme :: ReadP a -> ReadP a
lexeme p = p <* skipSpaces

Here, lexeme takes a space-naive parser p, and converts it into a new parser that parses whatever p was planning to parse, and then reads and discards any trailing spaces.  You use lexeme in the definitions of any of your parsers that would reasonably be assumed to read a complete "lexeme" and ignore any trailing space.  For example, number should be a lexeme parser:

number :: ReadP Expression
number = lexeme $ fmap (Number . read) (many1 digit)

So should operator, though obviously not digit!  expression won't need to use lexeme, because we'll arrange to end it with a lexeme parser.

It's also helpful to define a symbol parser which is essentially string that ignores trailing spaces:

symbol :: String -> ReadP String
symbol = lexeme . string

Consistently used, lexeme (and symbol) will deal with all unwanted spaces, other than any leading spaces at the very start of your parse.  If you have a non-recursive "top level" grammar production, like say a parser for program :: ReadP Program, then you would probably deal with them there.  In your example, you don't have such a production (e.g., expression is recursive), so you'd stick an extra skipSpaces in parseExpression.  This is also a good place to put eof to make sure there isn't any trailing material that you aren't parsing:

parseExpression :: String -> Maybe Expression
parseExpression input = case readP_to_S (skipSpaces *> expression <* eof) input of
    [] -> Nothing
    xs -> (Just . fst . last) xs

Second, your use of a Read instance for parsing your operators is very unusual.  It would be more standard to make it a helper in the operator parser, writing something like:

operator :: ReadP Operator
operator = readSymbol <$> (symbol "+" <|> symbol "-")
  where readSymbol "+" = Add
        readSymbol "-" = Subtract

(though an even more standard version is given below).

Third, in expression, you can avoid the case construct by using alternation (<|>) like so:

expression' :: ReadP Expression
expression' = do
    left <- number
    (do op <- operator
        right <- expression
        return (Infix left op right)
     <|> return left)

This would be the standard approach for non-parallel parser libraries (e.g., Parsec or Megaparsec).  For ReadP, it's better to replace the (<|>) operator with the ReadP-specific (<++) operator to avoid also following the unwanted second parse in parallel.  Beware that (<++) has higher precedence than (<|>), so some extra parentheses might be needed if it's being used in combination with other operators, as in the examples below.

Fourth, you've probably noticed my use of the applicative operators <* and *> and the alias <$> for fmap in the code above.  It is very common to use these -- plus the additional applicative operator <*> and sometimes the operators <**> or <$ -- in parsers.  Once you get used to them, they tend to lead to less cluttered code.

For example, a more standard way of writing expression would be:

expression' :: ReadP Expression
expression' =     Infix <$> number <*> operator <*> expression
              <|> number

or the slightly more efficient solution:

expression :: ReadP Expression
expression = do
  left <- number
  (Infix left <$> operator <*> expression) <++ return left

Note that, in the context of parsers, an expression like f <$> p <*> q means "try to run the parser p, and then the parser q; assuming they both succeed, pass their return values to f".  In other words, that Infix expression is essentially:

Infix left op right

where op is the return value from the parser operator and right is the return value from the parser expression.

Similarly, the standard way of writing operator is actually:

operator :: ReadP Operator
operator = Add <$ symbol "+" <|> Subtract <$ symbol "-"

This one requires an additional word of explanation.  The operator <$ is kind of an odd duck.  It's type signature is:

(<$) :: a -> f b -> f a

but in the context of parsers specifically, the meaning of x <$ p is "try to run the parser p; if it succeeds, ignore its return value and return x".  Basically, it's used to replace the return value of a parser that's used only for its success or failure and not its return value.

Note that these versions of expression, like your original version, treat the operators as right associative.  This may be a problem if you're trying to parse "1-2-3" as equivalent to "(1-2)-3" instead of "1-(2-3)".

A few additional minor points:

isDigit is a more readable name for \c -> c >= '0' && c <= '9'
munch1 is more efficient than many1 (satisfy xxx), so I'd redefine number to use it
for testing, it's probably a good idea to have a parseExpressions function that looks at all the parses
for production, it's probably a good idea to check for ambiguous parses and do something about it, rather than (fairly arbitrarily) selecting the last parse in the list

With all of these suggestions implemented, the final version would look something like:

{-# OPTIONS_GHC -Wall #-}

import Data.Char (isDigit)
import Control.Applicative
import Text.ParserCombinators.ReadP (eof, munch1, ReadP, readP_to_S,
                                     skipSpaces, string, (<++))

data Operator = Add | Subtract

data Expression = Number Int
                | Infix { left :: Expression, op :: Operator, right :: Expression }

lexeme :: ReadP a -> ReadP a
lexeme p = p <* skipSpaces

symbol :: String -> ReadP String
symbol = lexeme . string

number :: ReadP Expression
number = lexeme $ Number . read <$> munch1 isDigit

operator :: ReadP Operator
operator = Add <$ symbol "+" <|> Subtract <$ symbol "-"

expression :: ReadP Expression
expression = do
  x <- number
  (Infix x <$> operator <*> expression) <++ return x

top :: ReadP Expression
top = skipSpaces *> expression <* eof

parseExpressions :: String -> [(Expression, String)]
parseExpressions = readP_to_S top

parseExpression :: String -> Maybe Expression
parseExpression input = case parseExpressions input of
    [] -> Nothing
    [(x,"")] -> Just x
    _ -> error "ambiguous parse"


Printing out a diamond with a user-inputted size I made a program that prints out a diamond with n lines. n is equal to whatever the user inputted. I know I overuse the ternary or conditional operator, but in my opinion, it makes the code more compact, and since I am the only one reading my code, readability doesn't matter to me. My friend challenged me to write one of these programs in under 50 lines of code, so that is why I am cramming everything together.

#include <iostream>

int main() {

    int lineNum = 1, lines,  stars = 1, spaces = 0;

    std::cin >> lines;

    // if lines is even, split it equally, if not, split in half and add 1
    while (lineNum <= ((lines % 2 == 0) ? (lines / 2) : (lines / 2) + 1)) {

        for (int c = (((lines % 2 == 0) ? (lines / 2) : (lines / 2) + 1) - lineNum); c != 0; c--)
            std::cout << " ";

        for (int c = 0; c < stars; c++)
            std::cout << "*";

        std::cout << "\n";

        lineNum++;
        stars += 2;

    }

    stars = (lines % 2 == 0) ? stars - 2 : stars - 4;
    spaces = (lines % 2 == 0) ? spaces = 0 : spaces = 1;

    while (lineNum <= lines) {

        for (int c = 0; c < spaces; c++)
            std::cout << " ";

        for (int c = stars; c != 0; c--)
            std::cout << "*";

        std::cout << "\n";
        stars -= 2;
        spaces++;
        lineNum++;

    }

}

I see a number of things that could help you improve your code.

Don't disregard readability

Readability of code is always of use and always of interest, even if you're the only one who ever sees the code.  Since, by definition, you're asking for a code review by posting here, even that assumption is clearly faulty, because every reviewer is going to have to read and understand your code to comment usefully on it.  Even if you really were the only person to ever look at the code, if you decide to modify it some time later, readability will be important to you.

Decompose your program into functions

All of the logic here is in main in one rather long and dense chunk of code.  It would be better to decompose this into separate functions.

Don't further clutter trinary operators

The code currently includes this line:

spaces = (lines % 2 == 0) ? spaces = 0 : spaces = 1;

The problem is that the spaces = 0 and spaces = 1 on the right side are wholly uneccesary.  If you must use trinary operators, (and I question that in this case), use them correctly:

spaces = (lines % 2 == 0) ? 0 : 1;

Simpler, and better in my view, would be this:

spaces = lines & 1;

Rethink your algorithm

The biggest improvements are often found in simply rethinking and reimplementing the basic algorithm. In this case, each line is 0 or more spaces, followed by 1 or more stars.  For each increasing line in the first half, the number of spaces decrements by one and the number of stars increments by two.  A little bit of examination with paper and pencil will quickly yield algebraic expressions for the number of spaces and number of stars for each line in the top half of an n-line diamond pattern.  The bottom half is simply the reverse of the top half.  This suggests a simple algorithm.

Putting it all together

Your original program, as posted, was 44 lines long.  Using all of these hints, and with better readability, here it is in exactly half that many lines:

#include <iostream>

void multiprint(unsigned n, char ch) {
    for ( ; n; --n)
        std::cout << ch;
}

void line(unsigned n, unsigned maxline) {
    unsigned half = (maxline-1)/2;
    if (n > half) n = maxline - n - 1;
    multiprint(half - n, ' ');
    multiprint(n*2 + 1, '*');
    std::cout << '\n';
}

int main() {
    int lines;
    std::cin >> lines;

    for (int i =0; i < lines; ++i)
        line(i, lines);
}


Performing batch insertion for a single row I've following code that insert single row in if (rows == 1) { part and inside loop it is batch insert.
So I'm thinking to skip single insert and have it in batch insert code.

private void jButton1ActionPerformed(java.awt.event.ActionEvent evt) {

        int rows = jTable.getRowCount();
        if (rows == 1) {
            String itemName = (String) jTable.getValueAt(0, 0);
            int itemQty = (int) jTable.getValueAt(0, 1);
            Double itemPrice = (Double) jTable.getValueAt(0, 2);

            Items items = new Items();
            items.setName(itemName);
            items.setPrice(itemPrice);
            items.setQty(itemQty);
            items.setTransactionNumber(manager.getTransNo());
            manager.saveItems(items);

Above part insert single record if there's only one row in a JTable
Below in else part inside loop performed batch insert if JTable contains more than one row.

        } else {
            for (int i = 0; i < 1; i++) {
                String itemName = (String) jTable.getValueAt(i, 0);
                int itemQty = (int) jTable.getValueAt(i, 1);
                Double itemPrice = (Double) jTable.getValueAt(i, 2);
                Items items = new Items();
                items.setName(itemName);
                items.setPrice(itemPrice);
                items.setQty(itemQty);
                items.setTransactionNumber(manager.getTransNo());
                int max = items.getTransactionNumber();
                manager.saveItems(items);

And here in a second loop it does perform insertion after first row in a JTable when int i = 1.

for (i = 1; i < rows; i++) {
            String itemsName = (String)
            jTable.getValueAt(i, 0);
            int itemsQty = (int) jTable.getValueAt(i, 1);
            Double itemsPrice = (Double)
            jTable.getValueAt(i, 2);

            items.setName(itemsName);
            items.setPrice(itemsPrice);
            items.setQty(itemsQty);
            items.setTransactionNumber(max);
            manager.saveItems(items);
            }
        }
    }
}

I was thinking to save all rows without considering is either only one row or more.
So my modified version would not have if (rows == 1) {....} part
Will have only for (int i = 0; i <= 0; i++) {
So if there will have only one row in a JTable would this modified version can be cause of lower performance?

Short answer to your question: No it's the same performance wise.

Full review:

My first concern with your code is the double for loop that uses the same looping variable i. This makes it harder to understand when you see an i inside the inner for loop.

My other major concern is that the only difference between the code before the inner for loop and the code inside the inner loop is setting the variable max which I had no idea what it should be at first sight. Let's start by renaming that to transactionNumber. And since this is independent of the loop itself let's initialise that from the manager right above the loops instead:

int transactionNumber = manager.getTransNo();
for (int i = 0; i < 1; i++) {
    String itemName = (String) jTable.getValueAt(i, 0);
    int itemQty = (int) jTable.getValueAt(i, 1);
    Double itemPrice = (Double) jTable.getValueAt(i, 2);
    Items items = new Items();
    items.setName(itemName);
    items.setPrice(itemPrice);
    items.setQty(itemQty);
    items.setTransactionNumber(transactionNumber);
    manager.saveItems(items);

    for (i = 1; i < rows; i++) {
        String itemsName = (String) jTable.getValueAt(i, 0);
        int itemsQty = (int) jTable.getValueAt(i, 1);
        Double itemsPrice = (Double) jTable.getValueAt(i, 2);

        items.setName(itemsName);
        items.setPrice(itemsPrice);
        items.setQty(itemsQty);
        items.setTransactionNumber(transactionNumber);
        manager.saveItems(items);
    }
}

Now with a closer look there's a second difference. You only instantiate items once and overwrite that with the setters. I don't think this is a good idea. You should probably create a new items object each time with it's own name, price, quantity and save that independent of any previous values. This means we can just combine the 2 for loops into 1 like so:

int transactionNumber = manager.getTransNo();
for (int i = 0; i < rows; i++) {
    String itemName = (String) jTable.getValueAt(i, 0);
    int itemQty = (int) jTable.getValueAt(i, 1);
    Double itemPrice = (Double) jTable.getValueAt(i, 2);
    Items items = new Items();
    items.setName(itemName);
    items.setPrice(itemPrice);
    items.setQty(itemQty);
    items.setTransactionNumber(transactionNumber);
    manager.saveItems(items);
}

If you now take a close look at what would happen if you only had 1 row you can see that the code inside the loop gets executed exactly 1 time with the i replaced by a 0. This is exactly the same code you have inside your if(rows == 1) block so you can remove that check and only keep the for loop.

Depending on how the Items are used in the rest of your code I also strongly suggest to remove the setters and pass any needed parameters with the constructor instead. This results in the following implementation of the entire method:

private void jButton1ActionPerformed (java.awt.event.ActionEvent evt){
    int transactionNumber = manager.getTransNo();
    for (int i = 0; i < rows; i++) {
        String itemName = (String) jTable.getValueAt(i, 0);
        int itemQty = (int) jTable.getValueAt(i, 1);
        Double itemPrice = (Double) jTable.getValueAt(i, 2);
        Items items = new Items(transactionNumber, itemName, itemPrice, itemQty);
        manager.saveItems(items);
    }
}

The only improvement you should consider is to rename jTable to something more meaningful.


Capture worksheet formulas in VBA format I had been searching for a simple way to capture worksheet formulas in VBA format. I came up with a solution below, which I wanted to share.

With any luck this could be useful to someone down the road. Suggestions for improvement are welcome.

Public Const vbQuadrupleQuote As String = """""" 'represents 2 double quotes for use in VBA R1C1 formulas ("")
Public Const vbDoubleQuote As String = """" 'represents 1 double quote (")
Public Const vbSingleQuote As String = "'" 'represents 1 single quote (')

Sub CaptureFormulas()  'simplifies the capturing of worksheet formulas in VBA format

'Peter Domanico, May 2017

'Steps:

'(1) place this script in your personal macro workbook
'(2) open Immediate Window in VBA (Control + G)
'(3) run this script and follow prompts
'(4) a With statement containing formulas for your selection will be printed to the Immediate Window
'(5) you can use this With statement in any script

Dim ws As String
Dim rng As Range
Dim MyString As String
Dim MyColumn As Variant
Dim MyRow As Variant
Dim LastRow As String
Dim MyRange As String
Dim MyFormula As String

'set worksheet string
ws = "Activesheet" 'change this as needed

'error handling
On Error GoTo OuttaHere

'select range
Set rng = Application.InputBox("Select range to capture", ": )", Type:=8)

'determine formula type
MyQuestion = MsgBox(Prompt:="Fill formulas to last row?", _
            Buttons:=vbYesNo, Title:="???")

Debug.Print "Dim ws as Worksheet" 'change this as needed
Debug.Print "Set ws = Activesheet" 'change this as needed
Debug.Print "LastRow = ws.Cells(Rows.Count,1).End(xlUp).Row" 'change this as needed
Debug.Print "With ws" 'change this as needed

For Each rng In rng
    MyColumn = rng.Column
    CurrentRow = rng.Row
    Select Case MyQuestion
        Case vbYes
            LastRow = "LastRow"
        Case vbNo
            LastRow = CurrentRow
    End Select
    MyRange = ".Range(.Cells(" & CurrentRow & "," & MyColumn & "),.Cells(" & LastRow & "," & MyColumn & "))="
    MyFormula = rng.FormulaR1C1
    MyFormula = Replace(MyFormula, vbDoubleQuote, vbQuadrupleQuote)
    MyString = MyRange & vbDoubleQuote & MyFormula & vbDoubleQuote
    Debug.Print MyString
Next rng

Debug.Print "End With"

MsgBox "See Immediate Window in VBA", vbInformation, ": )"

OuttaHere:
End Sub

First design problem is that you're outputting to the immediate pane, which isn't meant to hold data like this - it will only ever display up to 255 lines, which means as soon as your range has more than 250-some rows, your code stops being useful.

First code problem is that Option Explicit is missing, and you have undeclared variables:

MyQuestion (which is really an answer, since it's a vbMsgBoxResult) isn't declared.
CurrentRow isn't declared.

Then you have declared variables that aren't used anywhere:

MyRow isn't used.

You're not validating rng, so if the InputBox gets cancelled, you get a run-time error 424 "Object Required" and jump OuttaHere - which isn't too bad, except for the fact that getting valid user input is a concern in its own right, and really belongs in its own dedicated function.

    Set rng = SelectRangeToCapture
    If rng Is Nothing Then Exit Sub

'@Description "Prompts for a Range. Returns Nothing if no selection is made."
Private Function SelectRangeToCapture() As Range
    On Error Resume Next
    Set SelectRangeToCapture = Application.InputBox("Select range to capture", ": )", Type:=8)
    Err.Clear
    On Error GoTo 0
End Function

It doesn't feel right that you need to check MyQuestion for every single iteration of that loop - the value is the same every time, you're wasting cycles here. MyQuestion should be named fillToLastRow and be a Boolean:

fillToLastRow = MsgBox(...) = vbYes

And then the LastRow assignment can be a simple IIf function result:

LastRow = IIf(fillToLastRow, "LastRow", CStr(currentRow))

Note the explicit type conversion for currentRow, from Long to String.

The generated code needs to be indented and properly spaced. Lack of whitespace between operators and operands can easily turn into compile errors, because of how VBA interprets type hints - not necessarily here in this code, but lack of whitespace turning & concatenation operator into a & type hint on a variable or literal value is a quite recurrent, easily avoided question on Stack Overflow.

The LastRow logic assumes column A contains data: the reference column should be based on rng instead.

I'm also a bit puzzled about why you would want to use R1C1 notation in the formulas, but maybe it's just because I never needed to use it in 15 years.

Why not go all the way, reference the VBIDE type library and actually output your generated code to an actual procedure, in an actual module? That way you wouldn't be limited to 255 lines. Although.. I wonder if that's a good idea, given VBA won't even compile a procedure that's more than 10,000 lines, or a module with more than 65,535 lines - in other words, rng.Count needs to be capped and validated, otherwise you either lose data, or generate code that can't even compile.

In general I don't like VBA code that requires frequent modifications. Code should be written to resist change - yours embraces change with arms wide open, pretty much as part of its own specification, including instructions-in-comments.

Speaking of comments... Comments should say why, not what. These comments say nothing that the code doesn't already say:

'error handling
On Error GoTo OuttaHere

'select range
Set rng = Application.InputBox("Select range to capture", ": )", Type:=8)

'set worksheet string
ws = "Activesheet"

One last note about naming: OuttaHere isn't normally a label you'd find in professionally-written code. CleanExit would be more appropriate. MyXxxxx identifiers are annoying, too: such names don't convey much information, other than a vague data type that belongs to something (or someone?). Take the time to name things, meaningfully.

MyRange holds an assignment instruction, not a Range.
MyFormula holds the Excel formula in the current cell.
MyString holds a VBA instruction.

And so on.