Optimization over 7 constants, running a function 10,000,000 times I am not quite sure how to attach my data, but it is 5737 observations of 3 variables. Variables V and P are strictly < 0, and variable R is -2 < R < 2.

The last function, correcttot, will help me find the optimal constants, or ranges of constants, for this function. However, the function that it runs over, percentcorrect, takes several seconds to run, which means that repeating it 10,000,000 times is not feasible.

Data

setwd("~/Desktop")
dat<-read.csv(file="data.csv",sep=",",header=T)
attach(dat)
###################################################################################
voltoalph<-function(v,c){  # Where V is a data vector and C is a constant
  alpha<-c(rep(0,length(v)))
  for(i in 1:length(v)){
    alpha[i+1]<-abs(c*(v[i]/max(v,na.rm=TRUE)))
  }
  alpha
}
voltoenen<-function(v,c){  # Where V is a data vector and C is a constant
  enen<-c(rep(1,length(v)))
  for(i in 1:length(v)){
    enen[i+1]<-abs((v[i]/max(v,na.rm=TRUE)))
  }
  enhelp<-1/(enen)
  enhelp2<-c*(enhelp/max(enhelp))
  enhelp2
}

ema<-function(v,n,a){     # Where V is a data vector and n is a contant and a is a constant
  avevec<-c(rep(0,n))
  for(i in 1:n){
    avevec[i]<-((1-a[i])^(n-i))*v[i]}
  divvec<-c(rep(0,n))
  for(i in 1:n){
    divvec[i]<-((1-a[i])^(n-i))
  }
  sum(avevec)/sum(divvec)
}
betaema<-function(v,n,a,l){ # Where V is a data vector and n,a,l are constants
  secondvec<-c(rep(0, length(v)))
  for(i in l:length(v)){
    secondvec[i]<-ema(v[(i-n[i]+1):i],n[i],a)
  }
  secondvec
}
#################################################################################################################
howright<-function(v,r,c,l){   # Where v and r are data vectors and c,l are constants
  rightvec<-0
  for(i in l:(length(r)-c)){
    if((v[i]*mean(r[i+1]:r[i+c]))>0){
      rightvec<-rightvec+1
    }
    else{
      rightvec<-rightvec
    }
  }
  rightvec/(length(r)-l-c)
}
#################################################################################################################
percentcorrect<-function(ca1,ca2,cn1,cn2,e,d,c,v,p,r){   ###V, P, and R are data vectors, rest constant
  vol1<-voltoalph(v,ca1)
  vol2<-voltoalph(v,ca2)
  ens1<-voltoenen(v,cn1)
  ens2<-voltoenen(v,cn2)
  als<-c(vol1)
  als2<-c(vol2)
  n1<-c(ens1)
  n2<-c(ens2)
  anotherema1<-betaema(p,n1,als,max(cn1,cn2))
  anotherema2<-betaema(p,n2,als2,max(cn1,cn2))
  slope1<-c(rep(0,length(p)))
  slope2<-c(rep(0,length(p)))
  for(i in (max(cn1,cn2)+d):length(anotherema1)){
    slope1[i]<-(anotherema1[i]-anotherema1[i-d])/d
  }
  for(i in (max(cn1,cn2)+e):length(anotherema2)){
    slope2[i]<-(anotherema2[i]-anotherema2[i-e])/e
  }
  sig<-slope1-slope2
  hvec<-howright(sig,r,c,max(cn1,cn2))
  hvec
}
##########################################################################################
correcttot<-function(v,p,r){    ###Where v, p, and r are data vectors
  correct3<-array(0,dim=c(10,10,10,10,10,10,10))
  for(i in 1:10){
    for(j in 1:10){
      for(k in 1:10){
        for(l in 1:10){
          for(m in 2:10){
            for(n in 2:10){
              for(o in 1:10){
                correct3[i,j,k,l,m,n,o]<-percentcorrect((i/10),(j/10),(20*k),(20*l),m,n,o,v,p,r)
              }
            }
          }
        }
      }
    }
  }
  print(correct3)
}
newvec<-correcttot(vl,p,rt)  # run it on the vectors vl, p and rt
which(newvec==max(newvec2),arr.ind=TRUE)

You have several loops that are completely unnecessary. Remove them and you will see dramatic speedups. Take voltoalph and voltoenen for example:

voltoalph2 <- function(v,c) {
  # v: a data vector
  # c: a scalar constant
  c(0, c[1L] * (v / max(v, na.rm=TRUE)))
}
voltoenen2 <- function(v,c) {
  # v: a data vector
  # c: a scalar constant
  enhelp <- 1 / c(1, abs(v / max(v, na.rm=TRUE)))
  c[1L] * (enhelp / max(enhelp))
}
require(microbenchmark)
v <- 1:10000
microbenchmark(voltoalph(v, 2), voltoalph2(v, 2))
# Unit: microseconds
#              expr        min          lq        mean     median          uq        max neval cld
#   voltoalph(v, 2) 105004.652 106038.7245 108767.0230 106725.827 108326.7975 151482.035   100   b
#  voltoalph2(v, 2)     81.925     83.5825    141.3382     89.794     91.6065   5350.052   100  a
identical(voltoalph(v, 2), voltoalph2(v, 2))
# [1] TRUE
microbenchmark(voltoenen(v, 2), voltoenen2(v, 2))
# Unit: microseconds
#              expr        min         lq        mean     median        uq        max neval cld
#   voltoenen(v, 2) 103595.526 105433.803 108797.6308 106882.274 110160.65 148550.397   100   b
#  voltoenen2(v, 2)    200.275    203.182    227.6582    207.932    220.57   1007.375   100  a
identical(voltoenen(v, 2), voltoenen2(v, 2))
# [1] TRUE

The vectorized (non-loop) versions are over 1000, and 500 times faster (for voltoalph and voltoenen, respectively), and provide identical output. percentcorrect calls each of those functions two times, which makes the speed improvement have twice the impact.


Name scope for memoization Fibonacci Here's a snippet of code I use to examine the functionality of Python decorator(@memorize). An example of Fibonacci computation:

def memorize(f):
    memo = {}
    def helper(*args):
        if args not in memo:
            memo[args] = f(*args)
        return memo[args]
    return helper

def fib(n):
    if n==0:
        return 0
    elif n==1:
        return 1
    else:
        return fib(n-1) + fib(n-2)

Here's the problem:

Different naming is causing a huge speed difference, why is it?

Execution 1:

s = time.time()
fib = memorize(fib)
a = fib(40)
e = time.time()
print(a)
print(e-s)

returns

  102334155

  7.319450378417969e-05

Execution 2:

s = time.time()
memo_fib = memorize(fib)
a = memo_fib(40)
e = time.time()
print(a)
print(e-s)

returns

  102334155

  46.79982662200928

Edited:

A screen copy from running the code

Edited:

Unless running the two executions seperately, to obtain aforementioned results, "Execution 2" must be run before "Execution 1".

fib works by recursively calling the function named fib:

def fib(n):
    if n==0:
        return 0
    elif n==1:
        return 1
    else:
        return fib(n-1) + fib(n-2)

In your "execution 1" the function named fib is the memoized version of the function, because you have assigned it like this:

fib = memorize(fib)

But (assuming that you haven't run "execution 1" yet), in your "execution 2" the function named fib is the original function (not the memoized version of the function, which you have assigned to memo_fib), so when you call memo_fib it calls the original fib and when that recurses it calls the original fib, bypassing the memoization.


Rebol View layout compose, seek minima notation I am attempting to break down a Rebol View into a Forth style 'divide and conquer' word set, to optimize readability and organization, and to segregate the business logic from the view layout inner workings.

Business intelligence

Field

declaration
initialization
processing

Software intelligence

view layout

Is there a way to simplify the following code?

    REBOL[]

    Result: "Test"

    field1.Process: [
                alert join  "You typed: "
                Result: field1/text
                ]

    field1.Initialize: [do [field1/text: Result]]

    field1.Declare: compose/deep [
            field1: field
            [(field1.Process)]
            (field1.Initialize)
        ]

    block:     compose [
            (field1.Declare)
            field
            field
            ]

    view layout (block)

Layout block structure being

field: field-type [field-process]     [field-initialize]

I seek to optimize notation

field1.Process: []
field1.Initialize: [do []]

field1.Declare: compose/deep [
        field1: field
        [(field1.Process)]
        (field1.Initialize)
    ]

Towards minima

field1.Process: []
field1.Initialize: []

field1.Declare: compose/deep [
        field1: field
        [field1.Process]
        [field1.Initialize]
    ]

Try to avoid mixed casing in new dialect designs

Rebol preserves the case of characters when they match the lexical rules to become a symbolic type in the homoiconic structure being produced:

>> to-string quote field1.Process
== "field1.Process"

>> to-string quote field1.process
== "field1.process"

Yet despite keeping the case in the symbol, it does not use this to distinguish lookups in the (DO "dialect") evaluator:

>> field1.Process: 10

>> print field1.Process
10

>> print field1.process
10

This is why you will rarely see mixed case symbols in Rebol.  That is, unless they are in a block representing a dialect that was specially written to have case-sensitive behavior.  Your example here is not quarantined in that way.  It's all just running in the default evaluator.

Even if it were quarantined in a dialect: not only is lowercase typing consistent with the language's lookup rules, it's easy on the shift key (as is Rebol's choice of square brackets for the primary block delimiter...)  Try to stick with it where possible.

Don't fight the lexer, use structure!

When you write something like field1.Process that's going to become a single symbol, you're setting yourself up for further processing work.  Now you have to find the position of the period, separate things out, etc.  Luckily Rebol makes that easy:

>> parse "field1.Process" [copy left to "." skip copy right to end]
== true

>> print left
field1

>> print right
Process

Still, it's a bit of a shame to not think a little more outside the box.  Why not filenames (starting with %) to identify fields, and tags (enclosed in <>) to identify your steps?  You don't need to do any extra work:

>> length? [%field1 <Process>]
== 2

>> type? first [%field1 <Process>]
== file!

>> type? second [%field1 <Process>]
== tag!

Depending on your needs, it might be good to free up ordinary words for your dialect keywords.  I don't have an immediate intuition for what you are trying to accomplish here.  But the point is, the more you can leverage the Rebol typeset the better.  And of course there are paths, which are structural:

>> type? quote field1/Process
== path!

>> length? quote field1/Process
== 2

Don't fight the evaluator, make a dialect!

Remember that COMPOSE is (a) something you could have written yourself, and (b) there to help you as a tool in creating your own dialect.  You can even use the /NEXT refinement of DO to chain to the Rebol evaluator to execute one full expression and stop:

niftyprint: function [spec [block!]] [
    pos: head spec
    reversed: false
    while [not tail? pos] [
        case [
            #backwards = first pos [
                reversed: true
                pos: next pos
            ]

            #forwards = first pos [
                reversed: false
                pos: next pos
            ]

            true [
                result: do/next pos 'pos
                prin either reversed [
                    reverse form result
                ] [
                    form result
                ]
                prin space
            ]
        ]
    ]
    prin newline
]

Trying it out, you'll get:

>> niftyprint [{Sum is} #backwards 1020 + 304 {,looc} #forwards "huh??"]
== Sum is 4231 cool, huh??

So you didn't have to reinvent the wheel to delegate to Rebol's inner logic of figuring out (for instance) that infix addition needs to have its two arguments fulfilled.  This can be a way of reducing the need to go around putting brackets on everything, and it can become more free.

Of course, that freedom has some drawbacks.  You get less checking.  But when the tradeoffs are balanced you can get something that has a lot of the feeling of just writing sentences as one would in English.

Moreover, the PARSE dialect itself can be used to make processing your odd blocks all the easier.  Consider an alternative implementation of the above:

niftyprint: function [spec [block!]] [

    direction: #forwards

    output: function [value [any-type!]] [
        str: form value
        if direction = #backwards [reverse str]
        prin str
        prin space
    ]

    parse spec [
        (print "hi" probe pos print "there")
        any [
           set direction #backwards
        |
           set direction #forwards
        |
           pos: (output do/next pos 'pos) :pos
        ]
    ]
]

Just a few thoughts to consider.

P.S. As with LISP eventually going with the trends and going by Lisp, REBOL has "modernized" by officially changing to Rebol (Although the originating company, currently inactive after the open-sourcing, is still incorporated as REBOL Technologies.)  I personally think it looks better to start scripts with Rebol [], but as mentioned above the lookup is case-insensitive... and mixed case is discouraged.  So it's up to you.  Kind of a special case.


Compare directory entries (sort by access date) I have a comparison function for use in scandir or qsort to sort an array of struct dirent *s by access date. S is the directory path of the directory whose files are being sorted, and SavesLen is strlen(S).
static int Q(const struct dirent **const First, const struct dirent **const Second) {
    const char *const FirstName = (*First)->d_name, *const SecondName = (*Second)->d_name, *const Name[] = {FirstName, SecondName};
    const unsigned short FirstLen = (unsigned short)strlen(FirstName), SecondLen = (unsigned short)strlen(SecondName), Len[] = {FirstLen, SecondLen};
    char File[SavesLen+Len[FirstLen < SecondLen]+2], *CurrPtr = memcpy(File, S, SavesLen)+SavesLen;
    *CurrPtr++ = '/';
    time_t Time[2];
    for (unsigned char I = 0; I < 2; I++) {
        memcpy(CurrPtr, Name[I], Len[I]);
        struct stat Stat;
        if (stat(File, &Stat)){
            return 0;
        }
        Time[I] = Stat.st_atimespec.tv_sec;
    }
    return Time[1]-(*Time);
}

Sorry for being obsessed with microoptimizations, but I'm looking to improve this.
S and SavesLen are both global variables defined and declared and initialized elsewhere.  Is there a way to take advantage of local scopes to delete some of the memory earlier?  Are there other improvements to be made? Thanks ahead of time.
An answerer pointed out that char File[SavesLen+Len[FirstLen > SecondLen]+2] is extremely hard to read. So here's why I do it that way. There are 2 files, and I need to check the access time of both of them. Rather than creating a path for each one, I'm reusing the same path, to minimize the number of VLA declarations and the number of memcpy() calls. Now let's break it down for you. char File[...] is a VLA declaration. The size is SavesLen plus the larger size of the 2 files, plus 2, for the / and null character. So what the heck is Len[FirstLen > SecondLen]? The reason why it's confusing is because I'm treating booleans like integers, and being weakly typed, C lets me do that. Len is an array so it will either be indexed by 0 or 1 (the 2 boolean values) depending on which one is bigger, because FirstLen > SecondLen will either evaluate to 0 or 1.
It seems the answerer understands, but in case there are others confused. I do agree that there was a bug, and that's because it used to be a ternary and I forgot to switch the values.

One declaration per line, please.

char File[SavesLen+Len[FirstLen > SecondLen]+2]; is extremely hard to read. In fact, I suspect that there is a bug. Correct me if I am wrong, an intention is to allocate enough memory to accomodate the longest pathname. Now, if FirstLen is indeed greater than SecondLen, the condition evaluates to true, which is in this context is 1. Now the VLA size becomes SaveLen + Len[1] + 2, that is SaveLen + SecondLen + 2, which is not enough for the longer first name.

The loop running just two iterations is better be two calls to a function. Consider
  static time_t get_atime(const char * filename) {
      char path[SavesLen + 1 + strlen(filename) + 1];
      sprintf(path, "%s/%s", S, filename);

      struct stat stat;
      stat(path, &stat);
      return stat.st_atimespec.tv_sec;
  }

  static int Q(const struct dirent **const First, const struct dirent **const Second) {
      time_t time0 = get_atime((*First)->d_name);
      time_t time1 = get_atime((*Second)->d_name);
      return time1 - time0;
  }

Of course, Q is not a name for a function. compare_atime perhaps?


excel vba discrete random variable udf I am working on writing a discrete random variable generator function in excel vba. I've pulled this off a number of ways but here is the one I like the most.

Imagine in excel we have the following in cells A1:B3

red     5
blue    7
yellow  3

The following code is modeled as follows. Imagine drawing a line going from 0 to 15 (where 15 is 5 + 7 + 3). The line is red from [0,5) and blue from [5,12) and yellow from [12,15). And you pick a uniformly distributed random number, x, from 0 to 15. And wherever on the line x falls, that is the color that wins.

Here is the function:

Function discrete(r1 As Range, r2 As Range)

  'shrink column ranges to contiguous cells
  Set r1 = Range(r1(1), r1(1).End(xlDown))
  Set r2 = Range(r2(1), r2(1).End(xlDown))

  'convert ranges to arrays
  Dim arr1, arr2 As Variant
  arr1 = r1.Value
  arr2 = r2.Value

  Dim n As Long
  n = Application.Sum(arr2) 'sum total of 2nd array

  Dim x As Double
  x = n * Rnd() 'generate a random number 0 <= x < n

  Dim min, max As Double
  min = 0
  max = 0

  Dim i As Long
  For i = LBound(arr1, 1) To UBound(arr1, 1) 'where on the line x falls
      max = max + arr2(i, 1)
      If x >= min And x < max Then
          discrete = arr1(i, 1)
      End If
      min = min + arr2(i, 1)
  Next i

End Function

I'm looking for some critique of my code and approach.

Assuming I'm not picking from 1000 values (column a), is it reasonable that I convert the ranges to arrays? Is this a time saving step? Or should I just work with the ranges? What I mean is, does this step pay for itself computationally given that I may always be working with only a few rows of data? Or might it be worth it solely on that basis that I may indeed supply this function with large ranges?
Even if you supply a full column, ie. discrete(range("a:a"), range("b:b")), the range inside the function is reset to only those cells that are contiguous starting from the first cells of the supplied ranges. Is that a reasonable approach?
The algorithm itself relies on a for loop and 2 simple variables (min, max). I this a lightweight enough approach? Or are there some generally accepted practices I'm not aware of?

Option Explicit

Option explicit. Always.

Strong typing

VBA facilitates strong typing. This should be used because it stops VBA guessing at types which improves performance and reduces the chance of coding errors. Especially as some types/objects have default values that are not well documented!

This is a function, so what does it return? At the moment, it returns a Variant which might be OK, but your code does not reflect this requirement.

Public Function discrete(r1 As Range, r2 As Range) As String

If you want it to be Variant, then be explicit and state that!

Also, another gotcha is the way variables are declared. The following lines are not doing what you think they are doing:

Dim arr1, arr2 As Variant
Dim min, max As Double

In these lines, both the first variables are not declared and thus they default to Variant. Again, if you want a variable to be a Variant, then be explicit (like you tried to be in the first line above). What you really wanted was:

Dim arr1 As Variant, arr2 As Variant
Dim min As Double, max As Double

By the way, kudos for declaring variables near where you are using them.

Proper qualification

Always put proper qualification on your objects. Excel uses defaults for things that are not properly qualified - the active sheets and cells may not be what you want! Especially in a UDF and you could have multiple workbooks open!

Range(r2(1), r2(1).End(xlDown))

should be

wb.ws.Range(r2(1), r2(1).End(xlDown)) ' wb and ws is something you define here - more notes below.

Default value for discrete

You only set the value for discrete in a conditional (If) statement in a loop. What if it never fires? What would you like the user to see?

A simple option is to set a default value at the beginning - the most obvious is an error message. For this to work, the UDF should return a variant:

Public Function discrete(r1 As Range, r2 As Range) As Variamt
    discrete = CVErr(xlErrValue) '<-- pick an error value to suit yourself

Now that we have set that up - what are some other ways this can fail?

user picks ranges that re not one column wide?
user picks two ranges that are a different size?

Now you can test for this early and then exit the function - the default value will be an error and the user will have an indicator of this.

Altering the parameters

This is bad:

Function discrete(r1 As Range, r2 As Range)
    […]
    Set r1 = Range(r1(1), r1(1).End(xlDown))

A big no-no. You should always consider the parameters on a UDF as immutable. If you want to reset the ranges, then use a temporary variable within the Function scope (e.g. Dim tempRange1 as Range).

However, you are double-guessing the user. What if they did not want to go to the end of the page? What if they had other stuff under the table?

By doing this manipulation, you are increasing the chances that the value arrays will be mismatched. arr1(found,1) may no longer have an equivalent arr2(found,1)!

Working with arrays

Yes, always practice working with arrays where you can. Even replace the Application.Sum call. You can never tell when your user is going to set up a 1000-value discrete array!

Some suggestions

Use only a single range in the UDF call (Public Function
discrete(twoColTable As Range) As Variant. This reduces the
opportunity for user error. You array will be two columns
arr(found,1) will correspond with arr(found,2)
Check your user input immediately for size and values to make sure
you are getting what you think you should. If column count does not
equal 2, or your second column is not ascending numbers then exit
with an error.
Traverse the array from beginning until you find a value that is
greater then your n. The value you want will be the one before you hit that value (i.e. go back one array index). Alternatively, traverse the array from the end until you find a value that is not greater than your n. This way, you do not need max or min.


Correlated subquery slow I have a table that looks like this

Directory nvarchar(max)
Extension nvarchar(10)
Length bigint

I also have another table that provides the type of file (lookup table)
extension nvarchar(10)
FileType nvarchar(50)

That being said I have a correlated subquery I wanted to migrate to a sproc that takes in the levels from the root and gives you back the directories and their file types at that specific level.  There are about 400k rows in the main table and about 800 extensions in the lookup table.

The query is this:

declare @levelsFromRoot as int = 7
--declare @auditName as varchar(max) = 'Aetna'

select
    ef.Type,
    sf.Directory,
    (select count(distinct part)
     FROM dbo.vwAuditView
     cross APPLY dbo.SplitPath( substring([Directory],1,LEN([Directory])

                        -CHARINDEX('\',REVERSE([Directory]))) ,'\') AS Results
                                where Directory = sf.Directory) as [LevelsFromRoot]
from dbo.FS02V_SourceFiles sf
inner join dbo.ExtensionFix ef
    on sf.Extension = ef.Extension
Where (select count(distinct part)
     FROM dbo.vwAuditView
     cross APPLY dbo.SplitPath( substring([Directory],1,LEN([Directory])

                        -CHARINDEX('\',REVERSE([Directory]))) ,'\') AS Results
                                where Directory = sf.Directory) = @levelsFromRoot
order by Directory asc

The cross apply section in the subquery counts the levels in each path starting from the root.  you should be able to pass in how many levels from the root and pass back a list of directories at that level and their corresponding file types.  This query has already been running 25 minutes.  It is returning data to me.  Is there anything I can do to improve this one?  I'm fairly new at correlated subqueries and figure I am doing something wrong here.

For clarity the view looks like this:

select    Directory
        --, ef.Extension
        , ef.Type
        , sum(Length) as [Size - Bytes]
from dbo.FS02V_SourceFiles sf
INNER JOIN dbo.ExtensionFix ef
    on sf.Extension = ef.Extension
group by ef.type, Directory

The solution was to refactor the function that splits the directories and counts:

ALTER FUNCTION [dbo].[Split]
(
    @RowData nvarchar(max),
    @SplitOn nvarchar(5)
)
RETURNS @RtnValue table
(
    Id int identity(1,1),
    Data nvarchar(max)
)
AS
BEGIN
    Declare @Cnt int
    Set @Cnt = 1

    While (Charindex(@SplitOn,@RowData)>0)
    Begin
        Insert Into @RtnValue (data)
        Select
            Data = ltrim(rtrim(Substring(@RowData,1,Charindex(@SplitOn,@RowData)-1)))

        Set @RowData = Substring(@RowData,Charindex(@SplitOn,@RowData)+1,len(@RowData))
        Set @Cnt = @Cnt + 1
    End

    Insert Into @RtnValue (data)
    Select Data = ltrim(rtrim(@RowData))

    Return
END

This new version is a ton more efficient and returns data in about 18 seconds vs. 30+ minutes.


How to improve this functional python fast exponentiation routine? I have the following python functions for exponentiation by squaring :

def rep_square(b,exp):
        return reduce(lambda sq,i: sq + [sq[-1]*sq[-1]],xrange(len(radix(exp,2))),[b])

def exponentiate(b,exp):
        return reduce(lambda res,(sq,p): res*sq if p == 1 else res,zip(rep_square(b,exp),radix(exp,2)),1)

They work. Calling print exponentiate(2,10), exponentiate(3,37) yields :

1024 450283905890997363

as is proper. But I am not happy with them because they need to calculate a list of squares. This seems to be a problem that could be resolved by functional programming because :

Each item in the list only depends on the previous one
Repeated squaring is recursive

Despite people mentioning that recursion is a good thing to employ in functional programming, and that lists are not good friends -- I am not sure how to turn this recursive list of squares into a recursive generator of the values that would avoid a list. I know I could use a stateful generator with yield but I like something that can be written in one line.

Is there a way to do this with tail recursion? Is there a way to make this into a generator expression?

The only thing I have thought of is this particularly ugly and also broken recursive generator :

def rep_square(n,times):
    if times <= 0:
        yield n,n*n
    yield n*n,list(rep_square(n*n,times-1))

It never returns.

You haven't said anything against itertools, so here's how I'd do it with generators:

from itertools import compress
from operator import mul

def radix(b) :
    while b :
        yield b & 1
        b >>= 1

def squares(b) :
    while True :
        yield b
        b *= b

def fast_exp(b, exp) :
    return reduce(mul, compress(squares(b), radix(exp)), 1)


Generate random numbers without repetitions I want to generate a list of N different random numbers:

public static List<int> GetRandomNumbers(int count)
{
    List<int> randomNumbers = new List<int>();

    for (int i=0; i<count; i++)
    {
        int number;

        do number = random.Next();
        while (randomNumbers.Contains(number));

        randomNumbers.Add(number);
    }

    return randomNumbers;
}

But I feel there is a better way.  This do...while loop especially makes this ugly. Any suggestions on how to improve this?

Updated answer in response to bounty: See Is that your final answer? at the end, and other changes - basically answer is significantly rewritten.

To break your problem down in to requirements:

you need a set of random numbers
the numbers need to be unique
the order of the returned numbers needs to be random

Your current code indicates that the range of random numbers is specified by Random.Next(), which returns values in the [0 .. Int32.MaxValue) range (note, it excludes Int32.MaxValue). This is significant for the purpose of this question, because other answers have assumed that the range is configurable, and 'small'.

If the range should be configurable, then the recommended algorithm would potentially be much larger.

Based on those assumptions, let's do a code review...

Code Style

do ... while

The most glaring problems here are the un-braced do-while loop. You already knew it, but this code is ugly:

    do number = random.Next();
    while (randomNumbers.Contains(number));

It really should be braced:

    do
    {
        number = random.Next();
    } while (randomNumbers.Contains(number));

This makes the statement clear, and significantly reduces confusion. Always use braces for 1-liners.

List Construction

The List class allows an initial capacity to be used. Since the capacity needs to just be count, it makes sense to initialize the list with this capacity:

List<int> randomNumbers = new List<int>(count);

Current Algorithm

This is where the most interesting observations can be made. Let's analyze your current algorithm:

Create a container for the results
repeat until you have selected N values:

Select a random value
check if it has been previously selected
if it is 'new', then add it to the container

This algorithm will produce random values, in a random order, and with good random characteristics (no skews, biases, gaps, etc.).

In other words, your results are good.

The problem is with performance....

There are two performance concerns here, one small, the other large:

the do-while loop to avoid collisions
the List container

do-while performance

The do-while has a very low impact on performance... almost negligible. This is hotly debated, but, the reality is that you would need a very, very large count before this becomes a problem. The reasoning is as follows:

Collisions happen when the random value was previously selected. For the specified range of [0 .. Int32.MaxValue), you would need a very large count before collisions actually happened. For example, count would have to be about 65,000 before there was better than a 50% chance that there was even a single collision.

In a general sense, given a Range of \$N\$, select \$M\$ numbers. If \$M < \sqrt{N}\$ then the probability of a single collision is < 50%. Since the Range is very large, the probability is small.

Obviously, if the range was small, then the probabilities would be significantly affected. But the range is fixed at Int32.MaxValue, so that's OK.

Additionally, if the count was large, then the probabilities would also be affected. How large would be very large? Well, you would be running in to very large arrays before you run in to significant problems..... I would venture you are hitting close to \$\frac{Int32.MaxValue}{2}\$ before you run in to a significant issue with performance.

List performance

This is without doubt your largest concern. You use the randomNumbers.Contains(number) call to determine whether a value was previously selected. This requires a scan of all previously-selected values to determine. As mentioned, this will almost always return false, and will thus have to scan the entire list.

As the count value increases, the length of time to perform the Contains will increase at an quadratic rate, \$O(n^2)\$ where n is count.

This performance problem will become critical much sooner than the random-collision problem.

Putting it together

The problem you have in your code is that you are trying to do too much at once, you are using a List because that is your return value, when a HashSet would be better. If you break the problem down in to stages, you will be able to solve things more elegantly.

If you add a duplicate value to a HashSet, it does not grow, and the operation performance is not dependent on the amount of data in the HashSet (it is \$O(1)\$). You can use the Count of the HashSet to manage the data uniqueness.

Once you have a clean set of unique random numbers, you can dump them in to a List, then shuffle the list using an efficient shuffle.

Combining these data structures, in the right way, leads to an overall \$O(n)\$ solution, which will scale fairly well.

Here is some code, which works in Ideone too. Note, my C# is weak, so I have tried to make the logic clear.

using System;
using System.Collections.Generic;

public class Test
{
    static Random random = new Random();

    public static List<int> GenerateRandom(int count)
    {
        // generate count random values.
        HashSet<int> candidates = new HashSet<int>();
        while (candidates.Count < count)
        {
            // May strike a duplicate.
            candidates.Add(random.Next());
        }

        // load them in to a list.
        List<int> result = new List<int>();
        result.AddRange(candidates);

        // shuffle the results:
        int i = result.Count;
        while (i > 1)
        {
            i--;
            int k = random.Next(i + 1);
            int value = result[k];
            result[k] = result[i];
            result[i] = value;
        }
        return result;
    }
    public static void Main()
    {
        List<int> vals = GenerateRandom(10);
        Console.WriteLine("Result: " + vals.Count);
        vals.ForEach(Console.WriteLine);
    }
}

The above code is my initial recommendation, and it will work well, and scale well for any reasonable number of values to return.

Second Alternate Algorithm

The problem with the above algorithm is threefold:

When count is very large, the probability of collision is increased, and performance may be affected
Data will need to be in both the HashSet and the List at some point, so the space usage is doubled.
The shuffle at the end is needed to keep the data in a random order (HashSet does not keep the data in any specific order, and the hashing algorithm will cause the order to become biased, and skewed).

These are only performance issues when the count is very large. Note that only the collisions at large count will impact the scalability of the solution (at large count it is no longer quite \$O(n)\$, and it will be come progressively worse when count approaches Int32.MaxValue. Note that in real life this will not likely ever happen.... and memory will become a problem before performance does.

@JerryCoffin pointed to an alternate algorithm  from Bob Floyd, where a trick is played to ensure that collisions never happen. This solves the problem of scalability at very large counts. It does not solve the need for both a HashSet and a List, and it does not solve the need for the shuffle.

The algorithm as presented:

initialize set S to empty
for J := N-M + 1 to N do
    T := RandInt(1, J)
    if T is not in S then
        insert T in S
    else
        insert J in S

assumes that RandInt(1, J) returns values inclusive of J.

To understand the above algorithm, you need to realize that you choose a random value from a range that is smaller than the full range, and then after each value, you extend that to include one more. In the event of a collision, you can safely insert the max because it was never possible to include it before.

The chances of a collision increase at the same rate that the number of values decreases, so the probability of any one number being in the result is not skewed, or biased.

Is this almost a final answer? No

So, using the above solution, in C#, would look like (in Ideone) (note, code is now different to Ideone):

public static List<int> GenerateRandom(int count)
{
    // generate count random values.
    HashSet<int> candidates = new HashSet<int>();
    for (Int32 top = Int32.MaxValue - count; top < Int32.MaxValue; top++)
    {
        Console.WriteLine(top);
        // May strike a duplicate.
        if (!candidates.Add(random.Next(top + 1)))
        {
            candidates.Add(top);
        }
    }

    // load them in to a list.
    List<int> result = candidates.ToList();

    // shuffle the results:
    int i = result.Count;
    while (i > 1)
    {
        i--;
        int k = random.Next(i + 1);
        int value = result[k];
        result[k] = result[i];
        result[i] = value;
    }
    return result;
}

Note that you need to shuffle the results still, so that the HashSet issue is resolved. Also note the need to do the fancy loop-condition top > 0 because at overflow time, things get messy.

Can you avoid the shuffle?

So, this solves the need to do the collision loop, but, what about the shuffle. Can that be solved by maintaining the HashSet and the List at the same time. No! Consider this function(in Ideone too):

public static List<int> GenerateRandom(int count)
{
    List<int> result = new List<int>(count);

    // generate count random values.
    HashSet<int> candidates = new HashSet<int>();
    for (Int32 top = Int32.MaxValue - count; top < Int32.MaxValue; top++)
    {
        // May strike a duplicate.
        int value = random.Next(top + 1);
        if (candidates.Add(value))
        {
            result.Add(value);
        }
        else
        {
            result.Add(top);
            candidates.Add(top);
        }
    }

    return result;
}

The above answer is never going to allow the first value in the result to have any of the Max - Count to Max values (because there will never be a collision on the first value, and the range is not complete at that point), and this is thus a broken random generator.

Even with this collision-avoiding algorithm, you still need to shuffle the results in order to ensure a clean bias on the numbers.

TL;DR

Is This Your Final Answer? Yes!

Having played with this code a lot, it is apparent that it is useful to have a range-based input, as well as a Int32.MaxValue system. Messing with large ranges leads to potential overflows in the 32-bit integer space as well.

Working with @mjolka, the following code would be the best of both worlds:

    static Random random = new Random();

    // Note, max is exclusive here!
    public static List<int> GenerateRandom(int count, int min, int max)
    {

        //  initialize set S to empty
        //  for J := N-M + 1 to N do
        //    T := RandInt(1, J)
        //    if T is not in S then
        //      insert T in S
        //    else
        //      insert J in S
        //
        // adapted for C# which does not have an inclusive Next(..)
        // and to make it from configurable range not just 1.

        if (max <= min || count < 0 ||
                // max - min > 0 required to avoid overflow
                (count > max - min && max - min > 0))
        {
            // need to use 64-bit to support big ranges (negative min, positive max)
            throw new ArgumentOutOfRangeException("Range " + min + " to " + max +
                    " (" + ((Int64)max - (Int64)min) + " values), or count " + count + " is illegal");
        }

        // generate count random values.
        HashSet<int> candidates = new HashSet<int>();

        // start count values before max, and end at max
        for (int top = max - count; top < max; top++)
        {
            // May strike a duplicate.
            // Need to add +1 to make inclusive generator
            // +1 is safe even for MaxVal max value because top < max
            if (!candidates.Add(random.Next(min, top + 1))) {
                // collision, add inclusive max.
                // which could not possibly have been added before.
                candidates.Add(top);
            }
        }

        // load them in to a list, to sort
        List<int> result = candidates.ToList();

        // shuffle the results because HashSet has messed
        // with the order, and the algorithm does not produce
        // random-ordered results (e.g. max-1 will never be the first value)
        for (int i = result.Count - 1; i > 0; i--)
        {
            int k = random.Next(i + 1);
            int tmp = result[k];
            result[k] = result[i];
            result[i] = tmp;
        }
        return result;
    }

    public static List<int> GenerateRandom(int count)
    {
        return GenerateRandom(count, 0, Int32.MaxValue);
    }


Get  texts into a tab separated string table_with_headers = $(this.dom.table).find("thead tr th").map(->
  $(this).text()
).get().join("\t")

Can one write it nicer? I especially don't like the inner function in map syntax enforced by CoffeeScript and jQuery.

You can use explicit find like so

$ 'thead tr th', @dom.table

and put some indentations instead of braces

table_with_headers = $ "thead tr th", @dom.table
.map -> @.text()
.join '\t'

Also try to use list comprehesion

table_with_headers = [
  $(th).text()
  for th in
  $ "thead tr th", @dom.table
].join '\t'


Calculate temperature based on differing regression coefficients The following function calculates temperature based off of a given suction pressure, where different regression coefficients are assigned depending on the value of the pressure.

The regression equation is fairly standard, of the form:

$$ax^2 + bx + c$$

where \$x\$ is the pressure, and \$a\$, \$b\$, and \$c\$ are the coefficients. There are three different set of coefficients that are outlined in the code. I apply the regression equation by mapping the current pressure to the coefficients with LINQ.

I would like to optimize this function, as it takes in as a parameter a very large array of pressures, and needs to perform this calculation on every element in the array, and return those numbers as either an array or a list.

public static List<double> GetSSTList(double[] suctionPressureList)
{
    var result = new List<double>();
    var coefficientList = new List<double[]>() {
        new double[] { -28.43, 1.96582, -0.1582 },
        new double[] { -27.955, 2.4866, -0.055 },
        new double[] { -17.178, 0.5952, -0.0048 }
    };

    foreach (var psig in suctionPressureList)
    {
        double[] coefficients;

        if (psig >= -9 && psig <= -4)
        {
            coefficients = coefficientList[0];
        }
        else if (psig > -4 && psig <= 9)
        {
            coefficients = coefficientList[1];
        }
        else if (psig > 9 && psig <= 85)
        {
            coefficients = coefficientList[2];
        }
        else
        {
            coefficients = null;
        }

        var sst = coefficients.Select(c => c * Math.Pow(psig, Array.IndexOf(coefficients, c))).Sum();
        result.Add(sst);
    }

    return result;
}

There are a couple of things you can do to speed this up.

Set the initial capacity of result

The List<T>() constructor uses a default initial capacity. When the number of elements in the list goes beyond its capacity, the list must be resized. We can avoid resizing the list by setting the initial capacity of result to suctionPressureList.Length.

Remove the call to Array.IndexOf.

Assuming that each element in coefficients is unique, we can remove the call to Array.IndexOf. We can do this by either using the overload of Select that gives us the index of the element:

var sst = coefficients.Select((c, i) => c * Math.Pow(psig, i)).Sum();

or we can manually do the summation:

double sst = 0;
for (var i = 0; i < coefficients.Length; i++)
{
    sst += coefficients[i] * Math.Pow(psig, i);
}

With these changes, these are the results that I got for an input array of size 1,000,000:

BenchmarkDotNet-Dev=v0.9.3.0+
OS=Microsoft Windows NT 6.2.9200.0
Processor=Intel(R) Core(TM) i7-6650U CPU @ 2.20GHz, ProcessorCount=4
Frequency=2156248 ticks, Resolution=463.7685 ns
HostCLR=MS.NET 4.0.30319.42000, Arch=64-bit RELEASE [RyuJIT]

Type=Foo  Mode=Throughput

   Method |      Median |    StdDev |
--------- |------------ |---------- |
      New |  80.4571 ms | 2.6079 ms |
 Original | 220.0254 ms | 9.9705 ms |


Building session-variables from a SQL stored procedure Calling code and attempt at building session variables:

DataTable dtServerVars = clCommonFunctions.TryAutoLogin("europe\\MrTest");
Session["CablingUserID"]= dtServerVars.Rows[0]["CablingUserID"].ToString();
Session["CablingUseremail"] = dtServerVars.Rows[0]["CablingUseremail"].ToString();
Session["CablingLogin"] = dtServerVars.Rows[0]["CablingLogin"].ToString();
Session["CablingPassword"] = dtServerVars.Rows[0]["CablingPassword"].ToString();
Session["CablingPersonnel"] = dtServerVars.Rows[0]["CablingPersonnel"].ToString();
Session["CablingSurname"] = dtServerVars.Rows[0]["CablingSurname"].ToString();
Session["CablingFirstName"] = dtServerVars.Rows[0]["CablingFirstName"].ToString();
Session["CablingSuperUser"] = dtServerVars.Rows[0]["CablingSuperUser"].ToString();
Session["CablingDateAdded"] = dtServerVars.Rows[0]["CablingDateAdded"].ToString();
Session["CablingContact"] = dtServerVars.Rows[0]["CablingContact"].ToString();
Session["CablingApprovalAuthority"] = dtServerVars.Rows[0]["CablingApprovalAuthority"].ToString();
Session["CablingAdminUser"] = dtServerVars.Rows[0]["CablingAdminUser"].ToString();
Session["SharedInfoID"] = dtServerVars.Rows[0]["SharedInfoID"].ToString();
Session["SharedInfousername"] = dtServerVars.Rows[0]["SharedInfousername"].ToString();
Session["SharedInfopassword"] = dtServerVars.Rows[0]["SharedInfopassword"].ToString();
Session["SharedInfoname"] = dtServerVars.Rows[0]["SharedInfoname"].ToString();
Session["SharedInfoemail"] = dtServerVars.Rows[0]["SharedInfoemail"].ToString();
Session["SharedInfoICLlocation"] = dtServerVars.Rows[0]["SharedInfoID"].ToString();
Session["SharedInfoPhone"] = dtServerVars.Rows[0]["SharedInfoPhone"].ToString();
Session["SharedInfoSecLevel"] = dtServerVars.Rows[0]["SharedInfoSecLevel"].ToString();
Session["IMSUserID"] = dtServerVars.Rows[0]["IMSUserID"].ToString();
Session["IMSUserName"] = dtServerVars.Rows[0]["IMSUserName"].ToString();
Session["IMSIsAnonymous"] = dtServerVars.Rows[0]["IMSIsAnonymous"].ToString();
Session["IMSLastActivityDate"] = dtServerVars.Rows[0]["IMSLastActivityDate"].ToString();
Session["loggedin"] = "unknown";

Code being called:

public static DataTable TryAutoLogin(string strREMOTE_USER)
{
    SqlConnection siConnection = new SqlConnection();
    siConnection.ConnectionString = Databases.getDbConnectionString("csSharedInfo");
    siConnection.Open();
    SqlCommand seCmd = new SqlCommand("GetSignOnDetails", siConnection);
    seCmd.CommandType = CommandType.StoredProcedure;
    seCmd.Parameters.Add(new SqlParameter("@DomainAccount", SqlDbType.NVarChar, 300));
    seCmd.Parameters["@DomainAccount"].Value = strREMOTE_USER;
    seCmd.CommandType = CommandType.StoredProcedure;
    SqlDataAdapter sda = new SqlDataAdapter();
    seCmd.Connection = siConnection;
    sda.SelectCommand = seCmd;
    DataTable dtServerVars = new DataTable();
    sda.Fill(dtServerVars);
    siConnection.Close();
    if (dtServerVars != null)
    {
        if (dtServerVars.Rows.Count > 0)
    {
        return dtServerVars;
    }
}
    return null;
}

First your TryAutoLogin:

A SqlConnection is IDisposable. Use it in combination with using.
You have some duplicate lines of code, like seCmd.CommandType = CommandType.StoredProcedure;.
dtServerVars can never be null.
You do not have to open the connection, SqlDataAdapter will do that for you.
Since you are using only ONE datarow, just return one datarow.

Resulting in:

 public static DataRow TryAutoLogin(string strREMOTE_USER)
 {
      using(SqlConnection siConnection = new SqlConnection(Databases.getDbConnectionString("csSharedInfo")))
       {
           SqlCommand seCmd = new SqlCommand("GetSignOnDetails", siConnection);
           seCmd.CommandType = CommandType.StoredProcedure;
           seCmd.Parameters.AddWithValue("@DomainAccount", strREMOTE_USER);
           SqlDataAdapter sda = new SqlDataAdapter(seCmd);
           DataTable dtServerVars = new DataTable();
           sda.Fill(dtServerVars);
           if (dtServerVars.Rows.Count > 0)
                return dtServerVars.Rows[0];
           return null;
      }
 }

Having done this, the next piece of code will also be a lot easier:

DataRow drServerVars = clCommonFunctions.TryAutoLogin("europe\\MrTest");
Session["CablingUserID"]= drServerVars["CablingUserID"].ToString();
Session["CablingUseremail"] = drServerVars["CablingUseremail"].ToString();
Session["CablingLogin"] = drServerVars["CablingLogin"].ToString();
...


When should I call set_exception_handler with this exception handler class? This is a simplified version of ExceptionHandler.php:

<?php
class ExceptionHandler
{
    public function __construct()
    {
        set_exception_handler(array($this, 'handle'));
    }

    public function handle($exception)
    {
        // output some information about the exception
    }
}

I'm wondering where the best place is to call set_exception_handler:

Register itself in the constructor, as above? Usage would simply be:

$exceptionHandler = new ExceptionHandler;

Move the set_exception_handler call into a separate method, and call after instantiation:

$exceptionHandler = new ExceptionHandler;
$exceptionHandler->register();

Outside of the class entirely:

$exceptionHandler = new ExceptionHandler;
set_exception_handler(array($exceptionHandler, 'handle'));

Some general things to consider:

As stated in the docs, the handler's signature is: void handler ( Exception $ex ), note the type-hint. Below is a lengthy explanation as to why this type-hint is important.
Your handler is not static (nor should it be). set_exception_handler expects the argument to be callable: set_exception_handler ( callable $exception_handler ). What if I destroy your class? The handler is not unregistered. I'd like to have a method that calls the restore_exception_handler function. Removing the handler this class registers.
Add properties that capture the exception handlers' state prior to registering your handler, so you can restore it correctly, and allow users to pass arguments to the contructor that allow them to control the behavior of your handler
Whenever you're writing echo in a class method, take a step back. Think of a case where you really don't want to echo everything to the end-user. If your handler catches an unhandled PDOException instance, do you really want the end-user to see everything about how, when and where you're trying to connect to the DB? Probably not.

There are a lot of things you should think about when writing something like an exception handler. It's a component that plays a vital part of any sizable application. Therefore, it should allow a lot of customization, and it should be extendable. This, in turn, implies that your base handler is as generic as is humanly possible. Generic code is hard to get right, so it's a very good exercise. I'll leave you with these critiques for now. If you want to, apply them to your code, and perhaps submit the result for another review here later.
Also take a peek at how other projects implement exception handling (Symfony2, Zf2, Codeignitor, ... they're all open source. Copy-pasting their code is not allowed, but you can look at them for inspiration)
Am I calling set_exception_handler in a suitable place?
IMHO, not really. If I use an exception handler class, I'd expect to be able to set it up some more. I want to customize a thing or two like: where does the exception handler send its output to, how should the exception handler format the exceptions. I may want to set the handler up differently depending on the environment (production behavior vs development).
When I see a handler as minimalist as yours (minimalism is a good thing in code), I'd probably think about extending it, to best suite my needs. I might add a logger property, which I can use to write output to (instead of echoing it). I could also choose to log the exceptions to the DB, or have the handler call a webservice. Your class makes this rather difficult to do:
You've probably come across references to the SOLID principles. An important part of this, and one that is too often overlooked or not understood, is the Liskov principle. This states that, in order for a child to comply with the substitution principle, it is not allowed for that child to strengthen the contract it inherits from the parent. What does that mean, well let's look at your handle method's signature:
public function handle($exception)

If I extend your class, but I don't want this handle method to handle objects that aren't instances of the Exception class, I might want to add a type-hint:
class MyHandler extends ExceptionHandler
{
    public function handle(Exception $exception)//only accepts Exception instances
    {}
}

This, according to the Liskov principle is not allowed. Because MyHandler extends your class, any code using my child class has to be able to assume that the contract (number of arguments, type requirements and visibility of members) is the same. Meaning: public properties remain public, protected properties have to be at least protected, too (public is allowed, private is not).
By my adding this simple type-hint, I'm breaking this rule. To clarify, take this example:
class Model
{
    public $data = null;
    public function setData($value)
    {
        $this->data = $value;
        return $this;
    }
}
class ArrayModel extends Model
{
    protected $data = [];//<-- reduce visibility
    public function getData($offset = null)
    {
        if ($offset === null)
            return $this->data;
        if (!isset($this->data[$offset]))
            return null;
        retrun $this->data[$offset];
    }
    public function setData(array $value)//strengthen argument restrictions
    {
        $this->data = $value;
        return $this;
    }
}

Now, at first glance, you may not see the problem: I'm using a generic Model class as a data container, and extend it to create a data container that only deals with arrays. But let's say there's a piece of code that looks like this:
function initData(Model $model)
{
    if ($model->data === null)
        $model->setData('initialized');
    return $model;
}

This function uses a type-hint to ensure that the $model argument will be an instance of the Model class. Because this is guaranteed to be the case (if not, PHP will generate a fatal error), it should be safe to assume that the data property is public, and the setData method accepts a string as an argument. However, if I were to pass it an instance of ArrayModel, these assumptions are false: data is protected, and setData only accepts an array argument.
Bottom line
Your ExceptionHandler class is too permissive to be extended in a safe, meaningful way.
Now this wouldn't be much of a problem if I were able to do everything I wanted with your class as it stands, but like I said in the beginning: I can't.
Because you simply have to add customization support to your class, and rethink the contract this class imposes, you simply can't call set_exception_handler in the constructor. I want to be able to extend this class, set up everything I need to set up, and then register a handler when, where, and how I want to register it.
I also want to be able to register several handlers, and perhaps unregister them if I need to. Your class seems to have been written under the assumption that no other handlers are registered, and no other handlers will be registered after your handle method. That's just not right.


Binary search tree with tree traversal Although there are several implementations of binary search tree, I have made my own implementation for practice. The following code does the followoing operation
- Create a binary tree
- search through a binary tree
- inorder traversal
- preorder traversal
- breadth first traversal
- depth first traversal(post order)

Please suggest improvements or possible short comming in the code

import static org.junit.Assert.assertEquals;
import java.util.LinkedList;
import java.util.Queue;
import java.util.Stack;
import org.junit.Test;

public class BST {
public TreeNode root = null;

public TreeNode get(int element){
    if(root == null){
        return null;
    }
    TreeNode runner = root;
    while (true){
        if(runner.data > element){
            if(runner.leftNode == null){
                return null;
            }
            runner = runner.leftNode;
        } else if(runner.data < element) {
            if(runner.rightNode == null){
                return  null;
            }
            runner = runner.rightNode;
        } else {
            return runner;
        }
    }
}

public void insert(int element){
    if(root == null){
        root = new TreeNode(element);
        return;
    }
    TreeNode runner = root;
    while (runner.data != element){
        if(runner.data > element){
            if(runner.leftNode == null){
                runner.leftNode = new TreeNode(element);
                return;
            }
            runner = runner.leftNode;
        } else {
            if(runner.rightNode == null){
                runner.rightNode = new TreeNode(element);
                return;
            }
            runner = runner.rightNode;
        }
    }
}

public static void breathFirstSearch(TreeNode root){
    Queue<TreeNode> queue = new LinkedList<TreeNode>();
    queue.add(root);
    TreeNode runner = root;
    while(!queue.isEmpty()){
    runner = (TreeNode)queue.remove();
    if(runner.leftNode != null){
        queue.add(runner.leftNode);
    }
    if(runner.rightNode != null){
        queue.add(runner.rightNode);
    }
    System.out.println("visited node " + runner.data);
    }
}

public static void depthFirstSearch(TreeNode root){
    Stack<TreeNode> stack = new Stack<TreeNode>();
    if(root == null){
        return;
    }
    TreeNode runner = null;
    stack.add(root);
    while(!stack.empty()){
        runner = stack.peek();
        if(runner.leftNode != null && !runner.leftNode.visited){
            stack.add(runner.leftNode);
            continue;
        }
        if(runner.rightNode != null && !runner.rightNode.visited){
            stack.add(runner.rightNode);
            continue;
        }
        stack.pop();
        runner.visited = true;
        System.out.println("visited node " + runner.data);
    }
}

public static void preOrderTraversal(TreeNode root){
    if(root == null){
        return;
    }
    System.out.print(root.data + " ");
    preOrderTraversal(root.leftNode);
    preOrderTraversal(root.rightNode);
}

public static void inOrderTraversal(TreeNode root){
    if(root == null){
        return ;
    }
    inOrderTraversal(root.leftNode);
    System.out.print(root.data + " ");
    inOrderTraversal(root.rightNode);
}

private class TreeNode {
    public int data;
    public TreeNode leftNode;
    public TreeNode rightNode;
    public boolean visited;
    TreeNode(int data){
        this.data = data;
    }
}

@Test
public void basicTest(){
BST tree = new BST();
int[] data = {9, 5, 3, 1, 4, 8, 15, 11, 21, 20, 29};
for (int i : data){
    tree.insert(i);
}
assertEquals(tree.get(3).data, 3);
}

}

Simplify get

You can simplify the get method like this:

public TreeNode get(int element) {
    TreeNode runner = root;
    while (runner != null) {
        if (runner.data > element) {
            runner = runner.leftNode;
        } else if (runner.data < element) {
            runner = runner.rightNode;
        } else {
            return runner;
        }
    }
    return null;
}

(Thanks @david-harkness for pointing out the extra shortcut!)

Breadth-first search

It's called breadth-first, not "breath-first".

Also, this initialization is pointless, as you overwrite the value of runner immediately in the loop:

Queue<TreeNode> queue = new LinkedList<TreeNode>();
queue.add(root);
TreeNode runner = root;
while (!queue.isEmpty()) {
    runner = (TreeNode) queue.remove();

And, the cast is also pointless, because the Queue is correctly declared with TreeNode type.

Depth-first search

The depthFirstSearch method will set all node.visited = true, so you won't be able to call it twice. Actually it's really bad to have side effects like this. It would be better if TreeNode didn't have the visited field at all. It's polluting the class with data only used by one specific purpose, the depth-first search. It doesn't belong there. You could track what was visited using a Set, used only in depthFirstSearch.

TreeNode

I would rename leftNode to left and rightNode to right. It's simpler and everybody understands what they are.

Since you never need to change data, it can be final.

The current implementation is limited to int values. It would be better if the BST could store anything comparable.

Change the class declaration to this:

public class BST<T extends Comparable<T>> {

Likewise for TreeNode
Replace all comparison operations like x.data < value with x.compareTo(value) < 0, likewise for >

Unit testing

Your basic test could easily become more useful (and less basic) if you do a get not only for one random element, but for all elements you inserted, since you have the array of values ready anyway:

@Test
public void basicTest() {
    BST tree = new BST();
    int[] data = {9, 5, 3, 1, 4, 8, 15, 11, 21, 20, 29};
    for (int i : data) {
        tree.insert(i);
    }
    for (int i : data) {
        assertEquals(i, tree.get(i).data);
    }
}

A single test case named basicTest is not going to make this class "properly tested". At the very least you could add a few more test cases that are obviously necessary when talking about a binary search tree.

For example, what happens if you try to get non-existent elements?

@Test
public void testNoSuchElement() {
    BST tree = new BST();
    assertNull(tree.get(3));
    assertNull(tree.get(31));
}

The BST should not allow duplicate values:

@Test
public void testNoDuplicateVaues() {
    BST tree = new BST();
    int val = 3;
    tree.insert(val);
    tree.insert(val);
    tree.insert(val);
    assertEquals(1, tree.size());
}

Of course this requires a .size method, so implement one, and a test case with it too:

public int size() {
    int count = 0;
    Queue<TreeNode> queue = new LinkedList<TreeNode>();
    queue.add(root);
    TreeNode runner;
    while (!queue.isEmpty()) {
        runner = queue.remove();
        if (runner != null) {
            ++count;
            queue.add(runner.leftNode);
            queue.add(runner.rightNode);
        }
    }
    return count;
}

@Test
public void testSize() {
    assertEquals(0, new BST().size());
    BST tree = new BST();
    int[] data = {9, 5, 3, 1, 4, 8, 15, 11, 21, 20, 29};
    for (int i : data) {
        tree.insert(i);
    }
    assertEquals(data.length, tree.size());
}

Formatting

The formatting is messy:

the indentation is broken in breathFirstSearch and in basicTest
you should put a space before and after parentheses, for example:

while(!stack.empty()){ should be formatted as while (!stack.empty()) {
if(root == null){ should be formatted as if (root == null) {

Basically, use your IDE to format code correctly. It also makes it easier to review. (For yourself too!)


Generic Int conversion to scalar I'm new to Scala and I'm trying to write a generic function to convert from Int to any scalar.

For example:

val res: Float = convertInt[Float](12)
val res: Double = convertInt[Double](12)
//etc

I've achieved the following code:

  trait FromIntConverter[T]{
    def apply(int: Int): T
  }

  def convertInt[T](int: Int)(implicit converter: FromIntConverter[T]): T = converter(int)

  implicit val IntToIntConverter = new FromIntConverter[Int]{
    def apply(int: Int) = int
  }

  implicit val IntToFloatConverter = new FromIntConverter[Float]{
    def apply(int: Int) = int.toFloat
  }

  implicit val IntToDoubleConverter = new FromIntConverter[Double]{
    def apply(int: Int) = int.toDouble
  }
  //etc

Is it ok in regards to readability/brevity/performance?

It's ok in regards of readability, but it can be shortened.

There is no need to create a dedicated trait FromIntConverter with implementations. All that can happen anonymously.

The convertInt function can take a function as implicit parameter:

def convertInt[T](i: Int)(implicit converter: Int => T) = converter.apply(i)

And all the implicit vals can become one-liners:

implicit def intToFloat(i: Int) = i.toFloat

implicit def intToDouble(i: Int) = i.toDouble

P.S. I discourage the use of int as variable name, that shouldn't collide with the Java type name.


Permutation and combination calculator I wanted to make something for the Community Challenge, so I made a simple permutation and combination calculator. I'd like input on:

The overall design - any suboptimal choices that could be improved?
Naming - This is hard, I know. But is it confusing to follow? Some names seem repetitive, others seem overly long.
Any general improvements I could make.

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class PermutationCombinationCalculator extends Application {
    @Override
    public void start(Stage stage) {
        TextField entry = new TextField();
        entry.setPromptText("Enter a value to permutate/combine here");

        TextField limiter = new TextField();
        limiter.setPromptText("Enter S, where S is the number of selections possible");

        Label permutationResult = new Label("Result will be here!");
        Label combinationResult = new Label("Result will be here!");

        Button permutate = new Button("Permutate");
        permutate.setOnAction(e -> {
            permutationResult.setText(
                computePermutation(entry.getText(), limiter.getText())
            );
        });

        Button combine = new Button("Combine");
        combine.setOnAction(e -> {
            combinationResult.setText(
                computeCombination(entry.getText(), limiter.getText())
            );
        });

        HBox permutationLayout = new HBox();
        permutationLayout.getChildren().addAll(permutate, permutationResult);

        HBox combinationLayout = new HBox();
        combinationLayout.getChildren().addAll(combine, combinationResult);

        VBox layout = new VBox();
        layout.getChildren().addAll(entry, limiter, permutationLayout, combinationLayout);

        stage.setScene(new Scene(layout));
        stage.show();
    }

    private static String computePermutation(String entry, String limit) {
        int total;
        int select;

        if (limit.length() == 0) {
            try {
                total = Integer.parseInt(entry);
                return Integer.toString(permutate(total));
            } catch(Exception e) {
                return "Invalid input";
            }
        }

        try {
            total = Integer.parseInt(entry);
            select = Integer.parseInt(limit);
        } catch(Exception e) {
            return "Invalid input";
        }
        return Integer.toString(permutate(total, select));
    }

    private static String computeCombination(String entry, String limit) {
        int total;
        int select;

        try {
            total = Integer.parseInt(entry);
            select = Integer.parseInt(limit);
        } catch(Exception e) {
            return "Invalid input.";
        }
        return Integer.toString(combine(total, select));
    }

    private static int permutate(int p, int r) {
        int result = 1;
        for (int i = p; r > 0; r--, p--) {
            result *= p;
        }

        return result;
    }

    private static int permutate(int p) {
        return permutate(p, p);
    }

    private static int combine(int total, int select) {
        return permutate(total) / (permutate(select) * permutate(total - select));
    }

    public static void main(String[] args) {
        launch(args);
    }
}

Download a runnable Jar here (Requires JRE 8)

Catch specific exceptions, avoid Exception

This is a good example where it's a bad idea to catch Exception:

try {
    total = Integer.parseInt(entry);
    return Integer.toString(permutate(total));
} catch(Exception e) {
    return "Invalid input";
}

The catch here was clearly intended for parsing errors,
but what if something goes wrong in permutate due to some bug?
The bug will get masked under an "Invalid input" response.

It's good to catch the most specific exception type.
It also communicates to readers of the code what exactly can go wrong with the code.

Limit the body of the try block as much as possible

Still on the previous example,
catching NumberFormatException is a good first step, but not enough.
You should also move the permutate call outside the try-catch block,
to avoid catching NumberFormatException thrown by permutate instead of the Interger.parseInt(entry) line.

Sure, if you look at the implementation of permutation this is impossible now,
but who knows, you might add a change later.
In general, it's not practical to verify all calls going out from a try block for potential hidden bugs.
It's a lot easier to just move such calls outside of the try block
and eliminate all suspicions and potential future bugs.

Avoid repeated logic

The computePermutation method has some duplication of logic in the parsing of total.
You could rearrange the code to do it only once.

Avoid duplicated string literals

The message "Invalid input." appears multiple times.
It would be good to move it to a constant.

Check empty strings with isEmpty

Instead of limit.length() == 0,
a more idiomatic way is limit.isEmpty().


Return product ID, if it is specified in a form I often encounter this problem - I need to get some value, if it exists, and then do something with that value.

Getting that value becomes a separate method, and since that value might not exist, I need to indicate that somehow. For example, I might have a method that returns a productId if a product is attached to the form the user is filling out. If it doesn't exist, I can return 0, which is an invalid productId, which is a magic number.

My current problem with this instance is this:

public int GetProductIdFromFormIfAny(JObject form)
{
  int productId = 0; //default to 0
  IList<FormComponentMetadata> formFields = ParseFormForDropdowns(form);
  bool isProductAttached = formFields.Any(field => field.DataLookupTable == "Products");
  if(isProductAttached){
    FormComponentMetadata productField = formFields
      .Single(field => field.DataLookupTable == "Products");
    string filterStringContainingProductId = productField.DataLookupFilter;
    productId = filterStringContainingProductId.Split("=")[1];
  }
  return productId;
}

I don't know how to get around the magic number issue. I can create a new class, which holds an id and a bool indication whether or not that id exists, but that seems very heavy-handed and a LOT of extra code that someone has to maintain, even though the tradeoff is clarity.

As a side note, this is what comes back for the filterStringContainingProductId: "data_lookup_filter":"Product_ID=9" - I am not sure how to make the Split("=")[1] clear without comments or constants, which, again, seem heavy.

Edit: Looks something like this when refactored to use nullable types.

 public int? GetProductIdFromFormIfAny(JObject form)
{
  IList<FormComponentMetadata> formFields = ParseFormForDropdowns(form);
  bool isProductAttached = formFields.Any(field => field.DataLookupTable == "Products");
  int? productIdOrNull = isProductAttached ?GetExistingProductId(formFields) : default(int?);
  return productIdOrNull;
}

private int GetExistingProductId(IList<FormComponentMetadata> formFields)
{
  const string keyValueSeparatorChar = "=";
  const int idxOfProductId = 1;
  FormComponentMetadata productField = formFields
  .Single(field => field.DataLookupTable == "Products");
  string filterStringContainingProductId = productField.DataLookupFilter;
  string productIdSegment = filterStringContainingProductId.Split(keyValueSeparatorChar)[idxOfProductId];
  int productIdAsInt = int.Parse(productIdSegment);
  return productIdAsInt;
}

Seems like a ton of code for something so trivial...

It looks like your code would benefit from a few well-chosen utility methods. It also looks a bit 'stringly typed': things would be much easier if you could store that product ID in an integer field or property in the first place. But it's difficult to tell whether that is possible without more context.

A reusable method that can fetch a specific value from a filter string would have the biggest impact. A method that makes it easier to fetch specific fields from a form might also be useful. And don't forget about the ?. operator. With those, your code can be rewritten as following:

public int? GetProductIdFromFormIfAny(JObject form)
{
    // Returns null if no matching field is found:
    var productsField = GetFieldForTable(form, "Products");

    // Returns null if the given filter is null, or if it does not contain the specified key.
    // Note that the 'magic' index 1 is replaced by a more descriptive key name:
    var productIDValue = GetFilterValue(productsField?.DataLookupFilter, "Product_ID");

    // Returns null if the given string is null or not a valid integer:
    return int.TryParse(productIDValue, out var productID) ? productID : null;
}

I can't tell whether this is exactly what you need - maybe you'll want an exception to be thrown if a filter is malformed, for example - but it should give you some ideas.


Maximum profit earned on weighted un-directed tree I came across this problem while giving a sample test.

  The problem was that we have given a tree which is undirected. We can
  start from any node of our choice. Initially we have power "P" and
  while going from one node to other node we loose some power "X"
  (consider as cost of travelling) and earn some profit "Y". So we need
  to tell that what is the maximum profit that we can earn with a given
  power ? Every node can be visited only once.

Example: First line contains number of nodes and initial power

Next n-1 lines contains node-node-cost-profit

5 4

1 2 1 2

1 3 2 3

1 4 2 4

4 5 2 2

Answer => 7. We can start from 4 and go to 1 and than to 3.

I have applied DFS on this to get maximum profit earned by traversing every single path.

But is there a way to decrease time ??? Can we apply floyd warshall algorithm to calculate distances each edges ??

from collections import defaultdict
class tree:
    def __init__(self,nodes):
        self.nodes = nodes
        self.graph = defaultdict(list)
    def add(self,a,b,charge,profit):
        self.graph[a].append([b,charge,profit])
        self.graph[b].append([a,charge,profit])
    def start(self,power):
        maxi = -1
        visited = [False for i in range(self.nodes)]
        for i in range(1,self.nodes+1):
            powers = power
            visited[i-1] = True
            for j in self.graph[i]:
                temp = self.dfs(j,powers,0,visited)
                if temp > maxi:
                    maxi = temp
            visited[i-1] = False
       return maxi

    def dfs(self,node,powers,profit,visited):
        v,p,pro=node[0],node[1],node[2]
        if powers-p < 0:
            return 0
        if powers-p == 0:
            return profit + pro
        profit += pro
        powers = powers-p
        visited[v-1] = True
        tempo = profit
        for k in self.graph[v]:
            if visited[k[0]-1] == False:
                temp = self.dfs(k,powers,tempo,visited)
                if temp > profit:
                    profit = temp
        visited[v-1] = False
        return profit

t = tree(5)
t.add(1,2,1,2)
t.add(1,3,2,3)
t.add(1,4,2,4)
t.add(4,5,2,2)
print(t.start(4))

Firstly, use a PEP8 checker. It will raise a lot of issues about whitespace and also tell you to change

            if visited[k[0]-1] == False:

to either

            if visited[k[0]-1] is False:

or

            if not visited[k[0]-1]:

The DFS is far more complicated than necessary:

We don't need visited: given that we know that the graph is a tree, it suffices to track the previous vertex.
I'm used to DFS tracking vertices, not edges.
There's no reason for dfs to take profit as an argument: the profit that can be made in a rooted subtree is independent of the profit made getting there.

Rewriting with those notes in mind, we can reduce it to

    def start(self, power):
        return max(self.dfs(u, power, None) for u in range(1, self.nodes+1))

    def dfs(self, u, power, prev):
        return max((edge[2] + self.dfs(edge[0], power - edge[1], u)
                    for edge in self.graph[u]
                    if edge[0] != prev and edge[1] <= power),
                   default=0)

However, I think this is still inefficient in some cases. It's doing \$n\$ depth-first searches, for overall time \$\Theta(n^2)\$. But those searches are traversing the same vertices. I believe (although I haven't written the code to verify it) that it should be possible to do it in \$\Theta(np)\$ where \$p\$ is the starting power. The idea would be to do one DFS, but for each subtree to return a complicated structure containing the largest score available solely within that subtree, and the largest scores available from the root of that subtree for different powers (which would obviously be monotonically increasing). Then it would be necessary to combine these.

(Floyd-Warshall is cubic time, so that would be worse. Similarly, matrix-based all-pairs-shortest-path algorithms for general graphs would be worse).


Training diary calendar I often use jQuery plugins for front-end functionality in web applications. This requires me to write code like this, where I am assigning a function to an object property:

var trainingDiary = $("#training_diary").fullCalendar({
  dayClick: function(date, allDay, jsEvent, view) {

    var title = prompt('Event Title:');
    var dayObj = $(this);
    //console.log($(this));
    if (title) {
     $.ajax({
      url: "/app_dev.php/addtrainingday",
      global: false,
      type: "POST",
      data: "dayNo=" + date.getDate(), //returns day of month. getDay() returns day of week
      async:true,
      success: function(msg) {

               trainingDiary.fullCalendar('renderEvent',
                {
                  title: title,
                  start: date,
                  allDay: allDay,
                  backgroundColor:'#cccccc'
                },
            true // make the event "stick" across calendar pages
          )

      dayObj.css({'background-color':'#339933','background-image':'url()'})
      console.log(msg.valueOf());
      } //end ajax success
     })
   } else {
      trainingDiary.fullCalendar('unselect');
   }

  },

  theme: true,
  header: {left:'title',
            center: '',
            right: 'today prev next'},
  firstDay:1

});

The code becomes hard to read and maintain as there is now a success function within an Ajax function within yet another function. How can I improve the code so that dayClick: function(...) can call a function defined outside of the fullCalendar call? In this case, fullCalendar is a jQuery plugin.

You should use the JavaScript Module Pattern to create a private scope for your function declarations:

// Directives for JSLint
/*global console, prompt, $ */ // declaration of global variables used
/*jslint unparam: true, sloppy: true, white: true */ // disable less useful checks

// create a closure for private scope
(function(){

  // Private declarations

  var calendarOptions, // object, options for calendar diary
      trainingDiary;   // object, training diary using jQuery plugin fullCalendar()

  function onDayClick(date, allDay, jsEvent, view) {
    // Function: onDayClick(day, allDay, jsEvent, view)
    // Callback for the selection of a day in the calendar.
    //
    // Parameters:
    //   date - type?, description?
    //   allDay - type?, description?
    //   jsEvent - type?, description?
    //   view - type?, description?

    // single var declaration
    var title,       // string, event title, input by user
        dayObj,      // object, jQuerified DOM element
        ajaxOptions; // object, options for AJAX call to addtrainingday script

    // this declaration must be nested to access date, allDay... in closure scope
    function onAjaxSuccess(msg) {
      // Function: onAjaxSuccess(msg)
      // Callback for a successful AJAX call to server-side addtrainingday script.
      //
      // Parameter:
      //   msg - string, message received from server, for logging purpose

      var event = {
        title: title,
        start: date,
        allDay: allDay,
        backgroundColor:'#cccccc'
      };

      trainingDiary.fullCalendar(
        'renderEvent',
        event,
        true // make the event "stick" across calendar pages
      ); // added missing semicolon

      dayObj.css({
        'background-color':'#339933',
        'background-image':'url()'
      }); // added missing semicolon

      console.log(msg.valueOf());

    } // end of onAjaxSuccess declaration

    // Init
    title = prompt('Event Title:');
    dayObj = $(this);
    ajaxOptions = {
      url: "/app_dev.php/addtrainingday",
      global: false,
      type: "POST",
      // getDate() returns day of month. getDay() returns day of week
      data: "dayNo=" + date.getDate(),
        async: true,
        success: onAjaxSuccess
    };

    //console.log($(this));
    if (title) {
      $.ajax(ajaxOptions); // added missing semicolon
    } else {
      trainingDiary.fullCalendar('unselect');
    }

  } // end of onDayClick declaration

  // Initialization code

  calendarOptions = {
    dayClick: onDayClick,
    theme: true,
    header: {
      left: 'title',
      center: '',
      right: 'today prev next'
    },
    firstDay: 1
  };

  trainingDiary = $("#training_diary").fullCalendar(calendarOptions);

}()); // the function is called immediately to make the declarations effective

I also created extra variables to hold options objects, to enhance readability, and I added comments to describe the intent and parameters of functions declared.

Note that trainingDiary is now declared in the private scope as well. If you need to access it in the global scope, you will need to export it to the global object by assigning it to global this or window:

(function(){

  // Private declarations
  (...)

  // Init
  (...)

  // Public declarations
  this.trainingDiary = trainingDiary;
}());

I checked the code with latest JSLint; it is a useful tool once you know what to expect from it, e.g. by reading "JavaScript: The Good Parts" from its author Douglas Crockford. It would definitely help you to spot the missing semicolons, which can cause unexpected issues when you minify your code.


Is this good design for custom email template? I read effective Java by Joshua Bloch. And it said use enums over int constant. So I was thinking instead of using hard coded strings I could also use enums. The forgotpassword.txt and confirmation.txt are plain text resource files.

When the class is loaded, I get the contents of the text file and store it inside of a map, since enums are only loaded once.

When I want to create an email template I pass key value map to the EmailTemplate.[TemplateName].create () method and it produces an email text. Anytime I want to create a new template I just have to go to one place and add it.

The only down-side I could think of is if you have hundreds of different email templates. Do you think this is a good design?

public enum EmailTemplate
{
    FORGOT_PASSWORD("forgotpassword.txt"),
    CONFIRM_EMAIL("confirmation.txt");

    private final String mFile;

    public String getFile()
    {
        return mFile;
    }

    EmailTemplate(String file)
    {
        mFile = file;
    }

    public String create(Map<String, String> valuesMap)
    {
        //lazy load file content into template
        if (!lookup.containsKey(this))
        {
            lookup.put(this, FileUtil.getFileContents((this.getFile())));
        }
        StrSubstitutor sub = new StrSubstitutor(valuesMap);
        return sub.replace(lookup.get(this));
    }

    private static final Map<EmailTemplate, String> lookup = new HashMap<EmailTemplate, String>();
}

public class EmailGenerator
{
    public static void sendConfirmationEmail(String emailAddress,
            String confirmCode)
    {
        Map<String, String> valuesMap = new HashMap<String, String>();
        valuesMap.put("link", confirmCode);

        String text = EmailTemplate.CONFIRM_EMAIL.create(valuesMap);
        System.out.println(text);
    }

    public static void sendContactUs(String subject, String email, String message, String name)
    {
        Map<String, String> valuesMap = new HashMap<String, String>();
        valuesMap.put("name", name);
        valuesMap.put("text", message);
        valuesMap.put("email", email);

        String text = EmailTemplate.CONTACT_US.create(valuesMap);
        System.out.println(text);
    }
}

Don't use plain words as replacement keys since you might accidentally erase a real "name".  For example, in your template, use "Hello {{name}}," and you add "{{" and "}}" to the word you want to replace before calling replace.  It's easier to do that than to read each template carefully to make sure the keywords do not appear anywhere where they should not be changed.
Hide EmailTemplate inside EmailGenerator and make it private.  EmailTemplate is a "dirty" class that should not be exposed publicly: the method create(Map<String, String> valuesMap) must not be accessible since you need exactly the right arguments in the map.
You don't even really need EmailTemplate.  If you don't want to reread the files all the time, you can use a "lazy" map, Map<String, String> textFilesCache (file name -> file text).  When you fetch a file, you define a method that checks if textFilesCache(filename) is null and, if it is, it loads the file and adds it to textFilesCache.  It's even more efficient than the enum since the template files you never use will never be read.  An enum seems overkill here and it make the code less readable.
(If you want to have some fun, you can even take a look at WeakHashMap.  It's overkill here since you will only have a few files, but if you had very many and very large files, the JVM can drop some values of a WeakHashMap to free memory.)


HashMap or other collection? I need to parse a space- delimited text file. I want to read the file and do operations for each cell. Here is how I am parsing the file:

String line;
BufferedReader reader = new BufferedReader(new FileReader(file));
Map<String,String> mapFile = new HashMap<String, String>(19);
while((line = reader.readLine()) != null) {
    mapFile.put("valueCol1", line.substring(0,19));
    mapFile.put("valueCol2", line.substring(19,45));
    mapFile.put("valueCol3", line.substring(45,50));
    mapFile.put("valueCol4", line.substring(50,54));
}

Is this right, or should I use other types of collections?

It really depends on what you're using the map for.
As already raised, you are overwriting the map for each line you read.

If you want to do something with the data later then I'd recommend creating a custom object with the fields for each column. Then you could store it in a list or a map with one of the values as a key (the value should be unique to prevent clashes) for future use.

If you're operating on it within the line read loop then you don't even need a data structure, just operate on the data right there and then.

Really until we know what you want to do with the data it is hard to say if you're using the correct data structure.

As a final note you state it is space delimited, I'd probably use String.split(" ") to create an array of your column data instead of using absolute references in your substring call, this would allow for cases where the data is larger than your substring range but still delimited by a space, unless you're absolutely sure the data is in a fixed format and is clean.


Connecting to a MySQL database with C# Can anyone help me with checking this code? It is for connecting to a database. The method getFromDatabase is to take values from the database and put it to a dataset, and the other method getFromDatabaseToCombo is to get from the database to a comboBox.
I can't shake off this feeling that the last two methods can be combined somehow and something is not right in this class.

(the variables will be set to private later)

   public class dbConnect
   {
    public MySqlConnection con;//declare variable for connection
    public MySqlCommand com;//varialbe for command
    public DataTable dt;
    public DataSet ds;
    public DataView myview;

    public dbConnect()  //Constructor
    {
        string serverName = "localhost"; //server name
        string dbName = "airline_db";//database name
        //string pass = "***"; //password
        string uid = "root";
        string conString = "server="+serverName+";User Id="+uid+";database="+dbName+"";

        con = new MySqlConnection(conString); //instance of con object

    }

    public void openConnect() //to open connection to database
    {
        if (con != null && con.State == ConnectionState.Closed)//to check if conn is already open or not
        {
            con.Open();
        }
    }

    public void closeConnect() //to close connection to database
    {
        if (con!=null && con.State == ConnectionState.Open)
        {
            con.Close();
        }
    }

    public void comText(string sqlQuery) //method to use sql
    {
        com = new MySqlCommand(sqlQuery,con);
        //com.ExecuteNonQuery();
    }

    //to get from the database
    public DataTable getFromDatabase(string sqlQuery)
    {
        dt = new DataTable();
        ds = new DataSet();
        MySqlDataAdapter adp = new MySqlDataAdapter();

        comText(sqlQuery);
        adp.SelectCommand = com;
        adp.Fill(ds);
        dt = ds.Tables[0];      //all tables assignted as table[0]
        return dt;
    }
    public void getFromDatabaseToCombo(string sqlQuery) //check with above
    {
        dt = new DataTable();
        ds = new DataSet();
        MySqlDataAdapter adp = new MySqlDataAdapter(sqlQuery,"server=localhost;User Id=root;database=airline_db");
        adp.Fill(dt);
    }
}
}

Naming conventions:

You should follow the C# design guidelines and use PascalCase for public members, class names and methods. camelCase is for private members.

Give your members and methods meaningful names. Names like con or com don't mean anything, except for you. Try using sqlConnection and sqlCommand instead.

Method names should indicate what they do and/or return. So a method name like getFromDatabase is bad practice. What are you getting from the database? Instead, use GetUsers, GetRecords or GetData. Try to be specific, only use general names like the last example if there's no other way.

This also applies to the other methods, use Open or OpenConnection instead of openConnect. (same for Close)

String.Format:

Don't concatenate all sorts of variables with string constants. Use the String.Format method. Place the format and add your values as arguments. This results in cleaner code that is easier to read and maintain. Example, change this:

string conString = "server="+serverName+";User Id="+uid+";database="+dbName+"";

to:

var connectionString = String.Format("server={0};User Id={1};database={2}", server, userID, database);

Constructor + overloading:

Your constructor contains the values you use for the connectionstring. Don't place these values here as it makes the use of the constructor very limited. Create a separate method that will create the connection and call this method from the constructor. The default values you are using can be placed in a settings file or make them private constant fields:

private const string Server = "localhost";
private const string Database = "airline_db";
private const string UserID = "root";

Also, create another constructor that takes parameters so you can create a connection with other values. Combined you can overload the constructor and put all the logic in one. This looks clean and makes maintaining your code a lot easier. Example (classname is DatabaseConnector):

public DatabaseConnector() : this(Server, UserID, Database)
{
    //This will call the other constructor with the const values
}

public DatabaseConnector(string server, string userID, string database)
{
    mySqlConnection = CreateConnection(server, userID, database);
}

private MySqlConnection CreateConnection(string server, string userID, string database)
{
    var connectionString = String.Format("server={0};User Id={1};database={2}", server, userID, database);
    return new MySqlConnection(connectionString);
}

Methods:

comText is something I don't understand. It is a void method but doesn't do anything important enough to be a separate method. Let it return a MySqlCommand instead. Rewritten:

public MySqlCommand InitSqlCommand(string query)
{
    var mySqlCommand = new MySqlCommand(query, mySqlConnection);
    return mySqlCommand;
}

Your DataTable and DataSet variables should be scoped inside the methods. There's no reason they should be global in the class. This way you can rewrite your getFromDatabase to following:

public DataTable GetData(string query)
{
    var dataTable = new DataTable();
    var dataSet = new DataSet();
    var dataAdapter = new MySqlDataAdapter { SelectCommand = InitSqlCommand(query) };

    dataAdapter.Fill(dataSet);
    dataTable = dataSet.Tables[0];
    return dataTable;
}

Things you'll also see changed is, the use of:

an object initializer for initializing members
the var keyword, this looks cleaner and you let the compiler do the job

Your Open and Close method should be private. The change in state should be determined by the class itself. Otherwise you can close the connection and try executing some command.

Dispose:

When using instances that handle unmanaged resources sucha as streams, connections, ... you have to dispose them. Otherwise they will not be claimed by the garbage collector. There are two ways to do this:

Manually call the Dispose() method:

private void Finalize()
{
    if (mySqlConnection != null && mySqlConnection.State == ConnectionState.Closed)
        mySqlConnection.Dispose();
}

Use the using statement when using the commands.

More reading:

IDisposable.Dispose Method
using Statement (C# Reference)
Naming Guidelines
Object and Collection Initializers


Parser for tab-delimited data as a subclass of StringReader I wanted to parse lines of text as tab-delimited items, and I had just been using a StringReader for something else and I came up with this:

class TabDelimitedFieldReader : StringReader
    {
        private string _nextLine;
        public TabDelimitedFieldReader(string s)
            : base(s)
        {
        }

        public IEnumerable<string> ReadFields()
        {
            if (_nextLine != null)
            {
                var fields = _nextLine.Split('\t');
                foreach (string field in fields)
                {
                    yield return field;
                }
            }
        }

        public bool HasMore
        {
            get
            {
                _nextLine = this.ReadLine();
                return (_nextLine != null);
            }
        }
    }

I can't think of another time I directly subclasses a .NET framework class like this.  I figure maybe I'm just strange and it's normal for others, or maybe there's a reason I wouldn't want to do it.

Conceptually, I do not see anything inherently wrong with subclassing a framework object, particularly since many of them provide virtual/abstract members specifically to allow it.

In this case, it may be better to wrap a TextReader object instead of inheriting from StringReader.  This would allow you to provide tab delimited reading for multiple data sources instead of limiting yourself to in-memory strings.  For example, you could still pass in a StringReader for reading strings, but you could also pass in a StreamReader if you wanted to support reading from files instead.

Additionally, I noticed that HasMore is dangerous to call due to its side-effect.  It is not immediately obvious to a caller that checking HasMore would alter the reader's current position.  I would suggest using Peek instead.

You would then need to update _nextLine elsewhere.  You could have it be a side-effect of calling ReadFields.  Or, perhaps better, you could add another method (e.g., AdvanceLine) that is simply responsible for moving the reader to the next line.


How to verify if value already added to the list and if so, then update the value? I use 3 lists to store field name, its value and score. If field name is already added to according list, then I just need to update other 2 lists. If it is not there, then I need to add it. I do the following:

user = User.get_by_key_name(user_key_name)
if user:
    field_names = user.field_names
    field_values = user.field_values
    field_scores = user.field_scores
    if field_name in user.field_names:
        # field_names[field_index] = field_name
        field_index = user.field_names.index(field_name)
        field_values[field_index] = field_value
        field_scores[field_index] = field_score
    else:
        field_names.append(field_name)
        field_values.append(field_value)
        field_scores.append(field_score)
    user.field_names = field_names
    user.field_values = field_values
    user.field_scores = field_scores
    user.put()

Is it correct approach?

user = User.get_by_key_name(user_key_name)
if user:

Python convention is to use user is None when checking against None

    field_names = user.field_names
    field_values = user.field_values
    field_scores = user.field_scores

Why copy these references? It doesn't help you much.

    if field_name in user.field_names:
        # field_names[field_index] = field_name
        field_index = user.field_names.index(field_name)

Use index to check if field_name is present. That way it doesn't have to search through the lists twice.

        field_values[field_index] = field_value
        field_scores[field_index] = field_score

If possible, avoid maintaing parrallel lists. In this case, it looks like maybe you should have a dictionary of strings to tuples.

    else:
        field_names.append(field_name)
        field_values.append(field_value)
        field_scores.append(field_score)
    user.field_names = field_names
    user.field_values = field_values
    user.field_scores = field_scores

Unless user does something odd, this has no effect. Assignment doesn't copy in python, it just copies the references.

    user.put()