Dual state button algorithm in C I wrote code for determining the push button state whether it is long pressed or not. This function is called by timer interrupt routine every 1ms.

But it seems really dumb. How can I make it shorter and more efficient according to both readability and professional rules? What should I add or change to get double Tap pressing information with this  code?

void HAL_TIM_PeriodElapsedCallback(TIM_HandleTypeDef *htim)
{
    /*button pressed and count*/
    if(!HAL_GPIO_ReadPin(GPIOC, GPIO_PIN_13))
    {
        usTick++;
    }
    /*not pressed*/
    else
    {
        if( usTick > 1000){
            ButtonState.PressedState = LongPressed;
            HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);
            usTick = 0;
        }
        else if( usTick >350){
            ButtonState.PressedState = ShortPressed;
            HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);
            usTick = 0;
        }
        usTick = 0;
    }
}

... how can I make it shorter and more efficient according to both readability and professional rules?

Calling

usTick = 0;

is superfluous in the else branches sub-conditions. That can be rewritten like:

/*not pressed*/
else
{
    if( usTick > 1000){
        ButtonState.PressedState = LongPressed;
        HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);
    }
    else if( usTick >350){
        ButtonState.PressedState = ShortPressed;
        HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);
    }
    // Calling those statements Once is enough here
    usTick = 0;
}

Checking the minimum limit and reducing the whole sub-conditions with a ternary condition operator (as mentioned in @David Isla's answer), the whole code could be boiled down to a few lines:

/*not pressed*/
else
{
    if( usTick > 350) {
         ButtonState.PressedState = usTick > 1000 ? LongPressed : ShortPressed;
         HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);
    }
    usTick = 0;
}

  What should I add or change to get double Tap pressing information with this code?

You might consider to put any data updating operations into critical sections (or other non-interruptible code paths):

void HAL_TIM_PeriodElapsedCallback(TIM_HandleTypeDef *htim)
{
    /*button pressed and count*/
    if(!HAL_GPIO_ReadPin(GPIOC, GPIO_PIN_13))
    {
        START_CRITCAL_SECTION();
        usTick++;
        END_CRITCAL_SECTION();
    }
    /*not pressed*/
    else {
        START_CRITCAL_SECTION();
        if( usTick > 350) {
             ButtonState.PressedState = usTick > 1000 ? LongPressed : ShortPressed;
             HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);
        }
        usTick = 0;
        END_CRITCAL_SECTION();
    }
}

where START_CRITCAL_SECTION() disables all interrupts, and END_CRITCAL_SECTION() reenables their former state.

This may help, that any co-routines or threads will always read consistent values for ustick or the GPIO_PIN_5 state.


Character Picture Grid exercise - automatetheboringstuff Regarding the Character picture exercise located at the end the following page:
https://automatetheboringstuff.com/chapter4/

  Say you have a list of lists where each value in the inner lists is a
  one-character string, like this:

grid = [['.', '.', '.', '.', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['.', 'O', 'O', 'O', 'O', 'O'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['.', '.', '.', '.', '.', '.']]

  You can think of grid[x][y] as being the character at the x- and
  y-coordinates of a “picture” drawn with text characters. The (0, 0)
  origin will be in the upper-left corner, the x-coordinates increase
  going right, and the y-coordinates increase going down.

  Copy the previous grid value, and write code that uses it to print the
  image.

..OO.OO..
.OOOOOOO.
.OOOOOOO.
..OOOOO..
...OOO...
....O....

I wrote the following code, any feedback is appreciated.

grid = [['.', '.', '.', '.', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['.', 'O', 'O', 'O', 'O', 'O'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['.', '.', '.', '.', '.', '.']]

for i in range(6):
    for a in range(9):
        if a < 8:
            print(grid[a][i], end="")
        else:
            print(grid[a][i])

Your code is pretty good. It's what I'd expect the author of the book to have wanted their readers to write.

Good job!

In terms of making the code more understandable/maintainable you can get the same functionality using zip, *args, ''.join and a single print.

for column in zip(*grid):
    print(''.join(column))


Setting image margins and location based on orientation This code is for Android but I guess anyone can take a chance at it.

It does what I want it to do. Just that I am not comfortable with its deep level of nesting and too much of value swapping.

Can this be optimized for readability or in compactness?

for(int imagenum, imagenum <76, imagenum++){

    if (orientation.contentEquals("landscape")
            || orientation.contentEquals("reverse landscape")) {
        params.height = imageheight;
        params.width = imagewidth;
        imageButton.setImageResource(R.drawable.imageback);
        if (imagenum <= 38) {

            params.leftMargin = imagenum * xspaceforeachimage;
            params.topMargin = 0;
        } else {
            params.leftMargin = (imagenum - 39) * xspaceforeachimage;
            params.topMargin = topMargin;
        }
    } else if (orientation.contentEquals("portrait")
            || orientation.contentEquals("reverse portrait")) {
        params.height = imagewidth;
        params.width = imageheight;
        imageButton.setImageResource(R.drawable.imagebacklaid);
        if (imagenum <= 38) {
            params.leftMargin = 0;
            params.topMargin = imagenum * xspaceforeachimage;
        } else {
            params.leftMargin = topMargin;
            params.topMargin = (imagenum - 39) * xspaceforeachimage;
        }
    }
}

This doesn't save many lines of code, but I think it's more readable and maintainable:

boolean landscape = orientation.contains("landscape");

for(int imagenum = 0; imagenum <76; imagenum++){

    /* Missing code that assigns params and imageButton using imageNum? */

    int shortSideMargin = imagenum * xspaceforeachimage;
    int longSideMargin = 0;
    if (imagenum >= 39)  { //new row or column, shift back and over
        shortSideMargin -= 39 * xspaceforeachimage;
        longSideMargin = topMargin;
    }

    if (landscape) {
        params.height = imageheight;
        params.width = imagewidth;
        params.leftMargin = shortSideMargin;
        params.topMargin = longSideMargin;
        imageButton.setImageResource(R.drawable.imageback);
    } else {
        params.height = imagewidth;
        params.width = imageheight;
        params.leftMargin = longSideMargin;
        params.topMargin = shortSideMargin;
        imageButton.setImageResource(R.drawable.imagebacklaid);
    }
}


Function that returns the earliest time point when a worker is allowed to start duty The task is to provide a function that returns the earliest time point when a worker is allowed to start duty.

The function uses two parameters:

Time stamp when the rest starts
The number of local nights which is necessary according to a table

I wonder if there is anything wrong with void convertTimeStructToLocalTime(TimeStamp * const timeStructToConvert) in the code. Is it OK to use constant pointer to the TimeStamp structure in convertTimeStructToLocalTime?

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

//has tm_isdst flag to be used for daylight saving time
typedef struct tm TimeStamp;
typedef enum {false, true} bool;

TimeStamp getEndOfRest(TimeStamp startOfRest, int neededLocalNights);
bool isNeededLocalNightsInTheRange(int neededLocalNights);
void convertTimeStructToLocalTime(TimeStamp * const timeStructToConvert);

//to make tests easier
void wrapGetEndOfRest(TimeStamp startOfRest, int neededLocalNights);

int main(void) {
    int y=2016, m=2, d=27;
    int y2=2016, m2=12, d2=28;
    int daysDelta = 0;

    TimeStamp t = { .tm_year=y-1900, .tm_mon=m-1, .tm_mday=d, .tm_hour=12, .tm_min=5, .tm_sec=10 };
    TimeStamp t2 = { .tm_year=y2-1900, .tm_mon=m2-1, .tm_mday=d2, .tm_hour=9, .tm_min=59, .tm_sec=5};

    //tests
    for(daysDelta = -1; daysDelta < 8; daysDelta++){
        convertTimeStructToLocalTime(&t);
        printf("%s is the start add %d days\n", asctime(&t), daysDelta);
        wrapGetEndOfRest(t, daysDelta);
    }

    for(daysDelta = -1; daysDelta < 8; daysDelta++){
        convertTimeStructToLocalTime(&t2);
        printf("%s is the start add %d days\n", asctime(&t2), daysDelta);
        wrapGetEndOfRest(t2, daysDelta);
    }

    return EXIT_SUCCESS;
}

//Since the minimum for neededLocalNights is 2 according to the table the time of departure is always 6 am
TimeStamp getEndOfRest(TimeStamp startOfRest, int neededLocalNights){
    startOfRest.tm_mday += neededLocalNights;
    startOfRest.tm_hour = 6;
    startOfRest.tm_min = 0;
    startOfRest.tm_sec = 0;
    convertTimeStructToLocalTime(&startOfRest);
    return startOfRest;
}

bool isNeededLocalNightsInTheRange(int neededLocalNights){
    if(2 <= neededLocalNights && neededLocalNights <= 5){
        return true;
    }
    else{
        return false;
    }
}

void convertTimeStructToLocalTime(TimeStamp * const timeStructToConvert){
    int ret = mktime(timeStructToConvert);
    if( ret == -1 ){
        printf("Error: unable to make time using mktime\n");
    }
}

void wrapGetEndOfRest(TimeStamp startOfRest, int neededLocalNights){
    if(isNeededLocalNightsInTheRange(neededLocalNights)){
        startOfRest = getEndOfRest(startOfRest, neededLocalNights);
        printf("%s is the end\n\n", asctime(&startOfRest));
    }
    else{
        printf("Error: the neededLocalNights is out of range min 2, max 5 got: %d \n\n", neededLocalNights);
    }
}

Why not just use <stdbool.h> instead of defining your own enum for it?  If it has to do with the standard being used, then make sure you're compiling under C99 or GNU99.
It looks like t and t2 are for testing and more could be added at some point.  If so, you could have an array of TimeStamps to help make the code more concise.
isNeededLocalNightsInTheRange() can be simplified as such:

return 2 <= neededLocalNights && neededLocalNights <= 5;

This will automatically return the appropriate condition.
Try to avoid magic numbers by instead making them into constants.  This would apply to the 2, 8, 6, 5, and 0 (the -1 indicates an error and doesn't apply here).  Comments aren't always helpful enough and don't explain each of these anyway.
Error messages should be printed to fprintf() with its first argument as stderr to indicate this.  printf only prints to standard output.


Arrange lines of a text file This program orders the lines of a text file alphabetically and prints them out to a new text file.

Usage:

java Arranger input.txt output.txt

Is this the best and especially the most performant way?

import java.io.File;
import java.io.FileReader;
import java.io.BufferedReader;
import java.io.FileWriter;
import java.io.BufferedWriter;
import java.io.IOException;
import java.util.ArrayList;

/*
 * this program arranges the lines of a text file in it's alphabetical order
 * usage: java Arranger input.txt output.txt
 */

public class Arranger {

    public static void main(String[] args) {

        File inputFile = new File(args[0]);
        File outputFile = new File(args[1]);
        ArrayList<String> words = new ArrayList<>();

        try (BufferedReader reader = new BufferedReader(new FileReader(inputFile))) {
            // read lines of file and order them
            String line = new String();
            while ((line = reader.readLine()) != null) {
                words.add(line);
            }
            words.sort(String::compareToIgnoreCase);
        } catch (IOException e) {
            e.printStackTrace();
        }

        // write ordered list of words to file
        try (BufferedWriter writer = new BufferedWriter(new FileWriter(outputFile))) {
            for (String word : words) {
                writer.write(word + "\n");
            }
        } catch (IOException e) {
            e.printStackTrace();
        }

    }
}

You can probably do something like this as well:

Files.lines(path)

It gets all the lines from the file as a stream, then you can sort the string based on your logic and then collect the same in a list and write to the output file.

This answer gives you a functional programming approach. Most part of the method is self-explanatory.

Files.write(
            Paths.get(outFile),
            Files.lines(Paths.get(path))
                .sorted(String::compareToIgnoreCase)
                .collect(Collectors.toList())
    );


Regex to Match Email address I have the below regex to match email address with atleast one @ and one .

[-0-9a-zA-Z.+_]+@[-0-9a-zA-Z.+_]+\.[a-zA-Z]{2,4}

Am wondering can it be simplified further to match email address with atleast one @ and one .?


A color class and a CRUD class for it Is it better to have only one class color and include the functions from the second class in the 1st one? Or leave them apart?

Why is one better than the other or vice versa? The design pattern is important to me, so why choose one over the other?

Is there a problem if, let's say, we have the function create_color in which we instantiate the class itself like new color()?

class color
{
private $id = NULL;
private $name = '';
private $rgb = NULL;
private $cmy = NULL;
private $wavelength = NULL;
private $frequency = NULL;

public function __construct($name, $rgb, $cmy, $wavelenght, $frequency)
{
setName($name);
setRGB($rgb);
setCMY($cmy);
setWavelength($wavelength);
setFrequency($frequency);
}

public function __destruct()
{
}

public function setName($name)
{
$this->name=$name;
}

public function setRGB($rgb)
{
$this->rgb=$rgb;
}

public function setCMY($cmy)
{
$this->cmy=$cmy;
}

public function setWavelength($wavelength)
{
$this->wavelength=$wavelength;
}

public function setFrequency($frequency)
{
$this->frequency=$frequency;
}

public function getId()
{
return $this->id;
}

public function getName()
{
return $this->name;
}

public function getRGB()
{
return $this->rgb;
}

public function getCMY()
{
return $this->cmy;
}

public function getWavelength()
{
return $this->wavelength;
}

public function getFrequency()
{
return $this->frequency;
}

public function toJSON()
{
return "{'id':'".$this->id."', 'name':'".$this->name."', 'rgb':'".$this->rgb."', 'cmy':'".$this->cmy."', 'wavelength':'".$this->wavelength."', 'frequency':'".$this->frequency."'}";
}

public function toCSV()
{
return $this->id . ", " . $this->name . ", " . $this->rgb . ", " . $this->cmy . ", " . $this->wavelength . ", " . $this->frequency;
}

public function toHTML()
{
return "
ID: " . $this->id . "

Name: " . $this->name . "

RGB: " . $this->rgb . "

CMY: " . $this->cmy . "

Wavelength: " . $this->wavelength . "

Frequency: " . $this->frequency . "
";
}
}

Second class:

class CRUD_color
{
public function create_color($parameters)
{
   $color=new color();
   $color->setName($parameter['name']);
   $color->setRGB($parameter['rgb']);
   $color->setCMY($parameter['cmy']);
   $color->setWavelength($parameter['wavelength']);
   $color->setFrequency($parameter['frequency']);

   $entitymanager->persist($color);
   $entitymanager->flush();
}
public function request_color($parameters)
{
   $color=$entitymanager->find($parameter['id']);
   echo $color->toJSON($parameter['name']);
}
public function update_color($parameters)
{
   $color=$entitymanager->find($parameter['id']);
   $color->setName($parameter['name']);
   $color->setRGB($parameter['rgb']);
   $color->setCMY($parameter['cmy']);
   $color->setWavelength($parameter['wavelength']);
   $color->setFrequency($parameter['frequency']);

   $entitymanager->persist($color);
   $entitymanager->flush();
}
public function delete_color($parameters)
{
   $color=$entitymanager->delete($parameter['id']);
}
}

Just from looking at both classes, CRUD_color is essentially putting methods on top of Color but those methods seem like something you'd want in Color. I would ask yourself what the purpose CRUD_color serves that Color should not. If there is no reason for the 2nd class, then get rid of it.

I would recommend overloading the __get() operator and get rid of the get methods, it'll clean things up a little bit in your first class.

Also I wouldn't have create_color accept an array of parameters, instead be explicit with what the values need. If you don't want to take this advice, then at least do some error checking for when those parameters don't exist in the array.

...Also I think CRUD_color should probably be static, or at least right now it seems like you are treating it as a static class. If you want it to act like a class which is inheriting from Color I would instead have a constructor and initialize a Color object which is a data-member of CRUD_color.


Project Euler #8 in C++: greatest product of k consecutive digits This a problem from Hackerrank.com and Project Euler #8. The problem statement is as below. Is there a specific algorithm to solve such problems? Can my code be made more efficient?

  Find the greatest product of K consecutive digits in the N digit
  number.

  Input Format: First line contains T that denotes the number of test cases.  First line of each test case will contain two integers
  N & K. Second line of each test case will contain an N digit
  integer.

  Output Format

  Print the required answer for each test case.

  Constraints

  \$1 \le T \le 1000\$

  \$1 \le K \le 7\$

  \$K \le N \le 1000\$

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>
using namespace std;
#define ctoi(c)(int)((c)-'0')

inline unsigned long long prodK(string number, int start, int end){
    unsigned long long product=1;
    for (int j=start;j<end;j++){
        product*=ctoi(number[j]);
    }
    return product;
}

int main() {
    int T,N,K;
    string number;
    unsigned long long product,max;
    cin>>T;
    for (int i=0;i<T;i++){
        cin>>N>>K;
        cin>>number;
        product=1;
        product=prodK(number,0,K);
        max=product;
        for (int l=K;l<N;l++){
            if (ctoi(number[l-K])!=0){
                product=(product/ctoi(number[l-K]))*ctoi(number[l]);
            }else{
                product=prodK(number,l-K+1,l+1);
            }
            max=(product>max)?product:max;
        }
        cout<<max<<"\n";
    }
    return 0;
}

Extra multiplication

    unsigned long long product=1;
    for (int j=start;j<end;j++){
        product*=ctoi(number[j]);
    }

You can do one less multiplication.

    unsigned long long product = ctoi(number[j]);
    for (int j = start + 1; j < end; j++) {
        product *= ctoi(number[j]);
    }

Obviously this would fail if K were less than 1, but that's outside the problem scope.

ctoi is not a free operation

You also perform ctoi at least K times for each digit.  You could do this just once with something like

    std::vector<int> digits;
    for (char c : number) {
        digits.push_back((int)(c - '0');
    }

Then operate on digits rather than number.

Benchmark optimizations

            if (ctoi(number[l-K])!=0){
                product=(product/ctoi(number[l-K]))*ctoi(number[l]);
            }else{
                product=prodK(number,l-K+1,l+1);
            }

In theory, this saves K-1 multiplications at the cost of doing a multiplication, a division, and a comparison/branch.  But don't forget that it adds a comparison/branch to every iteration.  You might find that is cheaper to always do the K-1 multiplications.

Benchmark it both ways with a variety of inputs to see.

Don't waste math

You have

    for (int j=start;j<end;j++){

and

                    product=prodK(number,l-K+1,l+1);

But you could just have

    for (int j = start; j <= end; j++){

and

                    product = prodK(number, l-K+1, l);

That saves you an addition.  Might not be worth much, but it also might be better than nothing.

Assignments can be expensive

            max=(product>max)?product:max;

This will produce the correct answer, but it does an assignment every time.  You don't need to do the no-op assignment.  Benchmark against

            if (product > max) {
                max = product;
            }

That's more code, but it's faster unless the compiler already optimizes out the no-op assignment.  Probably not a lot faster, as this is a comparatively simple assignment.

Simplify compare

    for (int i=0;i<T;i++){

i<T is often implemented as i - T < 0.  But do you need to do the i - T?

Since you never use T for anything other than this loop, you could say

    for (; T > 0; T--) {

That saves having an i variable at all, and now you compare T to 0 directly.

Benchmark

These are suggestions that assume that the compiler is operating in a certain way.  If I'm wrong about that, then some of these suggestions may be counterproductive.  Note that the compiler may have similar or better optimizations that it is already doing.  You benchmark to confirm (or refute) such assumptions.


Class for returning a different value every so often I have a class that I'm trying to determine a good name for. I don't know if this concept comes up a lot in programming or not, but I'm wondering if I can get some suggestions for a good name for the class.
Basically, this is how the class functions:

The first time its Get() method is called, it returns value A.
Every subsequent time, it returns value B.
If enough time has passed (provided), return value A one time, then revert back to sending 'B'. Repeat ad finitum.

Is there any name for something that functions like this? It's somewhat like memoization, and kind of like caching, but not quite.
The application of this construct is so described: Every so often I want a method to load complex data from Active Directory to load into our database before returning the results from the table. This ensures that we have recent data in our database. However, the time to execute this is high, and 99% of the time nothing has changed. So I want to limit the checking for changes to long timespans in my web app. So, using this construct I can say "only do the long version every X hours, and once it's been done, don't do it again for X more hours."
Implementation
The ICurrentDateTimeProvider simply has a Now property so it can be faked for unit tests.
public interface ICurrentDateTimeProvider
{
    DateTime Now { get; }
}

public class CurrentDateTimeProvider : ICurrentDateTimeProvider
{
    public DateTime Now { get { return DateTime.Now; } }
}

public class ReturnDifferentValueEverySoOften<T>
{
    public ReturnDifferentValueEverySoOften(T firstAndSometimes, T most, TimeSpan flipTime = default(TimeSpan), ICurrentDateTimeProvider provider = null)
    {
        FirstAndSometimes = firstAndSometimes;
        MostOfTheTime = most;
        if (flipTime == default(TimeSpan))
            flipTime = TimeSpan.FromHours(6);
        FlipTime = flipTime;
        if (provider == null)
            provider = new CurrentDateTimeProvider();
        DateTimeProvider = provider;
    }

    public void SetTimeProvider(ICurrentDateTimeProvider provider)
    {
        DateTimeProvider = provider;
    }

    public T FirstAndSometimes;
    public T MostOfTheTime;

    ICurrentDateTimeProvider DateTimeProvider { get; set; }
    TimeSpan FlipTime;
    DateTime LastFlip { get; set; }
    DateTime NextFlip { get { return LastFlip.Add(FlipTime); } }
    bool ever = false;

    public T Get()
    {
        var now = DateTimeProvider.Now;
        if (!ever)
        {
            ever = true;
            LastFlip = now;
            return FirstAndSometimes;
        }
        if (NextFlip < now)
        {
            LastFlip = now.Add(FlipTime);
            return FirstAndSometimes;
        }
        return MostOfTheTime;
    }
}

Unit Test
Below is a unit test I wrote to ensure that it functions properly:
[Fact]
public void CanFlipBetweenValues()
{
    var timeToWait = TimeSpan.FromMilliseconds(10);

    var currentTime = DateTime.Now;
    var timeProvider = new Mock<ICurrentDateTimeProvider>();
    timeProvider.SetupGet(x => x.Now).Returns(currentTime);
    var cacher = new ReturnDifferentValueEverySoOften<bool>(false, true, timeToWait);
    cacher.SetTimeProvider(timeProvider.Object);

    Assert.False(cacher.Get());
    Assert.True(cacher.Get());
    Assert.True(cacher.Get());
    Assert.True(cacher.Get());
    Assert.True(cacher.Get());

    //add some arbitrary time > timeToWait
    var newTime = currentTime.Add(timeToWait).Add(new TimeSpan(0, 0, 1));
    var newTimeProvider = new Mock<ICurrentDateTimeProvider>();
    newTimeProvider.SetupGet(x => x.Now).Returns(newTime);
    cacher.SetTimeProvider(newTimeProvider.Object);

    Assert.False(cacher.Get());
    Assert.True(cacher.Get());
    Assert.True(cacher.Get());
    Assert.True(cacher.Get());
    Assert.True(cacher.Get());
}

Again, I'm mostly looking to see if there exists a better name for something like this, but I'm open to other code review suggestions as well.

I might be completely off with this, but I'll try to propose an alternative design for your problem.

It seems to me that you have two independent tasks in your application:

Fetching the data from the DB (user triggered?, frequent)
Updating the DB from the AD periodically (time consuming)

Define a flag in your application, say, UpdateInProgress. Update task would set it to true before running and reset it after it's done. Every Fetch request that comes while an update is in progress would be queued and executed after the update has completed.


Mapping Many Parameters Programmatically I have a function for merging data from an object (a PrintTestDataItem) into a template file for a printed label (a LabelFormatDocument called label in this snippet). The LabelFormatDocument is a third-party class that I use to set various fields on my label and I do this by setting the numerous substrings to the values that I want from my PrintTestDataItem object.

There are a lot of these substrings and my current function just sets each substring sequentially, which is obviously not an ideal way to do this sort of operation. What would be a better way to set these substrings programmatically and any other suggestions for increasing the maintainability of this function?

My first idea is to use a dictionary to store the substring names and map them to the appropriate parameter of the PrintTestDataItem, is this the right path to go down?

public void MergePrintData(PrintTestDataItem printTestData)
{
    label.SubStrings["Barcode"].Value = printTestData.IDNumber.ToString();
    label.SubStrings["Date"].Value = printTestData.TestDate.ToString();
    label.SubStrings["WRN"].Value = printTestData.WRNNumber;
    label.SubStrings["Operator"].Value = printTestData.OperatorName;
    label.SubStrings["PrintheadType"].Value = printTestData.HeadType;
    label.SubStrings["PrintheadSerial"].Value = printTestData.HeadSerial;
    label.SubStrings["Batch"].Value = printTestData.BatchNumber;
    label.SubStrings["PrintFrequency"].Value = (printTestData.PrintFrequency + "kHz");
    label.SubStrings["PrintDistance"].Value = (printTestData.PrintDistance + "mm");
    label.SubStrings["InkType"].Value = printTestData.InkType.ToString();
    label.SubStrings["InkColour"].Value = printTestData.InkColour;
    label.SubStrings["InkTemperature"].Value = (printTestData.InkTemperature + "°C");
    label.SubStrings["DifferentialPressure"].Value = (printTestData.DifferentialPressure + "mBar");
    label.SubStrings["MeniscusPressure"].Value = (printTestData.MeniscusPressure + "mBar");
    label.SubStrings["Comments"].Value = printTestData.Comments;
    label.SubStrings["EncoderDivide"].Value = printTestData.EncoderDivide;
    label.SubStrings["EncoderMultiply"].Value = printTestData.EncoderMultiply;
    label.SubStrings["Offset"].Value = printTestData.Offset;
    label.SubStrings["NumberOfGreyLevels"].Value = printTestData.NumberOfGreyLevels;
    label.SubStrings["Mirror"].Value = BooleanRemapping(printTestData.Mirror);
    label.SubStrings["DIR"].Value = BooleanRemapping(printTestData.DIR);
    label.SubStrings["ReversePrint"].Value = BooleanRemapping(printTestData.ReversePrint);
    label.SubStrings["PrintDirection"].Value = printTestData.PrintDirection.ToString();
    label.SubStrings["PrintMode"].Value = printTestData.PrintMode.ToString();
    label.SubStrings["PrintTransportMode"].Value = printTestData.PrintTransportMode.ToString();
    label.SubStrings["AmbientTemperature"].Value = (printTestData.AmbientTemperature + "°C");
    label.SubStrings["HoursRun"].Value = printTestData.HoursRun;
    label.SubStrings["LinesDown"].Value = printTestData.LinesDown;
}

I think the solution for this case maybe is more painful than a long code of config, and this code looks like test data.

A possible workaround could be match the label string with the same property of printTestData. and then call it by reflection. But i prefer have the long config. instead of a hackish-call-property.


Refactor Helper methods I have two helper methods that return li for view with the li for current view having id as selected.

def get_li(a)
  li = "<li"
  li += ' id=selected' if (a[:controller] == params[:controller] && a[:action] == params[:action])
  li += ">" + link_to(a[:text], { :controller => a[:controller], :action => a[:action]}) + "</li>"
end

def get_tab_options
  if (params[:controller] == 'sessions' || params[:controller] == 'users')
    [{:text => "Login", :controller => 'sessions', :action => 'new'},
     {:text => "Sign Up", :controller => 'users', :action => 'new'}]
  elsif (params[:controller] == 'meetings')
    [{:text => "Meetings List", :controller => 'meetings', :action => 'index'},
     {:text => "Create Meeting", :controller => 'meetings', :action => 'new'},
     {:text => "Running Meetings", :controller => 'meetings', :action => 'monitor'}]
  end
end

And the view file has

 %ul
   -get_tab_options.each do |a|
     = get_li(a)

How can I write this better?

First of all let's talk about method names:

The get_ prefix is usually not used in ruby - you'd just leave it off.

Further the name get_li (or just li) is not very descriptive of what the method does. It tells me that it will produce an
, sure, but it doesn't tell me that the
 will contain a link. From the method name I would just assume that this is a general helper method I can use whenever I want a list item. It should probably be called something like list_item_with_link or possibly navigation_entry, assuming that the
s produced by this method are meant as entries in a navigation bar/menu.

The name of the argument is also not very descriptive. I assume you called it a because of the  tag, but a does not really contain an a tag, just the information you use to build one. I'd call it something like link instead.

Your methods should also be commented. Sure in the case of short methods like this, it's easy enough to tell what it does from looking at the code, but there should still be comments explaining what the method does, when and how it's intended to be used and what the arguments should look like.

Now to the actual code: Instead of building the li tag using string concatenation, you should use the content_tag helper instead, which builds HTML-tags containing content, like
.

I also feel that the code would become nicer if the the logic to determine whether a link points to the current page was factored into its own method. I'm sure it could be useful in other places as well.

On a general design note, it seems wrong that you're restricting the links to only contain :controller and :action. Maybe you would later like to add a link to an action with a specific parameter. Your current design would not allow that.

For this you could just use your current system and just remove the :text key out of the hash after retrieving it and then passing the rest to link_to. However this would break if you ever have an action to which you want to supply a parameter named text.

So what I would do is the separate the link text from the link location. For example an array where the first element is the link text and the second is the location might work. This way ["Meetings List", {:controller => 'meetings', :action => 'index'}], ["Meetings List", {:controller => 'meetings'}] and ["Meetings List", {:controller => 'meetings', :action => 'index', :show_only_active => true}] would all be valid links.

Instead of an array, it could be even nicer to use a struct/class. This way you could also put the logic of determining whether the link points to the current page and how to turn the link to HTML into that class.

With all those points, my code would probably look like this:

# Class to represent links.
# First argument to new will be the text to be displayed for the link and the second
# argument will be the location that the link points to as a hash.
Link = Struct.new(:text, :location) do
  include ActionView::Helpers

  # Turns this link into an HTML link
  def to_html
    link_to(text, location)
  end

  # Returns true if this link points to the same controller and action as the given
  # location hash.
  def points_to?(other_location)
    # If no action is specified, the :index action is used
    # This way the locations {:controller => :foo} and {:controller => :foo, :action => :index}
    # will be treated as equal
    action = location[:action] || :index
    other_action = other_location[:action] || :index
    location[:controller] == other_location[:controller] && action == other_action
  end
end

# Returns an li tag containing a link to the location represented by the given Link object.
# The tag's id will be "selected" if the link points to the current action and controller.
def navigation_item(link)
  selected = link.points_to?(params) ? "selected" : nil
  content_tag(:li, link.to_html, :id => selected)
end

You'd then use Link.new("title", :controller => "foo", :action => bar) inside get_tab_options to create a link.

As a last note, it also seems a bit icky to me to hardcode which links will be available in which controller into a helper method. It seems that this should rather be handled in the controllers.


Simple structure that would contain validation errors messages Before I go too far, does this look right?

This is mostly to have a "central" place that hold a "hierarchical/structured" errors message.

public static class ValidationError
{
    static ValidationError()
    {
        Prefix = "VE";
        Notice = new _Notice();
        Exporter = new _Exporter();
        Importer = new _Importer();
        Shipping = new _Shipping();
        //few others one
    }

    public static string Prefix { get; private set; }

    public static _Notice Notice { get; private set; }
    public class _Notice
    {
        public _Notice()
        {
            Prefix = ValidationError.Prefix + "NO";
            MustHaveOneTreatmentOption = Prefix + "001";
        }

        public string Prefix { get; private set; }
        public string MustHaveOneTreatmentOption { get; private set; }
    }

    public static _Exporter Exporter { get; private set; }
    public class _Exporter
    {
        public _Exporter()
        {
            Prefix = ValidationError.Prefix + "EX";
            Mailing = new Address(Prefix, "MA");
            Physical = new Address(Prefix, "PH");
        }

        public string Prefix { get; private set; }

        public Address Mailing { get; private set; }
        public Address Physical { get; private set; }
    }

    public static _Importer Importer { get; private set; }
    public class _Importer
    {
        public _Importer()
        {
            Prefix = ValidationError.Prefix + "IM";
        }

        public string Prefix { get; private set; }
    }

    public static _Shipping Shipping { get; private set; }
    public class _Shipping
    {
        public _Shipping()
        {
            Prefix = ValidationError.Prefix + "SH";
        }

        public string Prefix { get; private set; }
    }

    public class Address
    {
        public Address(string _Prefix, string _Suffix)
        {
            Prefix = _Prefix + _Suffix + "AD";
        }

        public string Prefix { get; private set; }
    }
}

Naming
Class names in C#, whether they're for a static, non-static, internal, public, private, abstract, nested or on-its-own class, should be PascalCase.
Find the intruder:
public class _Notice
public class _Exporter
public class _Importer
public class _Shipping
public class Address

 Only Address is correct!

Keep the underscore prefix for private fields, it seems you're using prefixes only to avoid name clashes - this smells, because in naming what's important is consistency - using arbitrary prefixes is bad practice.
Also, it's possible the nested types are not warranted, but it's very hard to tell without seeing how you're using this ValidationError static class.


Validating a model and focus to the element if validation fails I am using Backbone Marionette - I would like to set the user name and password if that passes the model validation method.

I am highly confused with that.  Please show me a handy approach or correct me if I'm doing this wrong.

define(["backbone"], function(Backbone){
    var LoginModel = Backbone.Model.extend({
        defaults:{
                userName:'',
                password:''
        },
        validate:function(attr, options){
            if (!attr.userName) {
                return 'I need your userName';
              }
              if (!attr.password) {
                return 'I need your password';
              }
        },
        initialize:function(){
            this.on("invalid", function(model, error){
                console.log(model, error); // error consoles 2 times (4 times for both)
            });
        }
    });

    return LoginModel;
});

View.js:

define([
    'jQuery','underscore',
    'backbone','marionette',
    'text!./templates/loginView.html'],
    function($,_,Backbone,Marionette,template){
        "use strict";

        var LoginView = Backbone.Marionette.ItemView.extend({

            className:'col-xs-12 col-md-4 col-md-offset-4',

            template:_.template(template),

            events:{
                "submit form" : "loginSubmit"
            },

            loginSubmit:function(e){
                e.preventDefault();
                var data = Backbone.Syphon.serialize(e.target),

                validUser = this.model.set('userName',data.userName, {validate: true}),
                validPass = this.model.set('password', data.password,{validate: true});

                if(!validUser){
                    $(e.target).find('#userName').select();
                } else{
                    $(e.target).find('#userName').next(':input').select();
                }

                if(!validPass){
                    $(e.target).find('#userName').select();
                }

            }

        });

        return LoginView;
    }
);

This works, but I am looking for the best approach.

I'm not very familiar with Backbone, however I do have a couple of comments:

To simplify your life, check out Marionette Forms. It can handle quick validation of forms. This is an alternative to Backbone Forms, which is generally more demanding in terms of code needed.
The following snippet:

if(!validUser){
    $(e.target).find('#userName').select();
} else{
    $(e.target).find('#userName').next(':input').select();
}

if(!validPass){
    $(e.target).find('#userName').select();
}

could simply be:

if (validUser && validPass) {
    $(e.target).find('#userName').next(':input').select();
} else {
    $(e.target).find('#userName').select();
}


TicTacToe in Javascript I made this two player TicTacToe game as a challenge i was given in class. The game starts where the first player chooses where their X will be placed. Each area in the array has been given a number starting from 1 and ending at 9. After X chooses their area, the game will let the user know who's turn it is, and they will keep playing until one of them wins.

As of right now, the game doesn't let you know that nobody won, but that's another update i have to make.

Please do review the code and let me know how i can make it more efficient and i am also open for suggestions, so please be honest as much as possible as this is the first time i write in Javascript using Nodejs, and i would love to get better.

 //user input and format
let input = process.stdin;
input.setEncoding("utf-8");

console.log("You will start with X");
console.log("Please choose a box number from 1 to 9 to place your X");

//initializing an array of 9 fields where each one contains * as a start
let gameboard = []; // = ["*", "*", "*", "*", "*", "*", "*", "*", "*"];
let symbol = "*";

for (let i = 0; i < 9; i++) {
  gameboard[i] = symbol;
}

//determining who's turn it is and the players' symbols
let player1isplaying = true;
let player1 = "X";
let player2 = "O";

//Game loop
input.on("data", function(data) {
  console.clear();

  //bad input
  if (parseInt(data) > 9 || parseInt(data) < 1 || isNaN(data)) {
    console.log("Please enter a number between 1 and 9");
  } else if (gameboard[parseInt(data) - 1] != symbol) {
    console.log("The spot you're trying to fill is already taken.");
  }
  //which player's turn
  else if (player1isplaying) {
    gameboard[parseInt(data) - 1] = player1;
    player1isplaying = false;
  } else if (!player1isplaying) {
    gameboard[parseInt(data) - 1] = player2;
    player1isplaying = true;
  }

  showboard(gameboard);
  checkforXwin(gameboard);
  checkforOwin(gameboard);

  if (player1isplaying) {
    console.log(player1 + "'s turn");
  } else {
    console.log(player2 + "'s turn");
  }
});

//winner validation
function checkforXwin(array) {
  if (
    (array[0] == player1 && array[1] == player1 && array[2] == player1) ||
    (array[3] == player1 && array[4] == player1 && array[5] == player1) ||
    (array[6] == player1 && array[7] == player1 && array[8] == player1) ||
    (array[0] == player1 && array[4] == player1 && array[8] == player1) ||
    (array[2] == player1 && array[4] == player1 && array[6] == player1) ||
    (array[0] == player1 && array[3] == player1 && array[6] == player1) ||
    (array[1] == player1 && array[4] == player1 && array[7] == player1) ||
    (array[2] == player1 && array[5] == player1 && array[8] == player1)
  ) {
    console.log(player1 + " wins");
    process.exit();
  }
}

//winner validation
function checkforOwin(array) {
  if (
    (array[0] == player2 && array[1] == player2 && array[2] == player2) ||
    (array[3] == player2 && array[4] == player2 && array[5] == player2) ||
    (array[6] == player2 && array[7] == player2 && array[8] == player2) ||
    (array[0] == player2 && array[4] == player2 && array[8] == player2) ||
    (array[2] == player2 && array[4] == player2 && array[6] == player2) ||
    (array[0] == player2 && array[3] == player2 && array[6] == player2) ||
    (array[1] == player2 && array[4] == player2 && array[7] == player2) ||
    (array[2] == player2 && array[5] == player2 && array[8] == player2)
  ) {
    console.log(player2 + " wins");
    process.exit();
  }
}

//showing the board in 3 rows and 3 columns
function showboard(array) {
  console.log("---------");
  console.log(array.slice(0, 3).join("   "));
  console.log("");
  console.log(array.slice(3, 6).join("   "));
  console.log("");
  console.log(array.slice(6, 9).join("   "));
  console.log("---------");
}

First the really easy advice:

use strict mode https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode
use const instead of let for stuff you know you're not supposed to change (symbol, player1, player2); declaring it const helps you keep some assumptions later
use === instead of == generally; I think the only legitimate use for == is for using it with null when you allow undefined or results of typeof if you feel like you're a JS human minifier :)
prefer camel case since JS already does it and it would look weird to have two casing strategies in the same codebase
use better naming in general: I'm talking about array in function checkforOwin(array) - array is extremely generic (what array?!?) I would call it board so I know what I'm dealing with; the fact that it's implemented as an array is a detail
I'd also like a function named checkforOwin to just check and return true/false than exit the process xD
who's O in this same function name? I thought we're comparing with the variable player1; stick to one or the other, not both; you'd have to update quite a bit of code if you change the initial statement of let player1 = "X"

...and the more involved:

I would use nested arrays for the board because I can't easily picture what board[7] is; but I can picture what board[2][1] is
those really long if statements look very repetitive; they look a lot like patterns on a board so instead let's turn that code into data so we can manage it easier; we can rewrite the patterns like this:

const isOWinner = [
  [0, 1, 2],
  [3, 4, 5],
  [6, 7, 8],
  [0, 4, 8],
// ...
].some((indices) => indices.every((index) => board[index] === player1))

That big condition blob in your if statements can be rewritten like this. Because it's an array you can store it somewhere; load it; merge with other patterns or extend or whatever you can do with arrays. Code that looks like data should be represented as data.
In fact let's use the same data in both functions and merge them into one:

function checkForWin (board, player) {
  const isWinner = [
    [0, 1, 2],
    [3, 4, 5],
    [6, 7, 8],
    [0, 4, 8],
    // ...
  ].some((indices) => indices.every((index) => board[index] === player))

  if (isWinner) {
    console.log(player + 'wins')
    process.exit()
  }
}

And now you just call checkForWin(board, player1) or checkForWin(board, player2)


jQuery dropdown plugin to prevent scanning the whole DOM on every click Here is a simple plugin I have written for a jQuery dropdown effect. Simple show / hide (although open classes may be better).

When you click the HTML the event searches the whole DOM for a specific class and then hides it (or removes the open class), even if the dropdown is already close.

Is there a way to only apply the events to open dropdowns somehow?

$.fn.dropdown = function () {

        return this.each(function () {

            // Initial varaibles
            var $this = $(this),
                $toggle = $this.find('.dropdown-toggle'),
                $dropdown = $this.find('.dropdown-menu');

            // When clicking a dropdown toggle
            $toggle.click( function(e){

                // Stop the default behaviour
                e.preventDefault();

                // Clear any open dropdowns
                clearDrops();

                // Stop the event from bubbling up the DOM tree
                e.stopPropagation();

                // Toggle the dropdown
                $dropdown.toggle();

            });

            // Clear the dropdowns
            var clearDrops = function () {
                // Search the entire DOM for dropdowns & hide them
                // THERE MUST BE A BETTER WAY
                $('.dropdown-menu').hide();
            };

            // Trigger the closing of dropdowns when clicking on the HTML
            $('html').bind('click', clearDrops);

        });

    };

$('.dropdown').dropdown();

Comments:

Don't use the name 'dropdown' that has a very high probability for naming collisions with other libraries.
I get that you might want to do stuff as a learning experience but you are recreating something that already exists (as a matter of fact a lot of versions of this widget exist). You might want to check out how something like chosen works.
In a similar vein, this ain't gonna work with keyboard input. Maybe you don't need it, but people would expect a dropdown widget to work with keyboard input
This seems to require that a certain html structure exists. Someone using this without that knowledge will get confusing and silent failures. The way that jquery ui widgets usually handle this is to hide the initial element and then use jquery.after() to create the expected elements right after it
Actually you really should use the jquery ui widget factory that is a better fit for this then just a bare jquery plugin. (Jquery plugins generally are better for adding behaviors, JqueryUi Widget Factory is the preferred way to create enhanced controls)
Are you sure you want to only close dropdowns on a click event? That seems far too simplistic. What if someone opens your dropdown and uses the tab key to tab to other fields? Usually something like this will close when focus is lost (I believe that is the blur event)
I'm not sure if everything is a child of the html element - detached pop-up dialogs made via jquery ui for example. (not sure about this one)
You do not need to stop propogation nor clearMenus when the toggle is clicked. The click will propagate up to the html element and close it then.
You will bind the clearDrops function to the html element's click event as many times as there are controls on the page - you only need it once. Do a check or unbind.
Is scanning the whole DOM really a problem? The scan only happens when the user interacts with the page, she will have to click on things very quickly indeed for this to be a real preformance problem.

A direct answer to your question: Just remember what dropdown controls have been initialized and hide those.

( function($) {
    var initializedDropdowns = $();

    $.fn.dropdown = function () {

            return this.each(function () {

                var $this = $(this),

                ...
                initializedDropdowns.add($this)
                ...
                var clearDrops = function () {
                    initializedDropdowns.hide();
                };
                ...
            });

        };
} )(jQuery)


Optimised solution for implementing a queue with two stacks I need help to optimise the following code in C++, which implements a queue with two stacks. Unfortunately, it's not passing some of the test cases @ the following link: https://www.hackerrank.com/challenges/ctci-queue-using-two-stacks The (hidden) test cases yield "Terminated due to timeout 2s". How can I make my implementation faster?

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>
#include <stack>
#include <queue>
using namespace std;

class MyQueue {
    public:
        stack<int> stack_newest_on_top, stack_oldest_on_top;
        void push(int x) {

            /* Check if the stack_oldest_on_top is empty..
                if it is not empty, push all the elements onto
                stack_newest_on_top */
            while(true != stack_oldest_on_top.empty()) {
                stack_newest_on_top.push(stack_oldest_on_top.top());
                stack_oldest_on_top.pop();
            }
            /* When you want to push a value on to the que,
                it should be stack_newest_on_top*/
            stack_newest_on_top.push(x);
        }

        void pop() {
            /* Check if the stack_newest_on_top is empty.. */
            while(true != stack_newest_on_top.empty()) {
                /* Access the top of the newest stack */
                stack_oldest_on_top.push(stack_newest_on_top.top());
                /* After acessing the top, just pop it off */
                stack_newest_on_top.pop();
            }
            if (true != stack_oldest_on_top.empty()) {
                /* Pop the elem at the top of oldest stack. */
                stack_oldest_on_top.pop();
            }
        }

        int front() {
            int rt_value = 0;
            while(true != stack_newest_on_top.empty()) {
                stack_oldest_on_top.push(stack_newest_on_top.top());
                /* After acessing the top, just pop it off */
                stack_newest_on_top.pop();
            }
            if (true != stack_oldest_on_top.empty()) {
                /* Get the top element of the stack */
                rt_value = stack_oldest_on_top.top();
            }
            return rt_value;
        }
};

int main() {
    MyQueue q1;
    int q, type, x;
    cin >> q;

    for(int i = 0; i < q; i++) {
        cin >> type;
        if(type == 1) {
            cin >> x;
            q1.push(x);
        }
        else if(type == 2) {
            q1.pop();
        }
        else cout << q1.front() << endl;
    }
    /* Enter your code here. Read input from STDIN. Print output to STDOUT */
    return 0;
}

Lets assume the following test case: First you push a million elements into the queue. Than you perform alternating a pop and a push. The pop will move all of the million elements of the first stack to the second one and the push will move all of them back to the first time. Every alternating push and pop call has a time complexity of O(size of queue). This is not very efficient and you get time problems really soon. And that is why you get the "Terminated due to timeout 2s" error on Hackerrank.

So copying the elements like crazy is definitely not the way to go. How can you improve it?

Here's a hint: The main error that you do is, that you assume that one of the stacks should be empty at all time. This doesn't have to be the case. Only copy if you really have to.

If you are still stuck, check out the solution:

  stack_newest_on_top keeps the newest elements on top.
So if you push one element into the queue, just push it onto this stack. Don't worry about the about stack_oldest_on_top.
void push(int x) {
   stack_newest_on_top.push(x);
}
 If you pop/front an element, do the same thing. Just use the top element from stack_oldest_on_top. Only if stack_oldest_on_top is empty. You need to move all the elements from stack_newest_on_top to stack_oldest_on_top.
void pop() {
    if (stack_oldest_on_top.empty()) {
        while (!stack_newest_on_top.empty()) {
            stack_oldest_on_top.push(stack_newest_on_top.top());
            stack_newest_on_top.pop();
        }
    }
    stack_oldest_on_top.pop();
}
 Notice, in the worst case a pop has still O(size of queue) time complexity. But it is called very rarely. In fact every element moves from the first stack only once to the second stack. This gives you an amortised time complexity of O(1).


Decoding binary string message I am trying to learn some Java on my own, and I am tackling some "programming challenge" problems to practice the little I have learnt so far.

Could anybody constructively critique this beginner's effort (source on Bitbucket)?

public class BinaryCode {
  public String[] decode(String message) {
    int messageLength = message.length();
    return new String[] { decodeMessage(message, messageLength, 0),
        decodeMessage(message, messageLength, 1) };
  }
  private String decodeMessage(String encodedMessage, int messageLength,
      int assumedFirstDigit) {
    StringBuilder decodedMessage = new StringBuilder(messageLength);
    int nextDecodedDigit = assumedFirstDigit;
    for (int i = 0; i < messageLength; i++) {
      if (nextDecodedDigit != 0 && nextDecodedDigit != 1) {
        return "NONE";
      }
      decodedMessage.append(nextDecodedDigit);
      int encodedDigit = encodedMessage.charAt(i) - '0';
      int decodedDigit = decodedMessage.charAt(i) - '0';
      nextDecodedDigit = encodedDigit - decodedDigit;
      if (i > 0) {
        nextDecodedDigit -= decodedMessage.charAt(i - 1) - '0';
      }
    }
    int checkValue = decodedMessage.charAt(messageLength - 1) - '0';
    if (messageLength > 1) {
      checkValue += decodedMessage.charAt(messageLength - 2) - '0';
    }
    if (checkValue == encodedMessage.charAt(messageLength - 1) - '0') {
      return decodedMessage.toString();
    }
    return "NONE";
  }
}

These critiques are mostly style and syntax related, and do not address your algorithm (you could probably find the simplest possible algorithm with Google anyway).

make decode and decodeMessage static
explicitly hide the constructor for BinaryCode, e.g.

// hide constructor
private BinaryCode() {}

remove unneseccary - 0 from encodedDigit and decodedDigit declarations
let decodeMessage calculate messageLength (fewer parameters are nearly always better)
avoid using magic numbers and string literals (-1, -2, "NONE", etc...)
create method lastChar and call it instead of messageLength - 1 e.g. int checkValue = lastChar(decodedMessage) - '0';

private static char lastChar(CharSequence sequence) {
    int lastIndex = sequence.length() - 1;
    return sequence.charAt(lastIndex);
}

replace decodedMessage.charAt(messageLength - 2) and decodedMessage.charAt(i - 1) with previouslyDecodedChar e.g.

char previouslyDecodedChar = '\0';
for (int i = 0; i < messageLength; i++) {
    // ...
    if (i > 0) {
        nextDecodedDigit -= previouslyDecodedChar - '0';
    }
    previouslyDecodedChar = decodedMessage.charAt(i);
}
int checkValue = lastChar(decodedMessage) - '0';
if (messageLength > 1) {
    checkValue += previouslyDecodedChar - '0';
}


Optimizing Efficiency in a Stack I'm solving a problem on HackerRank where I'm required to implement a simple stack.

It's passing all the tests except for the last 4 where it fails due to surpassing the time constraint of 10s.  These 4 failing tests are running 200,000 operations on the stack.
How can I optimize my code below:
# operations of the form ['push -36', 'pop', 'push 16', 'pop', 'inc 1 -17', ...]

from collections import deque

def superStack(operations):

    def push(v):
        S.append(int(v))

    def pop():
        return S.pop()

    def inc(i,v):
        i, v = int(i), int(v)
        for pos in range(i):
            S[pos] += v

    S = deque()
    funcs = locals()

    for operation in operations:
        op, *args = operation.split(' ')
        funcs[op](*args)

        print(S[-1] if S else "EMPTY")

A few notes:

Some constraints:

I've chosen deque over list, to avoid the "contiguous memory block" problem that lists  may encounter.

Since pop and append are both O(1), the heaviest operation is the inc (especially so if i is large). So I keep all items as integers to avoid conversion from string to int multiple times in the inc loop

Just be lazy. Don't do what you're told. They'll never know.
Instead of truly increasing the bottom i values right away, just make a note at the i-th value from the bottom to increase it by v when it gets popped. And when that pop happens, move the note to the value below, so it gets increased by v as well (when it gets popped). And so on, so that this will add v to all bottom i values (when they get popped).
To support multiple inc operations: If a value already has a note to add w to it (and all values below it), change that to w + v.
For easier coding, just store all values along with such a note, but with initial adding value 0.
Now all three operations are O(1).
Oh wait. Not quite O(1), since you're using a deque so you can't access an arbitrary index in O(1). That's actually a problem in your solution as well. Your increasing the bottom i values isn't O(i) but only O(i2). If you switch to a list, yours would be O(i) and mine would be O(1). This loses the O(1) of appending/popping to/from a deque, but the list still does them in amortized O(1). So then all three operations are amortized O(1). Alternatively you could keep your deque and store the notes in a separate dict, but then it's O(1) just as much as dict access is O(1) (i.e., not truly, but practically), and I suspect it would be a bit slower than the list version.


Dependency Injection with classes which are created often I have a question about dependency injection and best practices when using simple classes which are meant to be created often, which have dependencies on external services. Here's a (vastly) simplified version of what I'm working on:

app.js

import {Factory} from './factory.js';
import {Convert} from './convert.js';

let convert = new Convert();
let factory = new Factory({ convert });

// assume this is being imported and executed somewhere
export function run() {
    let avengers = [
          factory.person({ first: 'Iron', last: 'Man' })
        , factory.person({ first: 'Captain', last: 'America' })
        , factory.person({ first: 'Scarlet', last: 'Witch' })
        , factory.person({ first: 'Black', last: 'Panther' })
    ];
    console.log(avengers);
}

convert.js

export class Convert {
    toString(val) {
        if (!val)
            return '';
        return val.toString();
    }
}

person.js

export class Person {
    constructor({
        convert,
        first,
        last
    }) {
        this.convert = convert;
        this.first   = this.convert.toString(first);
        this.last    = this.convert.toString(last);
    }
}

factory.js

import {Person} from './person.js';

export class Factory {

    constructor({ convert }) {
        this.convert = convert;
    }

    person(obj) {
        obj.convert = this.convert;
        return new Person(obj);
    }
}

My main interest is that class Person has a dependency on a function located on an instance of class Convert. My question is this -- is this taking dependency injection too far? Am I better off just importing an instantiated version of Convert into Person.js and referencing it directly? I know that will work, but I was trying to avoid coupling the two classes without a nice way to override. I definitely don't like what I have written here, since Convert will be defined on each Person, which is worse than coupled classes in my opinion.

I had started working on modularizing one of my libraries to decouple classes and use dependency injection, but my pattern broke down when I ran across an instance similar to this. Any suggestions are definitely welcome.

Side note, I know in this example I could just use the Convert methods inside the Factory instead of inside Person -- that doesn't work for my actual project, unfortunately.

Am I better off just importing an instantiated version of Convert into Person.js and referencing it directly? I know that will work, but I was trying to avoid coupling the two classes without a nice way to override. I definitely don't like what I have written here, since Convert will be defined on each Person, which is worse than coupled classes in my opinion.

There is nothing wrong with requiring a data conversion service. The problem is coupling the conversion service with the Person class. This coupling should occur in the Factory class instead.

Furthermore, you haven't coupled the classes together. ECMA Script is a dynamically typed language. At worst you've coupled the Person class to an object supporting the same public interface as the Convert class.

Solving this dilemma is pretty simple. The data conversion should happen in the factory:

factory.js

import {Person} from './person.js';

export class Factory {

    constructor({ convert }) {
        this.convert = convert;
    }

    person(obj) {
        obj.first = this.convert.toString(obj.first);
        obj.last = this.convert.toString(obj.last);

        return new Person(obj);
    }
}

person.js

export class Person {
    constructor({ first, last }) {
        this.first = first;
        this.last = last;
    }
}

This keeps your Person constructor clean and straight forward. The messiness of converting data types is encapsulated in the Factory class where it belongs, and the Person class no longer has a dependency on the Convert class.


Swift string multiplication I like Ruby's string multiplication so I thought I'd see if I could do it in Swift. I came up with these implementations:

@infix func * (left: String, right: Int) -> String {
    var result = ""
    for _ in 0..right {
        result += left
    }
    return result
}

operator infix ** {}

@infix func ** (left: String, right: Int) -> String {
    return Array(count: right, repeatedValue: left).reduce("", +)
}

println("Andrew rules! " * 42)
println("Andrew rules! " ** 42)

Anyone have improvements or other implementations?

As Flambino points out, the method is very problematic for negative integers.

You need to either change the function to only accept unsigned integers or come up with some solution that makes sense for negative numbers.

In general though, I don't particularly like operator-overloading.  Using functions/methods means there's some plain-English name to the operation you're performing that will give me some sort of hint at what the operation does so I don't have to guess.

As someone coming from an Objective-C/Foundation background (which will likely be most Swift programmers) and having zero Ruby experience, I could maybe guess at what the first one does:

println("Andrew rules! " * 42)

But as for the second one?

println("Andrew rules! " ** 42)

I'd have absolutely no clue.  And I doubt I'd be the only one.

The amount of time you save typing ** instead of a more descriptive function name (which the IDE will autocomplete for you meaning the amount of time is basically the same either way) is definitely not worth the amount of time that other programmers behind you take to go figure out what this does:

let myVar = "Hello world!" ** 27

There's not even the slightest hint as to what this returns, and there's no descriptive function name to give any clues... so now I have to go hunt down where ever you've put the operator overload function for a string/int.


Spring MVC @ControllerAdvice exception handling This is about handling sql - foreign key constraint failure exceptions (SQL Error: 1452, SQLState: 23000) globally using Spring's controller advices and Hibernate - ConstraintViolationException.class.

I'm working on a small scale REST API which has 28 database tables and contains 32 foreign keys.

To handle exceptions on entire project, in my approach has two @ControllerAdvice classes, one for handle Servlet exceptions and the other for handle Hibernate sql exceptions.

In my RootDataAccessExceptionsHandler class (sql exceptionsare being handled here), has following @ExceptionHandler method.

@ExceptionHandler(value = {ConstraintViolationException.class})
protected ResponseEntity<Object> handleConstraintViolationException(
        ConstraintViolationException ex,
        WebRequest request
) {
    String cause = ex.getCause().toString();
    LOGGER.error("[ APP Exception ] : {}", ex.getCause().toString());
        switch (ex.getErrorCode()) {
            // catch Cannot add or update a child row:
            // a foreign key constraint fails exceptions
            case 1452: {
                return handleExceptionInternal(
                        ex,
                        getSqlForeignKeyViolationExceptionsPreProcessor().generate1452SqlExceptionResponse(ex.getSQLException()),
                        new HttpHeaders(),
                        HttpStatus.NOT_FOUND,
                        request
                );
            }
            // catch other sql violations - to be developed
            default: {
                return handleExceptionInternal(
                        ex,
                        createDefaultExceptionErrorResponse(),
                        new HttpHeaders(),
                        HttpStatus.NOT_FOUND,
                        request
                );
            }
        }

    }

}

And generate1452SqlExceptionResp(SQLException) is responsible for generate appropriate error response as follows,

Simply this method scans for the key which is violated using getMessage() of SQLException class and then generate error response.

public ResponseEntity<?> generate1452SqlExceptionResponse(SQLException sqle) {
    LOGGER.error("[ APP Exception SQL ] : {}", sqle.getErrorCode());
    LOGGER.error("[ APP Exception SQL ] : {}", sqle.getSQLState());

    if (sqle.getMessage().contains("FK_ID_001_USER_ID")
            || sqle.getMessage().contains("FK_ID_006_USER_ID")) {
        return ResponseEntity
                .status(HttpStatus.NOT_FOUND)
                .body(
                        ErrorServiceMessage
                        .getBuilder()
                        .putCode("ERROR")
                        .putType("Unidentified user")
                        .putPayload("Given user cannot be found")
                        .build()
                );
    }
}

In here, FK_ID_001_USER_ID and FK_ID_006_USER_ID are two foreign keys, but like this, this method has bunch of conditional checks defined in order to process all 32 key violations.

Example

When an operation tries to do something with USER_ID - 10 (Now there's no user associated with this ID), an error will be generated and returned as JSON object asfollows,

{
    "headers": {},
    "body": {
        "code": "ERROR",
        "type": "Unidentified user",
        "payload": "Given user cannot be found"
    },
    "statusCode": "NOT_FOUND",
    "statusCodeValue": 404
}

Please review this approach and what are the drawbacks can be found on this.
Is there any other better approach ?
How can i improve this ?

Your approach seems well written and structured code-wise. However I feel that there is room to improve the structure of the application and the separation of concerns among the classes thus producing a more SOLID result.

My biggest concern is with the web-layer (The Controllers)having to deal with SQL related errors. You do not really describe how your application is structured, but I believe you have your logic implemented within the Controller methods.

If you enclose the application logic in a service class (e.g. UserService) there you could perform data validation and exception handling on the incoming request. For example you should check that the user exists before trying to run update queries that result in an Exception being thrown. You could then throw a custom ResourceNotFoundException with the custom message your ErrorServiceMessage generates and have your ControllerAdvice ExceptionHandler just wrap this to a ResponseEntity.

By hard coding your index names into your error handling classes. then you make it more error prone. What will happen if someone renames the Index? Additionally every time a new Index is being added you would then have to update the generate1452SqlExceptionResp method.

I hope you find my comments useful and thought provoking.

Best Regards


Display a number as a product of its prime factors Task:

Link to original problem :https://www.spoj.com/problems/FACTCG2/

  The task in this problem is to write a number in a multiplication of
  prime
      numbers separated by “x”. You need to put the number 1 in this multiplication.

  Input:

  The input consists of several lines.

  Each line consists of one integer N (1 <= N <= 10^7) .

  Example Input:

           1
           2
           4
           8

  Example Ouput:

           1
           1 x 2
           1 x 2 x 2
           1 x 2 x 2 x 2

My approach:

I pre-compute an array Primes which contains the smallest prime factor of every number from 1 to 1e7 using the sieve.  Then to find the answer , I divide the number given in each query by its least prime factor until it becomes '1' or a prime number. Then I display it.

The code works for small inputs but SPOJ gives me time limit exceeded. I think maybe the vector in factorise function has
            something to do with it. I tried printing the answer without storing it in vector but 'x' makes it difficult .

My code:

#include <bits/stdc++.h>
using namespace std;
vector<long long>Primes(1e7);
bool prime(long long x)
{

    for(int i=2;i<=sqrt(x);i++)
    {
        if(x%i==0)
        return false;
    }
    return true;
}
void least_prime_factor()   //To store least prime factor of a
                             //  given number
 {

    Primes[1]=1;
                    for(int i=2;i<=1e7;i++)
                    {
                            if(Primes[i]==0)
                            {
                               Primes[i]=i;
                               for(int j=2;i*j<=1e7;j++)
                                {
                                    if(Primes[i*j]==0)
                                    Primes[i*j]=i;
                                }

                             }
                    }

 }
vector<long long>factorise(int x)  //This is to store the factors
                                   //of a number

{
    vector<long long>ret;
    while(x!=1||prime(x)!=true)
    {
        ret.push_back(Primes[x]);
        x=x/Primes[x];
    }
return ret;
}

int main() {

least_prime_factor();
int n;
while(scanf("%d", &n) == 1)
if(n==1)
{
    cout<<"1"<<endl;
}
else{

    vector<long long>ans=factorise(n);
    cout << "1 x" ;
    for(int i=0;i<ans.size();i++)
    {
        if(i==ans.size()-1)
            printf(" %d ", ans[i]);
        else
            printf(" %d x", ans[i]);
    }
    cout<<endl;
}
    return 0;
}

Don't include <bits/stdc++.h> - it's not standard, and on platforms that provide it, it drags in far too much.

Don't using namespace std - this namespace is not designed for wholesale inclusion, and can produce subtle bugs when its names overload your own.

Don't use a floating-point number such as 1e7 when you want a simple integer 10'000'000.

Don't compute std::sqrt(x) every time around the loop - save the value as an integer for faster comparisons.

Use unsigned types for unsigned arithmetic.  Given that inputs (and therefore factors) are no greater than 10⁷, we can get away with std::uint_fast32_t rather than long long.

Although the code claims to use a sieve, every input number is inefficiently tested for primality without reference to the sieve (multiple times, even).

Why use a std::vector rather than std::array for something whose size is constant and known at compile time?  Also, why write the size in two places (which need to be consistent) instead of simply using Primes.size()?  (Also, fix the off-by-one error where we overstep the end of the vector).

I recommend using C++ streams, rather than mixing C buffered I/O and forcing the two libraries to repeatedly flush each other.

Avoid global variables - Primes can be a local static member (perhaps with an immediately-executed lambda expression as initializer).

Why not always include 1 as the first result, rather than have to add that in the printing code?

To dive in to one function in particular, let's pick on least_prime_factor().  Apart from the misleading name, we're doing a lot more work than we need to here.  Every time around the loop, we're multiplying i by j (a good compiler will hoist the three i*j into a single computation, and reuse that), but we could simply be adding i each time.

A standard improvement to naive sieves is to observe that every multiple of a prime p that's less than p² also has smaller factor than p, so once we've marked p, we can jump straight to p² before striding by p thereafter.

Apart from the first pass (which marks even numbers), we can stride by 2 p each time (knowing that we've already marked even numbers).  We can take that further with a wheel of factors, but even this is reducing the work by a factor of two.

Let's see what we have now:

// Create an array populated with the smallest prime factor of each
// number, or 0 for primes (and unity).
void least_prime_factor()
{
    Primes[1]=1;
    // First, mark all the even numbers; this allows us to halve the
    // work for all other primes.
    for (std::size_t i = 2;  i < Primes.size();  i += 2) {
        Primes[i] = 2;
    }

    for (std::size_t i = 3;  i < Primes.size();  ++i) {
        if (Primes[i]) { continue; }

        Primes[i] = i;
        for (std::size_t j = i * i;  j < Primes.size();  j += 2 * i) {
            if (!Primes[j]) {
                Primes[j] = i;
            }
        }
    }
}

That's noticeably less work, and if we also replace prime() with a simple lookup in Primes[], then we'll have a much more efficient program.


Determine if Array has an Increasing Sequence I wrote a function in JavaScript that expects an array of integers (negative or positive) and determines if that array has an increasing sequence.

For the sake of better time performance I made the function return out of the loop with false as soon as i+1 - i did not equal 1 or -1. I used continue to do this. Here is the function:

const increasingSequence = (arr) => {
    for (let i = 0; i < arr.length - 1; i ++) {
      if (arr[i+1] - arr[i] === 1 || arr[i+1] - arr[i] === -1) {
        continue;
      } else {
        return false;
      }
    }
    return true;
  }

Seems to be producing the desired output:

var arr1 = [1, 2, 3, 4];
var arr2 = [1, 2, 4, 5];
var arr3 = [-4, -3, -2, -1];
var arr4 = [-4, -3, -1, 0];

increasingSequence(arr1); // true
increasingSequence(arr2); // false
increasingSequence(arr3); // true
increasingSequence(arr4); // false

This is actually a helper function to a bigger problem I'm trying to solve, so time complexity is important. Feedback about improving time complexity or any missing edge cases would be greatly appreciated.

Your code has a bug because it will accept decreasing sequences as true as well, for example, increasingSequence([4,3,2,1]); will be true.

Having said that, you could do the tests a whole lot simpler by using a search:

  const increasingSequence = (arr) => !arr.some((val, idx) => val != arr[0] + idx);

How does that work? Well, each element in the array is expected to be the value of the first item, plus the index address of the current item, so, if the first value is 4 then the 3rd value will be expected to be 6 (4 + 2).

The above code simply looks for any value that does not meet those expectations.

If there is a value that does not meet the expectations, the some returns true, so we negate that, and return false.

Since the some(...) method is a short-circuiting function so it will terminate when the first mismatch is encountered (just like your logic).

Also, note that the short-circuit does not impact the time-complexity of the function, it is still \$O(N)\$ because the performance will scale linearly in relation to the number of elements.

You can see this running on Ideone.


Matrix Diagonal Difference Problem

Given a square matrix of size N×N, calculate the absolute difference between the sums of its diagonals.

Input Format

The first line contains a single integer, N. The next N lines denote the matrix's rows, with each line containing N space-separated integers describing the columns.

Output Format

Print the absolute difference between the two sums of the matrix's diagonals as a single integer.

Sample Input

3
11 2 4
4 5 6
10 8 -12

Sample Output

15

Code

(ns hackerrank.core
  [:require [clojure.string :as s]])

(defn get-diagonal-sums-reducer
  [n]
  (fn
    [sums [line-number line]]
    (let
      [primary (nth line line-number)
       secondary (nth line (- n (+ 1 line-number)))]
      (assoc
        sums
        :primary (+ primary (:primary sums))
        :secondary (+ secondary (:secondary sums))))))

(let
  [n (Integer/parseInt (read-line))
   matrix (for
            [_ (range n)]
            (mapv #(Integer/parseInt %) (s/split (read-line) #"\s+")))
   matrix-enumerated (map-indexed vector matrix)
   sums (reduce
          (get-diagonal-sums-reducer n)
          {:primary 0 :secondary 0}
          matrix-enumerated)
   difference (- (:primary sums) (:secondary sums))]
  (println (max difference (- difference))))

Couple questions in particular:

can I avoid the get-diagonal-sums-reducer somehow? I'm only using it so that it closes over n.
am I doing this as lazily as possible or is there any place I'm using vectors when seqs would have worked?
is there a much less verbose way to do this? Doesn't feel like this would be as complicated in Python.

Could you have avoided get-diagonal-sums-reducer?

You probably could have used partial if you wanted, though I'm not sure it ends up that much better:

(defn diagnoal-sums-reducer [n sums [line-number line]]
  ...)

(reduce (partial diagonal-sums-reducer n) ...)

Is there a much less verbose way to do this.

I think there is.  I can see why you've tried to use reduce, but your reducer is doing way more than it should be.  Essentially, there's 3 things it's doing:

Calculating the coordinates of the  diagonals for the line.
Getting the values of those coordinates.
Summing the values together with the previous values.

There might be some situations where you'd need to do all of those things in a single reduce, but I don't think this is one of those cases.  It's also complicated by having to do this for both the primary and the secondary cases.

It ends up much simpler if you split the whole process out into the individual steps:

(defn sum [x] (apply + x))

(let [n 3
      matrix [[11 2 4]
              [4 5 6]
              [10 8 -12]]

      ;; Build up a list of coordinates for the diagonals.
      primary-coords (for [i (range n)] [i i])
      secondary-coords (for [i (range n)] [(- n i 1) i])

      ;; Extract the values of those coordinates
      primaries (map #(get-in matrix %) primary-coords)
      secondaries (map #(get-in matrix %) secondary-coords)]

    ;; Sum them, take the absolute difference
    (Math/abs (- (sum primaries) (sum secondaries))))


Practical number algorithm I am trying to write a program to find the practical numbers, from an input from \$1\$ to \$n\$.

Practical numbers

My code is running correctly but it is extremely slow when calculating numbers around 50 - it gets stuck at 44.

import Foundation

func getInteger() -> Int {
    var firstNum:Int = 0
    while true {
        // get value from user. Using optional input since readLine returns an optional string.
        let input = readLine()
        // ensure string is not nil
        if let unwrappedInput = input {
            if let unwrappedInt = Int(unwrappedInput) {
                firstNum = unwrappedInt
                break
            }
            else { // the input doesn't convert into an int
                print("`\(unwrappedInput)` is not an integer. Please enter an integer")
            }
        }
        else { // did not enter anything
            print("Please enter an integer")
        }
    }

    return firstNum
}
func addOne(signArray: [Int]) -> [Int] { // finds the combinations
    var signArray2 = [Int]()
    for i in 0...signArray.count-1 {
        signArray2.append (signArray[i])
    }

    for i in 0...signArray2.count-1 {
        if signArray2[i] == 1 {
            signArray2[i] = 0
        }
        else {
            signArray2[i] = 1
            break
        }
    }
    return signArray2
}
func signEval (signArray: [Int], divArray: [Int], inNum: Int) -> Bool  {// changes 2nd
  var counts = 0
    for i in 0...divArray.count-1 {
        if signArray[i] == 0 {
counts = divArray[i] + counts        }
        if counts == inNum {
            return true
        }
    }
        return false
}
print("Please enter a number to find the summable numbers up to that number:")
var input2 = getInteger()// if num = 1 print 1 if num = 2 print 1 and 2 else print >2 1, 2
var inNum = 0
var numHalf = 0.0
var numRound = 0.0
var numCheck = false
var numCheck2 = false
var numQuarter = 0.0
var numSixth = 0.0
var divArray:[Int] = []
var theirArray = [Int]()
var signArray = [Int]()// array of 0s and 1s
var summableArray:[Int] = [1,2] // need to check if num is bigger than 2!
for input in 1...input2 {
        numHalf = Double (input) / 2.0
        numRound = round(numHalf)
        if numRound == numHalf {
            numCheck = true }
        if input > 2 && numCheck == false { // odd numbers greater than one are not summable
        }
        else { // these are possible summable nums
            numQuarter = Double (input) / 4.0
            numRound = round(numQuarter)
            if numRound == numQuarter {
                numCheck = true
            }
            else {
                numCheck = false
            }
            numSixth = Double(input) / 6.0
            numRound = round(numSixth)
            if numRound == numSixth {
                numCheck2 = true }
            else { numCheck2 = false}
            if numCheck == true || numCheck2 == true {
        theirArray = []
        divArray = []
        signArray = []
        summableArray = []
for i in 1...input {
    theirArray.append (i)
    }
for i in 1...input { // creates an array of all the diviors of inputted number
    if input%i == 0 {
        divArray.append (i)
    }
}
        for j in 1...divArray.count {//
            signArray.append(0)
        }

for i in 1...input{
let x: Int = Int(pow(Double(2),Double(input-1)))// int 2 to the power of input -1
    var Boolcheck = false

    for q in 1...x-1 { // i to 2^n -1 (sequence to check)
    Boolcheck = (signEval(signArray: signArray, divArray: divArray, inNum: i))// checks
        signArray = addOne(signArray: signArray)// adding the ones to the array
        if Boolcheck == true {
        summableArray.append(i)// creates array of mini summable numbers
        break
    }
        }
   if summableArray.count == input {
      print ("\(input)")
    }
}
    }
}
}

Some points in addition to what Roland already said:

You are correct that the readLine() returns an optional and you need
to ensure that it is not nil. However, nil is only returned on an
end-of-file condition, which means that it makes no sense to call
readLine() again. You can only terminate the program in that
situation. That is a typical use-case for guard:

func readInteger() -> Int {
    while true {
        guard let line = readLine() else {
            fatalError("Unexpected end-of-file")
        }
        if let n = Int(line) {
            return n
        }
        print("Please enter an integer")
    }
}

Now let's have a look at

var signArray2 = [Int]()
for i in 0...signArray.count-1 {
    signArray2.append (signArray[i])
}

First, this will crash if signArray is empty. Better use a half-open
range instead:

for i in 0..<signArray.count { ... }

or iterate over the indices

for i in signArray.indices {
    signArray2.append (signArray[i])
}

or just iterate over the elements:

for e in signArray {
    signArray2.append(e)
}

But actually, you are just copying the array:

var signArray2  = signArray

Determination of the divisors should be done in a separate function.
Your code

divArray = []
for i in 1...input {
    if input % i == 0 {
        divArray.append (i)
    }
}

can be simplified to

divArray = (1...input).filter { input % $0 == 0 }

There are various ways to make this faster. For example the observation
that with each divisor \$ i \$ of a number \$ n \$, \$ n/i \$ is another
divisor (see for example Find all divisors of a natural number).
That allows to reduce the number of loop iterations to the square-root of the
given number, and could look like this:

func divisors(n: Int) -> [Int] {
    var divs = [Int]()
    for i in 1...Int(sqrt(Double(n))) {
        if n % i == 0 {
            divs.append(i)
            if n/i != i {
                divs.append(n/i)
            }
        }
    }
    return divs // or divs.sorted(), if necessary
}

Your code

signArray = []
for j in 1...divArray.count {//
    signArray.append(0)
}

creates an array with the same size as divArray, but filled with
zeros. That can be simplified to

signArray = Array(repeating: 0, count: divArray.count)

There is no need to use string interpolation when printing an integer
(or any single value),

 print ("\(input)")

can be simplified to

 print(input)

Why is your code slow?

There is a logical error in

 let x: Int = Int(pow(Double(2),Double(input-1)))// int 2 to the power of input -1

because the number of possible combinations of divisors is
just \$ 2 ^ {\text{number of divisors}} \$, which can be computed as

 let x = 1 << divArray.count

With that change, your program computes all practical numbers
up to 200 in 0.2 seconds, and up to 1,000 in 25 seconds
(on a MacBook, compiled in Release mode, with optimization).

A faster algorithm

In order to determine if \$ n \$ is a practical number,
you test all numbers \$ 1 \le i \le n \$ if they are a sum
of distinct divisors of \$ n \$, and for each \$ i \$ that is done by building all
possible sums of divisors, until \$ i \$ is found.

It is more efficient to work the other way around. From the list of
divisors of  \$ n \$, build a list of all sums of distinct divisors.
This can be done iteratively: Starting with  \$ 0 \$, the first, second, ...
divisor is added to the numbers obtained previously.
Then check if all numbers from \$ 1 ... n-1 \$ are in that list.

The following implementation uses a boolean array to mark all numbers
which are confirmed as sums of divisors. In addition, it uses that
powers of two are always practical numbers (using the method from
Determining if an integer is a power of 2).

func divisors(_ n: Int) -> [Int] {
    var divs = [Int]()
    for i in 1...Int(sqrt(Double(n))) {
        if n % i == 0 {
            divs.append(i)
            if n/i != i {
                divs.append(n/i)
            }
        }
    }
    return divs.sorted()
}

func isPractical(n: Int) -> Bool {
    // 1 and 2 are practical numbers:
    if n == 1 || n == 2 {
        return true
    }
    // Every other practical number must be divisible by 4 or 6:
    if n % 4 != 0 && n % 6 != 0 {
        return false
    }
    // Every power of 2 is a practical number:
    if n & (n-1) == 0 {
        return true
    }

    var isSumOfDivisors = Array(repeating: false, count: n)
    isSumOfDivisors[0] = true
    var last = 0 // Index of last `true` element.

    // For all divisors d of n (except n):
    for d in divisors(n).dropLast() {
        // For all i which are a sum of smaller divisors (in reverse order, so that
        // we can simply update the array):
        for i in stride(from: min(last, n-1-d), through: 0, by: -1) where isSumOfDivisors[i] {
            // Mark i + d:
            isSumOfDivisors[i + d] = true
            if i + d > last {
                last = i + d
            }
        }
    }

    // n is "practical" if isSumOfDivisors[i] == true for all i = 0...n-1:
    return !isSumOfDivisors.contains(false)
}

Test code:

let startDate = Date()
let practicalNumbers = (1...10_000).filter(isPractical)
let endDate = Date()
print(endDate.timeIntervalSince(startDate))

print(practicalNumbers)

On my MacBook, this computes the practical numbers up to 1,000 in 0.003 seconds, and up to 10,000 in 0.1 seconds.


Selecting the number of working days minus weekend days and UK bank holidays I am fairly new to coding TSQL script and am looking for some second view on my script.
The goal here is to to pull in some data and amongst that data to show a field with a counting the number of working days since the order date only (i.e. minus weekend days and UK bank holidays). The SELECT script below is working as expected in that it shows results and is therefore calling the fn_WorkDays function. What would be helpful is to have a second glance of the function itself as a double check/review. The last part of the function is looking at a date field of a table that contains all of the bank holidays.

I think the script is working okay as I have checked a few going back to January, so I'm looking to get the double check to highlight any unforeseen potential errors or even code improvements.

SELECT
cir.[PW Number] AS PWNum
--Get the correct number of working dayts since the order date by using the fn_WorkDays function
,CONVERT(VARCHAR(10),singleended2.dbo.fn_WorkDays(cir.[Install Date], GETDATE()),103) AS NumberOfDaysSinceOrderDate
,CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103) AS  OrderDate--Get the order dates
,ISNULL(cirRep.CurrentStage, 'Not Set') AS CurrentStage
,cir.[ID] as CircuitID
FROM Quotebase.dbo.Circuits cir
    LEFT JOIN Quotebase.dbo.CircuitReports cirRep ON Cir.[PW Number] = CirRep.PWNumber
WHERE Cir.Status='New Circuit Order'
ORDER BY Cir.[PW Number]

Here is the function script that is called:

ALTER FUNCTION dbo.fn_WorkDays (@StartDate AS DATETIME, @EndDate AS DATETIME)
--Define the output data type.
RETURNS INT
AS
--Calculate the RETURN of the function.
BEGIN
    RETURN (
     SELECT
        (DATEDIFF(dd,@StartDate, @EndDate)+1)--Start with total number of days including weekends +1 Includes the day run
        -(DATEDIFF(wk,@StartDate, @EndDate)*2)--Subtact 2 days for each full weekend
        -(CASE WHEN DATENAME(dw, @StartDate) = 'Sunday' --If StartDate is a Sunday, Subtract 1
            THEN 1
            ELSE 0
        END)
        -(CASE WHEN DATENAME(dw, @EndDate) = 'Saturday'--If EndDate is a Saturday, Subtract 1
            THEN 1
            ELSE 0
        END)
        --Now check if there are any bank holidays between the start and end and remove that amount
        - (SELECT COUNT(ivm.[Date])
          FROM [EUROPEVUK386].[InvoiceManagement].[dbo].[tblBankHolidays] ivm
          WHERE ivm.[Date] BETWEEN @StartDate AND @EndDate)
          --WHERE ivm.[Date] BETWEEN '2006-04-14' AND '2006-05-29')
)
 END
GO

ISNULL, which you use a few times, is valid T-SQL, but there is an argument for using the more portable COALESCE instead. They're not straight aliases, though, so consider whether this is appropriate.
Comparing the two lines

,CONVERT(VARCHAR(10),singleended2.dbo.fn_WorkDays(cir.[Install Date], GETDATE()),103) AS NumberOfDaysSinceOrderDate
,CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103) AS  OrderDate--Get the order dates

you assume in the second line that cir.[Install Date] might be null, but the first line passes it directly to a function which has no special-casing for that situation. Is this intentional?
I think you're overdoing things in

CONVERT(VARCHAR(10), ISNULL(cir.[Install Date],'01/01/1900'),103)

You're trying to convert the date to a dd/mm/yyyy string, but if it's NULL you take a string in that format, convert it with whatever the default date format happens to be (although since the day and month are the same you have good odds of getting the right result), and then convert it back. Why not push in the CONVERT and handle the fallback outside?
You're making assumptions about locale-related settings inside the function. It's understandable that you hard-code an assumption about which days are the weekends: you're only interested in one culture, so that's a business-level setting. But the localisation of the values returned by DATENAME is a per-connection setting. You might think that your function will always be executed in an English locale, and you might even be right, but it's a small change to replace DATENAME with DATEPART, and it makes the code slightly more robust.

(I've had experience of maintaining a system built for a British organisation but which relied implicitly on using a US locale; since my dev machine has a UK locale, lots of things broke. Don't discount the possibility that in future your code will be maintained by someone from another country who configures their dev machine differently - or even that the production machine might be moved to a server with a different locale).
For the benefit of future maintenance programmers, who may not be SQL-Server experts or domain experts on UK bank holidays, it would be nice to add some comments pointing out that DATEDIFF ignores @@DATEFIRST in order to be deterministic, and that a bank holiday will never be on a weekend.